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\$

2 Answers 2

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\$ Dec 30, 2014 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, 2014 at 4:35
  • 1
    \$\begingroup\$ A note to future readers, "Enumerable" requires System.Linq \$\endgroup\$ Dec 30, 2014 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\$ Dec 31, 2014 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\$ Dec 31, 2014 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\$ Jan 1, 2015 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\$ Jan 1, 2015 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\$ Jan 1, 2015 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.