HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Mapping enum keys to instance values

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
enuminstancekeysvaluesmapping

Problem

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 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 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.

Solution

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 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 costs) : this()
    {
        foreach(var pair in costs)
        {
            tileCosts[(int) pair.Key] = pair.Value;
        }
    }

Code Snippets

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

    tileCosts = Enumerable.Repeat(1, length).ToArray();
}
[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;
        }
    }
public UnitTileCosts(Dictionary<TileType,int> costs) : this()
    {
        foreach(var pair in costs)
        {
            tileCosts[(int) pair.Key] = pair.Value;
        }
    }

Context

StackExchange Code Review Q#75205, answer score: 4

Revisions (0)

No revisions yet.