5
\$\begingroup\$

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 a Dictionary<TileType, int> in the first place?

Using the Dictionary (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 with TryGetValue 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 the int 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.

\$\endgroup\$
4
\$\begingroup\$

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 use Enumerable.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 bit too 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 that GenerateDefaults is a void. As such, it's not immediately clear that it modifies the tileCosts class variable. I would redefine it to return int[]. It's a few more keystrokes to call it, but it becomes obvious where tileCosts 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;
        }
    }   
\$\endgroup\$
3
  • 1
    \$\begingroup\$ 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! \$\endgroup\$ – Raven Dreamer Dec 30 '14 at 4:28
  • \$\begingroup\$ Ahh yes. You're right about that. Thanks! Another thing I meant to mention, but forgot to... If you deserialize XML into a UnitTileCosts 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. \$\endgroup\$ – RubberDuck Dec 30 '14 at 4:35
  • 1
    \$\begingroup\$ A note to future readers, "Enumerable" requires System.Linq \$\endgroup\$ – Raven Dreamer Dec 30 '14 at 4:59
3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ 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. \$\endgroup\$ – Raven Dreamer Dec 31 '14 at 1:25
  • \$\begingroup\$ 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. \$\endgroup\$ – craftworkgames Dec 31 '14 at 12:58
  • \$\begingroup\$ 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. \$\endgroup\$ – Raven Dreamer Jan 1 '15 at 15:34
  • \$\begingroup\$ 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. \$\endgroup\$ – craftworkgames Jan 1 '15 at 21:51
  • \$\begingroup\$ 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. \$\endgroup\$ – Raven Dreamer Jan 1 '15 at 22:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.