Mapping enum keys to instance values

Question 1

I am working on a pathfinding program / algorithm, and I have the following class:

[System.Serializable]public class UnitTileCosts{    int[] tileCosts;    public UnitTileCosts()    {        //empty constructor uses default value of 1 for all fields        GenerateDefaults();    }    public UnitTileCosts(Dictionary<TileType,int> costs)    {        GenerateDefaults();        foreach(var pair in costs)        {            tileCosts[(int) pair.Key] = pair.Value;        }    }    void GenerateDefaults()    {        tileCosts = new int[System.Enum.GetNames(typeof(TileType)).Length];        for(int i = 0; i < tileCosts.Length; i++)        {            tileCosts[i] = 1;        }    }    public int GetTileCost(TileType tile)    {        return tileCosts[(int) tile];    }}

Is this better or worse than simply using aDictionary<TileType, int> in the first place?

Using theDictionary (that is passed in to the constructor), I would need to check if the key exists because I only care about those values that differ from the result. (Which, admittedly, is easily doable withTryGetValue and an OUT parameter)

I would think that an array of ints would serialize better than a dictionary, to boot.

Is casting from the enum to theint acceptable in this instance? I understand that enums are designed to avoid magic numbers, but in this case I only care about the numbers (and not the magic), so I think it might be reasonable.

Question 2

I'm not sure if you're gaining anything from this wrapper class or not, but there is an alternative for initializing an array to have all the same value at each position. You could useEnumerable.Repeat instead of the loop.

void GenerateDefaults(){    tileCosts = Enumerable.Repeat(1, System.Enum.GetNames(typeof(TileType)).Length).ToArray();}

But perhaps the one liner is a bittoo terse to be readable.

void GenerateDefaults(){    var length = System.Enum.GetNames(typeof(TileType)).Length;    tileCosts = Enumerable.Repeat(1, length).ToArray();}

Whether or not that's an improvement may be debatable and is mostly a matter of preference in my opinion.

Something else I don't care for is the fact thatGenerateDefaults is a void. As such, it's not immediately clear that it modifies thetileCosts class variable. I would redefine it to returnint[]. It's a few more keystrokes to call it, but it becomes obvious wheretileCosts is being modified.

[System.Serializable]public class UnitTileCosts{    int[] tileCosts;    public UnitTileCosts()    {        //empty constructor uses default value of 1 for all fields        tileCosts = GenerateDefaults();    }    public UnitTileCosts(Dictionary<TileType,int> costs)    {        tileCosts = GenerateDefaults();        foreach(var pair in costs)        {            tileCosts[(int) pair.Key] = pair.Value;        }    }

Which begs the question, why set a default value at all if you're just going to overwrite the value a split second later?? It doesn't make sense to me and I would remove that code. At the least, I would make the overloaded constructor call the default constructor to dry things up.

    public UnitTileCosts(Dictionary<TileType,int> costs) : this()    {        foreach(var pair in costs)        {            tileCosts[(int) pair.Key] = pair.Value;        }    }

Question 3

To answer your begged question -- the dictionary may only have a single entry (if only one of the enum, of which there are 10 or so, is overwritten). Wasn't aware you could call the base constructor like that, so thanks!

Question 4

Ahh yes. You're right about that. Thanks! Another thing I meant to mention, but forgot to... If you deserialize XML into aUnitTileCosts class, the default constructor gets called. I don't think it's an issue, but you should be aware of that behavior. Welcome to Code Review. Interesting piece of code there.

Question 5

A note to future readers, "Enumerable" requiresSystem.Linq

Question 6

I could be way off base here, since I don't know the rest of your design, but have you considered changing the enum into a class?

public class UnitTile{    public UnitTile(int tileType)    {        TileType = tileType;        Cost = 1;    }    public int TileType { get; private set; }    public int Cost { get; set; }}

This way you could avoid classes like UnitTileCosts altogether and just generate your various tile types and costs in one place. Perhaps you still store the various tile types in a Dictionary or array, but at least you'd have the ability to add extra properties to them later.

Question 7

The cost is not based on the tile, but based on the combination of the unit + the tile. So that doesn't avoid having to do the lookup.

Question 8

I'm not suggesting you avoid the lookup, I just think the code will be easier to understand and extend this way. As I said though, I can't really tell the whole context of your design.

Question 9

Right, and I'm saying, your suggestion is not tenable to the question asked. The tiles don't have an inherent cost, so making a "UnitTile" class that gives the tiles an inherent cost... accomplishes nothing.

Question 10

Couldn't the UnitTile class be used to represent the combination of the unit + the tile? I obviously don't understand your question properly. If you give some more context perhaps someone can provide an acceptable answer. If not, there's nothing more I can do. Good luck with it.

Question 11

It's for pathfinding -- agents (units) moving around a tiled map. So your suggestion amounts to storing a separate map (of tile costs) per agent, which is non-viable as a matter of combinatorics.

RubberDuck 31.2k6 gold badges74 silver badges177 bronze badges · Accepted Answer · 2014-12-30 04:19:49Z

I'm not sure if you're gaining anything from this wrapper class or not, but there is an alternative for initializing an array to have all the same value at each position. You could useEnumerable.Repeat instead of the loop.

void GenerateDefaults(){    tileCosts = Enumerable.Repeat(1, System.Enum.GetNames(typeof(TileType)).Length).ToArray();}

But perhaps the one liner is a bittoo terse to be readable.

void GenerateDefaults(){    var length = System.Enum.GetNames(typeof(TileType)).Length;    tileCosts = Enumerable.Repeat(1, length).ToArray();}

Whether or not that's an improvement may be debatable and is mostly a matter of preference in my opinion.

Something else I don't care for is the fact thatGenerateDefaults is a void. As such, it's not immediately clear that it modifies thetileCosts class variable. I would redefine it to returnint[]. It's a few more keystrokes to call it, but it becomes obvious wheretileCosts is being modified.

[System.Serializable]public class UnitTileCosts{    int[] tileCosts;    public UnitTileCosts()    {        //empty constructor uses default value of 1 for all fields        tileCosts = GenerateDefaults();    }    public UnitTileCosts(Dictionary<TileType,int> costs)    {        tileCosts = GenerateDefaults();        foreach(var pair in costs)        {            tileCosts[(int) pair.Key] = pair.Value;        }    }

Which begs the question, why set a default value at all if you're just going to overwrite the value a split second later?? It doesn't make sense to me and I would remove that code. At the least, I would make the overloaded constructor call the default constructor to dry things up.

    public UnitTileCosts(Dictionary<TileType,int> costs) : this()    {        foreach(var pair in costs)        {            tileCosts[(int) pair.Key] = pair.Value;        }    }

To answer your begged question -- the dictionary may only have a single entry (if only one of the enum, of which there are 10 or so, is overwritten). Wasn't aware you could call the base constructor like that, so thanks!
Ahh yes. You're right about that. Thanks! Another thing I meant to mention, but forgot to... If you deserialize XML into aUnitTileCosts class, the default constructor gets called. I don't think it's an issue, but you should be aware of that behavior. Welcome to Code Review. Interesting piece of code there.
A note to future readers, "Enumerable" requiresSystem.Linq

Movatterモバイル変換

Stack Exchange Network

Mapping enum keys to instance values

2 Answers2

You mustlog in to answer this question.

Related

Hot Network Questions

Subscribe to RSS