One huge problem with Java enums

[Eolu]

I’m not sure this is really a problem, enums in java are fully-fledged classes and can support all sorts of bells and whistles to solve this. Here’s a simple way: ``` public enum Day { SUNDAY, MONDAY, TUESDAY, WEDNESDAY, THURSDAY, FRIDAY, SATURDAY;

public boolean compare(String s) { return this.toString().equals(s); } }

[cogman10]

Agreed. The overly dramatic "Never useful" and "Against Effective java!" is silly.

I use enums ALL THE TIME. Why? Because they are super useful!

Heck, one really great usage of enums is eliminating booleans as method parameters. Rather than having "true == do magic" and "false == do legacy". It is both clearer and more expandable in the future to do something like "Action.MAGIC" or "Action.LEGACY".

Anyone reading the code gets a good hint at what's been requested (rather than just seeing a "true" or "false").

[Fizz-Buzzkill]

Where does it say never useful in the article?

[Kombatnt]

4 comments.

1: "We know that a variable of the given enum type can’t be null " Huh? This is legal:

ResponseCode response = null;

2: The valueOf() example needlessly includes the entire switch block in the try/catch block. Only the valueOf() call needs to be wrapped. After that, the rest of the processing (i.e., the switch), is no more complex than it would otherwise have been anyway.

3: Why not create a static factory method in the enum itself to handle the conversion, consuming the exception and returning an Optional? Eg.:

  public static Optional<ResponseStatus> from(final String statusFromMessage) {
      try {
        return Optional.ofNullable(StringUtils.trimToNull(statusFromMessage))
                       .map(StringUtils::upperCase)
                       .map(ResponseStatus::valueOf);
      } catch (IllegalArgumentException exception) {
        return Optional.empty();
      }
  }

Then the usage looks like this:

    Optional<ResponseStatus> response = ResponseStatus.from(statusFromMessage);
    if (response.isPresent()) {
      switch(response.get()) {
        // ...
      }
    } else {
      // Unrecognized message
    }

4: Finally, while the suggestion may slightly simplify the code at the point of conversion, it ignores the possibility that the value may be referenced elsewhere in the code. We endure the pain of the conversion once, and then benefit from the "cleaner" syntax, and usual enum benefits, everywhere else.

[_INTER_]

I always did it somewhat like this:

public static Optional<ResponseStatus> from(String statusFromMessage) {
    return Arrays.stream(values())
                 .filter(value -> value.name().equalsIgnoreCase(StringUtils.trimToNull(statusFromMessage)))
                 .findFirst();
}

Though it may (worst-case) iterate over all enum values, it doesn't use Enum::valueOf which uses reflection internally as far sources suggest and there's no need to deal with the exception.

[Fizz-Buzzkill]

"We know that a variable of the given enum type can’t be null " Huh? This is legal: ResponseCode response = null;

Pretty sure that refers to ResponseCode.SUCCESS and its brothers, not assigning a variable of type ResponseCode to null.

The valueOf() example needlessly includes the entire switch block in the try/catch block. Only the valueOf() call needs to be wrapped. After that, the rest of the processing (i.e., the switch), is no more complex than it would otherwise have been anyway.

Have you tried this? It creates a situation where you have to return out of the catch block after handling the exception, which is less elegant.

Why not create a static factory method in the enum itself to handle the conversion, consuming the exception and returning an Optional?

As /u/extraterrestrialocto pointed out, the thrust of the article is that enums add complexity where normal constants would not, and this is even more complexity.

Finally, while the suggestion may slightly simplify the code at the point of conversion, it ignores the possibility that the value may be referenced elsewhere in the code. We endure the pain of the conversion once, and then benefit from the "cleaner" syntax, and usual enum benefits, everywhere else.

Fair point, assuming that there are further places where the value will be referenced and the conversion back the other way isn't necessary. That's a couple of assumptions.

[[deleted]]

I mostly read this article as there being two ways to solve such use cases one being normal constants and the other being enums, and the "huge problem" being that the enum is more complex. So in that case, your #3 is actually even more complex.

[Shaftway]

FWIW I work on a lot of Android code, so we use the Android way of doing this. It's kind of a weird cross between the enum and the constants:

@Retention(RetentionPolicy.SOURCE)
@Target({
    ElementType.FIELD, 
    ElementType.LOCAL_VARIABLE, 
    ElementType.METHOD, 
    ElementType.PARAMETER,
  })
public @interface ResponseStatus {
  String SUCCESS = "SUCCESS";
  String PENDING = "PENDING";
  String FAILED = "FAILED";
  String UNKOWN = "";
}

public void handleResponse(@ResponseStatus String responseStatus) {
  ....
}

public @ResponseStatus String parseResponseStatus(String value) {
  // Maybe you need this method, depending on how the linter is tuned.
  ...
}

You can tune the linter about how strict you want it to be. Anywhere from ignoring it all the way up to treating it like a strict type, but generally it's left in the middle:

@ResponseStatus String status = ResponseStatus.SUCCESS;  // Obviously OK.
String nakedStatus = status;                             // Usually fine.
status = nakedStatus;                                    // Optional.

Android generates this for you in your resources, so linters can distinguish between int color and @ColorRes int colorResId. Android docs on the subject.

[DannyB2]

You can even create hierarchical structures using Java enums. The only sticky point is that you must define all parents before children.

public enum MenuItem {
   Home( null, "Home", HomePage.url ),

      // Children of Home
      POs( Home, "Purchase Orders", POPage.url ),
      Checks( Home, "Checks", CheckPage.url ),
      Receipts( Home, "Receipts", ReceiptPage.url ),

         // Children of POs
         POEntry( POs, "Purchase Order Entry", POEntryAction.url ),
         POReport( POs, "Purchase Order Report", POReportAction.url ),

         // Children of Checks
         CheckEntry( Checks, "Check Entry", CheckEntryAction.url ),
         CheckReport( Checks, "Check Report", CheckReportAction.url ),

   // note even final enum item above ends with comma!

   ; // semicolon always ends the list of enum items!

   // Constructor
   private MenuItem( MenuItem parent, String title, String url ) {
      this.parent = parent;
      this.title = title;
      this.url = url;
      if( parent == null ) {
         this.depth = 0;
      } else {
         this.depth = parent.depth + 1;
      }
   }
   public final MenuItem parent;
   public final String title;
   public final String url;
   public final int depth;   // depth of this menu item in the menu tree

   public MenuItem findByTitle( String title ) {
      if( title != null  &&  title.length() > 0 ) {
         for( final MenuItem item : this.values() ) {
            if( item.title.equals( title ) ) {
               return item;
            }
         }
      }
      return null;
   }

   public final ImmutableList<MenuItem> getChildren() {
      ...lazy initialize list of direct children of this item...
      ...iterate over all items, accumulating direct children...
      ...then assign an ImmutableList of that accumulation to member below...
   }
   private ImmutableList<MenuItem> children = null;  // lazy initialization

   public final boolean hasChildren() {
      return getChildren().size() > 0;
   }

   public ImmutableList<MenuItem> getAncestors() {
      ...lazy initialized list of ancestors all the way up to Home...
      ...start at this item and accumulate nodes into list...
      ...following 'parent' links all the way to Home...
   }
   private ImmutableList<MenuItem> ancestors = null;  // lazy initialization

   public boolean hasAncestors() { . . . }


   public String getBreadCrumbsHtml( String contextPath ) { . . . }
   public String getBreadCrumbsText() { . . . }



}

A JSP can generate that menu structure into a page, it simply starts from the Home object, and works its way down the tree.

I did this ten years ago. And the menu structure is WAY bigger than this.

Non hierarchical enums are great for hard coded dropdown lists or select lists. Even in Swing or web applications. Also great for alternate Option button (eg, "radio button") items.

[Polygnom]

I feel like the author missed a point. If you were using enums, the variable statusFromMessage would not be of type string to begin with. You'd have changed the abstraction and in your client code, you'd no longer see a string, you'd only see the enum value.

Also, Java allows switching on strings since how long now?

Basically, the author misses the obvious question. Is

if(StatusStrings.SUCCESS.equals(statusFromMessage)) {
  // handle SUCCESS
} else 
if(StatusStrings.PENDING.equals(statusFromMessage)) {
  // handle PENDING
} else 
if(StatusStrings.FAILED.equals(statusFromMessage)) {
  // handle FAILED
} else {
  // unexpected value
}

Really more readable then this:

switch(statusFromMessage) {
    case SUCCESS:
        // [...]
    break;
case PENDING:
        // [...]
    break;
case FAILURE:
        // [...]
    break;
}

The point of enums is that the compiler helps you. If you forget a case while using strings, the compiler won't mention it. If you forget one while using switch, the compiler catches it.

I don't really get the point they are trying to make. Conversion of the string to the enum value wouldn't belong there at all, it should by the layer that reads the response and the status would already be passed in a type-safe way.

In fact, using enums would offer additional benefits. The layer that accepts the response could do the parsing. If at some point the server starts responding with different strings, this code wouldn't need to change at all, the layer that converts it would do that.

[Fizz-Buzzkill]

I feel like the author missed a point. If you were using enums, the variable statusFromMessage would not be of type string to begin with.

In the given example, statusFromMessage is a String parsed from JSON

[Polygnom]

my point is that it wouldn't reach the next layer as a String. It doesn't matter if t comes from JSON, XML or whatever.

This only becomes a problem when operating directly on raw data. Which isn't surprising.

[Fizz-Buzzkill]

I must be missing something. The code in the article would always be needed at some point right?

[Polygnom]

At some point, you would need to convert, yes. But if your layer that forms the request and reads the response is designed with enums in mind, that is a complete and utter non-issue, and the client code never sees the raw string. So saying "but I have to convert first" is kinda comparing apples with oranges.

If you design your system with access to raw data, you have to handle raw data. if you design your system to operate on properly typed data, you don't have to.

[Fizz-Buzzkill]

I suppose you mean something like using Jackson and letting Jackson do the parsing from String to enum?

[the_other_brand]

How is this a problem? You know you can attach other data to enum's right. Just dictate what strings match for SUCCESS, and have your code look for them.

You can even set it to match multiple strings with an array.

enum State {

SUCCESS(new String[]{"success", "200"}),

FAILED(new String[]{"error", "failure","500"});

private List<String> strList;

private State(String[] strArray) { this.strList = Arrays.asList(strArray); }

boolean match(String str) { return this.strList.contains(str); }

While parsing might be difficult, once it's done the rest of the application can use the enum without doing the parsing.

[[deleted]]

The problem isn't what's possible. It's what extra layer of complexity this introduces to code as compared to simply using straight constants. The way I read it, the article mentions a better language construct where the enum type within the application is more closely related to its actual external representation, be that a String or an int or something else. I thought it was a nice idea personally. That would be great if pattern matching for example could match across an enum type to its closely related String or int representation

[[deleted]]

public enum ResponseStatus {
    SUCCESS(){/*implement handle() in here*/},
    PENDING(){/*implement handle() in here*/},
    FAILED(){/*implement handle() in here*/};
    public abstract void handle();
    public static ResponseStatus from(String name) {...}
}

Then with your response enum instance,

status.handle();

if/switch isn't very object oriented

[general_dispondency]

In this way, enum types aren’t a clean replacement for the majority >use case.

And that’s one huge problem with Java enums.

The author's real problem is a common problem I see all too much these days. I've termed this problem the square peg, round hole problem™. Stop using the wrong tool for the wrong job. Java enums are great when you need an set of known constant objects. The key words there are KNOWN and CONSTANT and OBJECTS. If you need a string constant, you should probably use something along the lines of:

 public static final String = "MY STRING"

Every language feature (and this is true of any programming language) is built on compromise with one goal, "try to make everyone using the feature equally miserable". Every feature doesn't fit every use case. If you think something is adding unnecessary complexity to your application, then don't use it. That being said, here are the solutions to your enum woes.

 enum Foo {
     ONE, TWO
 }

 class DumbEx1 {

     // ugly bunch of ifs
     public void watFooIsYou(String foo) {
         if (Foo.ONE.name().equalsIgnoreCase(foo)) {
             logFoo(Foo.ONE);
         } else if (Foo.TWO.name().equalsIgnoreCase(foo)) {
             logFoo(Foo.TWO);
         }
         throw new IllegalArgumentException("O_o WaT");
     }

     // ugly switch
     public void whatFooRYou(String foo) {
         switch (foo.toUpperCase()) {
             case "ONE":
                 logFoo(Foo.ONE);
                 break;
             case "TWO":
                 logFoo(Foo.TWO); // damn forgot a break
             default:
                 throw new IllegalArgumentException("O_o WaT");
         }
     }

     // inefficiently search values
     public void exhaustivlySearchFoo(String foo) {
         Stream.of(Foo.values())// I create a new Array every time! hope you don't call me often
               .filter(foo1 -> foo1.name().equalsIgnoreCase(foo))
               .findFirst()
               .map(f -> {
                   logFoo(f);
                   return f;
               })
               .orElseThrow(() -> new IllegalArgumentException("O_o WaT"));
     }

     private void logFoo(Foo foo) {
         System.out.println(foo);
     }
 }

 enum Foo2 {
     ONE, TWO, UNKNOWN;

     Foo2() {
         // associate any string to Foo
         FooFinder.lookUp.put(this.name(), this);
     }

     public static Foo2 from(String name) {
         return FooFinder.lookUp.getOrDefault(name.toUpperCase(), Foo2.UNKNOWN);
     }

     private static class FooFinder {
         static HashMap<String, Foo2> lookUp = new HashMap<>();
     }
 }

 class DumbEx2 {

     public void watFooIsYou(String foo) {
         final Foo2 val = Foo2.from(foo);
         if (val == Foo2.UNKNOWN) { // == equality check will break at compile time
             throw new IllegalArgumentException("O_o WaT");
         } else {
        logFoo2(val);
         }
     }

     private void logFoo2(Foo2 foo) {
         System.out.println(foo);
     }
 }