What's the best practice to "look up" Java Enums?

Question

We have a REST API where clients can supply parameters representing values defined on the server in Java Enums.

So we can provide a descriptive error, we add this lookup method to each Enum. Seems like we're just copying code (bad). Is there a better practice?

public enum MyEnum {
    A, B, C, D;

    public static MyEnum lookup(String id) {
        try {
            return MyEnum.valueOf(id);
        } catch (IllegalArgumentException e) {
            throw new RuntimeException("Invalid value for my enum blah blah: " + id);
        }
    }
}

Update: The default error message provided by valueOf(..) would be No enum const class a.b.c.MyEnum.BadValue. I would like to provide a more descriptive error from the API.

Answer 1

Probably you can implement generic static lookup method.

Like so

public class LookupUtil {
   public static <E extends Enum<E>> E lookup(Class<E> e, String id) {   
      try {          
         E result = Enum.valueOf(e, id);
      } catch (IllegalArgumentException e) {
         // log error or something here

         throw new RuntimeException("Invalid value: " + id);
      }

      return result;
   }
}

Then you can

public enum MyEnum {
   static public MyEnum lookup(String id) {
       return LookupUtil.lookup(MyEnum.class, id);
   }
}

or call explicitly utility class lookup method.

Answer 2

Why do we have to write that 5 line code ?

public class EnumTest {
public enum MyEnum {
    A, B, C, D;
}

@Test
public void test() throws Exception {
    MyEnum.valueOf("A"); //gives you A
    //this throws ILlegalargument without having to do any lookup
    MyEnum.valueOf("RADD"); 
}
}

Answer 3

If you want the lookup to be case insensitive you can loop through the values making it a little more friendly:

 public enum MyEnum {
   A, B, C, D;

      public static MyEnum lookup(String id) {
        boolean found = false;
        for(MyEnum enum: values()){
           if(enum.toString().equalsIgnoreCase(id)) found = true;
        }  
        if(!found) throw new RuntimeException("Invalid value for my enum: " +id);
       }
}

Answer 4

Looks like you have a bad practice here but not where you think.

Catching an IllegalArgumentException to rethrow another RuntimeException with a clearer message might look like a good idea but it is not. Because it means you care about messages in your exceptions.

If you care about messages in your exceptions, then it means that your user is somehow seeing your exceptions. This is bad.

If you want to provide an explicit error message to your user, you should check the validity of the enum value when parsing user input and send the appropriate error message in the response if user input is incorrect.

Something like:

// This code uses pure fantasy, you are warned!
class MyApi
{
    // Return the 24-hour from a 12-hour and AM/PM

    void getHour24(Request request, Response response)
    {
        // validate user input
        int nTime12 = 1;
        try
        {
            nTime12 = Integer.parseInt(request.getParam("hour12"));
            if( nTime12 <= 0 || nTime12 > 12 )
            {
                throw new NumberFormatException();
            }
        }
        catch( NumberFormatException e )
        {
            response.setCode(400); // Bad request
            response.setContent("time12 must be an integer between 1 and 12");
            return;
        }

        AMPM pm = null;
        try
        {
            pm = AMPM.lookup(request.getParam("pm"));
        }
        catch( IllegalArgumentException e )
        {
            response.setCode(400); // Bad request
            response.setContent("pm must be one of " + AMPM.values());
            return;
        }

        response.setCode(200);
        switch( pm )
        {
            case AM:
                response.setContent(nTime12);
                break;
            case PM:
                response.setContent(nTime12 + 12);
                break;
        }
        return;
    }
}

Answer 5

The error message in IllegalArgumentException is already descriptive enough.

Your method makes a generic exception out of a specific one with the same message simply reworded. A developer would prefer the specific exception type and can handle the case appropriately instead of trying to handle RuntimeException.

If the intent is to make the message more user friendly, then references to values of enums is irrelevant to them anyway. Let the UI code determine what should be displayed to the user, and the UI developer would be better off with the IllegalArgumentException.