Store and manage a fixed amount of things generally

I have an enum let's say with four values. Let's call them A, B, C, D.

public enum Categories
{
    A,
    B,
    C,
    D
}

I then have a Thing which stores something and has one of the values from my enum.

public class Thing
{
    public final Categories category;
    public int var;

    public Thing (Categories category) {
        this.category = category;
    }
}

All good. Now I know I will have exactly four things. I can change which things, I can set them to null, but I will always have one of each. I will always have exactly one A,B, C and one D. No more no less. So I store them in my Container. Like this.

public class Container
{
    private final Map things = new HashMap<>();

    public Container () {
        for (int i = 0; i < Categories.values().length; i++) {
            things.put(Categories.values()[i], null);
        }
    }

    public Thing getThing (Categories c) {
        return things.get(c);
    }

    public Thing setThing (Categories c, Thing t) {
        Thing returnThing = null;

        if (things.containsKey(c) && t.category == c) {
            if (things.get (c) != null) {
                returnThing = things.get (c);
            }
            things.put(c, t);
        }

        return returnThing;
    }

    public Thing removeThing (Categories c) {
        Thing returnThing = null;

        if (things.containsKey(c) && things.get(c) != null) {
            returnThing = things.get(c);
            things.put (c, null);
        }

        return returnThing;
    }
}

While this code works, it feels a bit clunky. As I know I will never add another key, nor remove one. Is there any pattern I lack the knowledge of that solves this issue? Or, doubt it, is this the way to accomplish a task such as this?

I would prefer to stay away from external dependencies such as Guava.

Enum names are generally given in the singular form, rather than plural. Hence you may want to call your enum Category.

You should also consider using an EnumMap for your Map implementation:

private final Map things = new EnumMap<>(Category.class);

BTW, the return value of Map.put() will be the previously associated value, so you don't need the inner if (things.get (c) != null) { returnThing = things.get(c); } parts of your code (setThing()/removeThing()).

One more observation about your setThing() method: Since you are doing a t.category == c check before the insertion, why not make the method signature just setThing(Thing t)? In that case, the method body simply becomes:

public Thing setThing (Thing t) {
    // t == null check required?
    return things.put(t.category, t);
}

Now, to your question... may I know what's the fixation with exactly four entries in your things object, to the extent of mapping to null values? There is no difference in calling Map.get() for a mapped-to-null key, and an unmapped key: both returns null. The only difference is in Map.containsKey(), but as far as I can tell your class doesn't really expose this to external callers (which I guess is a good design). Is there a more relevant larger context to show how exactly four entries in things are being depended upon?

edit: The code-smell I observe here is that your code is doing/stating the obvious. Consider:

• You use null to practically indicate the absence of a mapped Thing.

• things is pre-populated with all your enum keys, so things.containsKey(category) will always be true.

• In other words, things.containsKey(category) && anotherCondition can be simplified to anotherCondition.

• When you are adding or removing mapped Things, you do a null-check to return the previous non-null mapping...

• but if the mapping is to null, you return a null anyways.

As mentioned above, the relevant methods in Map already returns the previously associated value or null, so your null checks are pretty much redundant.

Unless your code clearly distinguishes between the absence of a mapping and a null-mapping, which I don't see here, you should simplify your logic to treat both as the same. If you do however, then the difference for Map.containsKey() becomes crucial here, and then all the more should you not have null-mappings...

StackExchange Code Review Q#100749, answer score: 5