Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
        }
    }
}
share|improve this question

5 Answers 5

up vote 15 down vote accepted

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!

share|improve this answer
    
To get around those multi-threading issues, use a static initialization block, which is guaranteed to be run exactly once. –  WChargin Apr 4 at 16:33
    
@WChargin AFAIK private static final Map<String, String> mMap = Collections.unmodifiableMap(initializeMapping()); is also guaranteed to only run once. The initialization is done as a static final variable. –  Simon André Forsberg Apr 4 at 16:38

You could change

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

to

return mMap.get(code);

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

share|improve this answer
1  
Thanks palacsint.. –  user755806 Apr 3 at 12:13
1  
Actually, you should. It is shorter and leverages the api better. You could add a comment (for ppl less familiar with the Map api) telling if the key is not contained, null is returned –  RobAu Apr 4 at 15:31

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.

share|improve this answer

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.

share|improve this answer

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;
        }
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.