Java enum containing a hash map for look up

Question

I have the below enum. I need to get the description by code. It is working but can it be improved still?

public enum Maps {

    COLOR_RED("ABC", "abc description");

    private final String code;
    private final String description;
    private static Map<String, String> mMap;

    private Maps(String code, String description) {
        this.code = code;
        this.description = description;
    }
    public String getCode() {
        return code;
    }

    public String getDescription() {
        return description;
    }


    public static String getDescriptionByCode(String code) {
        if (mMap == null) {
            initializeMapping();
        }
        if (mMap.containsKey(code)) {
            return mMap.get(code);
        }
        return null;
    }

    private static void initializeMapping() {
        mMap = new HashMap<String, String>();
        for (Maps s : Maps.values()) {
            mMap.put(s.code, s.description);
        }
    }
}

Answer 1

Your code is very neat and clean, there is only one thing that I would change:

private static Map<String, String> mMap;

to

private static final Map<String, String> mMap = Collections.unmodifiableMap(initializeMapping());

The reasons:

1. By not declaring it final and not using a call to unmodifiableMap it is mutable, and it would be possible to modify the reference using reflection or to use .remove("ABC") on the map. Declaring it as final makes sure that the referenced map cannot change and unmodifiableMap makes sure that no changes can be done to the map itself.

2. Multi-threading issues. As it currently stands, if two threads would call the getDescriptionByCode method at the same time you would initialize the mapping twice, which is not needed.

This also of course requires a slight change in your initializeMapping():

private static Map<String, String> initializeMapping() {
    Map<String, String> mMap = new HashMap<String, String>();
    for (Maps s : Maps.values()) {
        mMap.put(s.code, s.description);
    }
    return mMap;
}

Besides this, it all looks good. Well done!

Answer 2

You could change:

if (mMap.containsKey(code)) {
    return mMap.get(code);
}
return null;

to:

return mMap.get(code);

since HashMap.get() returns null if there is no such key in the map.

Answer 3

This may be just a matter of preference, but it would seem more useful to me to have the map store the instance of Maps rather than the description directly, because then you also get for free the ability to find other information (the enum entry name, its ordinal value, etc). You may not need this, but as it is very simple to do, I would definitely consider it better.

Answer 4

For this class, lazy initialization brings too little benefit relative to the complication it adds. You only have one member in the enum, and each member of the enum is trivially simple to construct. Therefore, you should populate the map at class-loading time using a static initializer block. (As a bonus, it's guaranteed that the initialization happens only once, so it's thread-safe.)

public enum Maps {

    COLOR_RED("ABC", "abc description");

    private final String code;
    private final String description;

    private static final Map<String, String> MAP = new HashMap<String, String>();
    static {
        for (Maps s : Maps.values()) {
            MAP.put(s.code, s.description);
        }
    }

    private Maps(String code, String description) {
        this.code = code;
        this.description = description;
    }                                                                                                                               

    public String getCode() {                                                                                                       
        return code;                                                                                                                
    }                                                                                                                               

    public String getDescription() {                                                                                                
        return description;                                                                                                         
    }                                                                                                                               

    public static String getDescriptionByCode(String code) {                                                                        
        return MAP.get(code);
    }
}

Also, if COLOR_RED doesn't have any useful meaning, and the code strings are also valid Java identifiers, consider using the code itself as the name of each enum member. Enums already have t he ability to look up members by name, so you can take advantage of that mechanism.

public enum SimplerMap {                                                                                                            

    ABC("abc description");

    private final String description;                                                                                               

    private SimplerMap(String description) {                                                                                        
        this.description = description;                                                                                             
    }                                                                                                                               

    public String getCode() {
        return this.toString();                                                                                                     
    }                                                                                                                               

    public String getDescription() {
        return this.description;                                                                                                    
    }                                                                                                                               

    public static String getDescriptionByCode(String code) {
        try {
            return valueOf(code).getCode();
        } catch (IllegalArgumentException noSuchCode) {
            return null;
        }
    }
}

Answer 5

First of all, the names in your code are not very clear. What does Maps mean? why COLOR_RED, and why is the associated code ABC? Do you have only one value in you enum?

Maybe this is just an example code, but this is rather difficult to analyze.

For example, if your value was named ABC, you could simply call Maps.valueOf("ABC") to have the associated enum and get the corresponding description without needing a internal map.

Of course, if you need both Colors and Codes, you will have to relate them somehow. But why even use Strings? Maybe you could have another enumeration called Colors (defining COLOR_RED), so that the relationship between a color and a code is done only using enumerated types.

I haven't written in Java for some time now, but if I remember correctly, EnumMaps (e.g. from Colors to Codes and conversely) are very efficient (like integer indices in an array); moreover, switch case over enums are also more efficient and provide more feedback from the compiler: static analysis can tell you whether you take into account all the possible enums, or not; this simplifies a lot your code because you don't need to bother with error checking.

Of course, Strings are necessary when dealing with data coming from outside of your application. But the can be avoided most of the time when exchanging data internally.

Hope this helps.

Answer 6

Simply having an immutable map initialized within a static block. Also, placing lambdas can achieve higher goal:

private static final Map<String, String> mapCodes = new HashMap<>();     
static {
              final Map<String, String> items = EnumSet.allOf(Maps.class)
                    .stream()
                    /*.filter(// Can do some filtering excluding values)  */
                    .collect(Collectors.toMap(Maps::code, Maps::description));  

              mapCodes.putAll(items);
}

Depending on the level of re-usability, recommended is accessing to unique values plus Enum instances.

.collect(Collectors.toMap(Maps::code, m-> m));

Also, in getter method can reinforce immutability as:

• Guava: ImmutableMap.copyOf(mapCodes) //Recommended: Protects original map values of being mutated

Or:

• Java Collections: Collections.unmodifiableMap(mapCodes)