Using Enum to Handle String literals

Question

I have a java component that was using a lot of string literals that I need to make comparisons on and return booleans based on these comparisons.

In order to make the code more robust I externalized these strings first as class constants, then after other classes started to use these constants I had to separate them to decrease the dependency between the classes.

Knowing that the best practice is not to use variable-classes dedicated for string for many reasons, and since I am using Java 6, I decided to go for enums. below is the implementation that I had in mind

public enum SecurityClassification {
    SENSITIVE("Sensitive"), HIGHLY_SENSITIVE("Highly Sensitive"), PUBLIC("Public"), INTERNAL("Internal");

    private String value;

    private SecurityClassification(String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }

    public boolean hasValue(String param) {
        return value.equalsIgnoreCase(param);
    }

    public static SecurityClassification enumForValue(String param){
        for (SecurityClassification securityClassification : SecurityClassification.values()) {
            if(securityClassification.getValue().equals(param)){
                return securityClassification;
            }
        }
        return null;
    }
}

I was wondering specifically about the enumForValue method, is this an optimal solution? is there any other better way?

Answer 1

It looks fine overall, but...

A few pointers

1. You should make private String value final too though, to clearly indicate that they cannot be modified after instantiation.

2. One thing to note for values() is that it always returns a new array, so for that reason, sometimes it may be recommended to also construct a lookup Map<String, SecurityClassification> to avoid the extra arrays creation.

3. Also, is it really OK to just return null if an invalid security classification is specified here? Depending on your implementation, you may want to consider whether you should throw an IllegalArgumentException here to have a slightly better modelling of such cases.

4. How is hasValue() used? In fact, can it be used in enumForValue() for a case-insensitive comparison?

5. Finally, enumForValue() may seem like a mouthful, you can consider a shorter name like of(). The other thing to consider is that you don't really need to express that it's an enum this method is returning.

Answer 2

In addition to your given advice I'd recommend that your enumerated values have their own distinct lines. It will help extensibility, readability, and eliminate horizontal scrolling.

public enum SecurityClassification {
    SENSITIVE("Sensitive"),
    HIGHLY_SENSITIVE("Highly Sensitive"),
    PUBLIC("Public"),
    INTERNAL("Internal");
//...
}

That pattern is conventional, and makes it easy to alter values, or add a classification exclusive method or constructor.

Another note is that if your values are consistently named according to your current pattern you may consider foregoing the additional variable and simply altering toString:

public enum SecurityClassification {
    SENSITIVE,
    HIGHLY_SENSITIVE,
    PUBLIC,
    INTERNAL;

    @Override
    public String toString() { // or keep getValue() with this implementation
        return name().charAt(0) + name.substring(1).replace('_', ' ');
    }

    // ...
}

You may note, however, that wouldn't retain the capitalization of the proceeding words in a string value. You can modify the method to consider those cases, if desirable. e.g.

public String getValue() {
  StringBuilder value = new StringBuilder();
  for (String word : name().split("_")) {
    value.append(' ').append(word.charAt(0)).append(word.substring(1).toLowerCase());
  }

  return value.substring(1);
}

but then it may arguably become 'costly' and it becomes preferrable to simply store the value, maintaining a constructor for the longer named values.

Answer 3

I have updated my code based on @H.J.K comments. my updates included setting the value field as final and changed the name of enumForValue() to be of() to enhance code readability.

Most importantly added a static initializer that fills the enum values in a Map to cut the time needed every time to call enum values() method and also the loop used to query passed.

The throughput of the code was enhanced by 3 folds (I am using the the system time method which I know can wildly vary between executions, I did 10 runs each and collected the average).

I am adding the code below to indicate the changes added

package com.ibm.ecm.cetest;

import java.util.HashMap;

public enum SecurityClassification {
    SENSITIVE("Sensitive"), HIGHLY_SENSITIVE("Highly Sensitive"), PUBLIC("Public"), INTERNAL("Internal");

    //static map to hold the enums and their values as keys
    final static HashMap<String, SecurityClassification> securityClassifications = new HashMap<String, SecurityClassification>();

    //This will initialize the array so that it won't be called every time in of() method
    static {
        for (SecurityClassification securityClassification : SecurityClassification.values()) {
            securityClassifications.put(securityClassification.getValue(), securityClassification);
        }
    }

    private final String value;

    private SecurityClassification(String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }

    public boolean hasValue(String param) {
        return value.equalsIgnoreCase(param);
    }

    //The method has a new descriptive name, and the Map is now used to query the item   
    public static SecurityClassification of(String param) {
        if (securityClassifications.containsKey(param)) {
            return securityClassifications.get(param);
        } else {
            //Instead of returning null, an exception is thrown if no key was found and hence no corresponding enum
            throw new IllegalArgumentException("Invalid security classification: " + param);
        }
    }
}