3
\$\begingroup\$

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<Categories, Thing> 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.

\$\endgroup\$
5
\$\begingroup\$

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<Category, Thing> 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...

\$\endgroup\$
8
  • \$\begingroup\$ It is used in a very larger project that I am working. Posting it all and in its context would require somewhere around a million worth of bounty... \$\endgroup\$ – Emz Aug 13 '15 at 0:42
  • \$\begingroup\$ @Emz wells not to share all the context, but perhaps the parts where, for some reason, shows why they require things to have exactly four entries while they seem to work reasonably well with null values? That's the part which I don't quite get... \$\endgroup\$ – h.j.k. Aug 13 '15 at 0:51
  • \$\begingroup\$ Think a car, one of the things is the engine, another the wheels etc. Where null means it has none. \$\endgroup\$ – Emz Aug 13 '15 at 3:05
  • \$\begingroup\$ Then it's not exactly five, no more, no less isn't it? More like, at most five? \$\endgroup\$ – Edwin Aug 13 '15 at 3:11
  • \$\begingroup\$ (In this simplified example) a car consists of exterior, wheels, engine and a exhaust pipe. Now if one were to steal one of them (or while they are being replaced) I accept null as saying "the part is missing". \$\endgroup\$ – Emz Aug 13 '15 at 3:35
0
\$\begingroup\$

How about just use a EnumMap initialized with full population.

Map<Categories, Thing> storage = Arrays.asList(Categories.values())
    .stream()
    .collect(Collectors.toMap(
         (c) -> c, 
         (c) -> new Thing(c), 
         (c, c) -> throw new UnsupportedOperationException(), 
         () -> new EnumMap<Categories, Thing>(Categories.class) ));

This way works almost exactly the same as your class. And it conforms to java Collection library. And it does not allow adding new element, since there is just so much value for the Enum. And it works with null.

\$\endgroup\$
1
  • \$\begingroup\$ I will dig into that latter, see what I discover. \$\endgroup\$ – Emz Aug 13 '15 at 3:35

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.