3
\$\begingroup\$

I have Grade enum and there is a static method to convert a string value to the particular enum value.

public enum Grade {
    None("N"),
    NotAccepted("-"),
    Accepted("+"),
    Uncertain("?");

    private final String gradeSymbol;

    Grade(String gradeSymbol) {
        this.gradeSymbol = gradeSymbol;
    }

    public String getGradeSymbol() {
        return gradeSymbol;
    }

    public static Grade fromGradeSymbol(final String gradeSymbol) {
        switch (gradeSymbol) {
            case "N":
                return None;

            case "-":
                return NotAccepted;

            case "+":
                return Accepted;

            case "?":
                return Uncertain;

            default:
                throw new IllegalArgumentException("Unexpected grade symbol: " + gradeSymbol);
        }
    }

    @Override
    public String toString() {
        return this.gradeSymbol;
    }
}

My question is, what is better way to handle unexpected grade symbol in the static fromGradeSymbol() method.

Another option I consider is the approach using Optional and returning Optional.empty() instead of throwing IllegalArgumentException.

Like this:

public static Optional<Grade> fromGradeSymbol(final String gradeSymbol) {
    switch (gradeSymbol) {
        case "N":
            return Optional.of(None);

        case "-":
            return Optional.of(NotAccepted);

        case "+":
            return Optional.of(Accepted);

        case "?":
            return Optional.of(Uncertain);

        default:
            return Optional.empty();
    }
}

My thoughts:

Throwing exception looks more logical, because it is being thrown in not expected case, when wrong value has been passed. But throwing exception makes it necessary to catch this exception anywhere where I convert string to the enum.

Use of Optional does not involve exceptions and makes cleaner code, without try-catch construct. But it somehow hides the unexpected input.

Sure, I can assume that every time when Optional does not contain a value we have an unexpected input. But it feels like code smell.

What do you think?

\$\endgroup\$
  • \$\begingroup\$ You should at least be looping over Grade.values(). If you need to call fromGradeSymbol frequently, then you should build a static Map<String, Grade> and use that. \$\endgroup\$ – RoToRa Jan 9 at 11:03
  • \$\begingroup\$ Thank you. What do you think about the main question regarding using Optional and throwing IllegalArgumentException? Do you share my thoughts or have different opinion ? \$\endgroup\$ – Rafael Jan 9 at 11:13
4
\$\begingroup\$

All enumerations have a default valueOf(String) method that throw an IllegalArgumentException so your first idea is coherent.

I would say that it depends (..). It depends of your use cases:

  • If he conversion would be called form an unsafe (user/api) input, then there are chances that you receive an unexpected symbol. And using Optional will force the developer to handle that case more explicitely.
  • If the convesrion is made from a safe input (code/config), then you can live with the exception.

In the book Effective Java from Joshua Bloch (Addison-Wesley Professional, 978-0134685991) there is a full chapter on that subject (in the 2nd edition):

Enum types have an automatically generated valueOf(String) method that translates a constant’s name into the constant itself.

[..]

The following code (with the type name changed appropriately) will do the trick for any enum, so long as each constant has a unique string representation:

// Implementing a fromString method on an enum type
private static final Map<String, Operation> stringToEnum
        = new HashMap<String, Operation>();
static { // Initialize map from constant name to enum constant
    for (Operation op : values())
        stringToEnum.put(op.toString(), op);
    }

// Returns Operation for string, or null if string is invalid
public static Operation fromString(String symbol) {
    return stringToEnum.get(symbol);
}

That chapter (and the whole book) is worth the read: CHAPTER 6 ENUMS AND ANNOTATIONS

\$\endgroup\$
  • \$\begingroup\$ Thank you. Conversion would be called from unsafe input (received text files from outside). So, using Optional is better in this case ? \$\endgroup\$ – Rafael Jan 9 at 11:16
  • \$\begingroup\$ As said in my answer, and in my opinion, yes. Except if you want to "fail" the whole process for an unknown symbol. \$\endgroup\$ – gervais.b Jan 9 at 11:24
  • \$\begingroup\$ Understood. Thank you. \$\endgroup\$ – Rafael Jan 9 at 11:26
  • \$\begingroup\$ Do not use Optional in this case at all. See my answer. \$\endgroup\$ – TorbenPutkonen Jan 9 at 16:49
1
\$\begingroup\$

Single Responsibility

My rule of thumb is that no extra methods or data should be added to an enum. If input has to be converted to an enum (or enum to output), a separate Function is created to handle the transformation. This way the enum does not get piggybacked with any additional responsibilities. The enum should be used for computation only.

Once you do that your problem becomes simple, as the responsibility of the unknown grade symbol is no longer responsibility of the enum. It becomes the responsibility of the component that should be responsible for incorrect input: the input handler. Now the correct action can be chosen from the specification that was made for the input handler. The Operation enum no longer has to worry about situations where it doesn't exist. It always exists.

This follows the single responsibility principle.

Thus:

public class SymbolToOperationConverter implements Function<String, Operation> {

    // Copy gervais.b's init routine.
    private static final Map<String, Operation> CONVERSION_MAP = ...;

    public Operation apply(String symbol) {
        Operation op = CONVERSION_MAP.get(symbol);
        if (op != null) {
            return op;
        } else {
            // My spec chooses to throw an excepotion.
            throw new InvalidInputException(...);
        }
    }
}

Useless Use Of Optional

Optionals are to Java programmers what cat is to Unix admins. More often than not they are completely unnecessary. Optionals were intended to be a way to communicate the possibility of null values in public interfaces in a code level (as opposed to documentation or annotations). The fromGradeSymbol method is completely internal to your application so you should know that it can return nulls and have unit tests for those use cases (that is, if you choose to not follow my advice above). There is literally nothing evil with returning a null value or testing for null with an if statement. The evil is not knowing if a third party library you use might return null or not. Optional was created to fight that evil.

\$\endgroup\$
  • \$\begingroup\$ "for computation only"? I think you meant "for grouping the possible choices and storing one of them only". \$\endgroup\$ – Roland Illig Jan 9 at 17:14
  • 1
    \$\begingroup\$ I disagree about the uselessness of optionals and being only for third party interfaces. Being internal is irrelevant if you're a client of your own code. They're semantically useful for any reader including yourself, and in my opinion are more readable than annotations. Either way, you could have had a comparable question of returning null versus throwing an exception. Both implementations have valid use cases, so pick the one that's actually more useful to you. If both are necessary, you can implement both and differentiate by name. \$\endgroup\$ – butt Jan 9 at 19:32
  • 1
    \$\begingroup\$ You can disagree as much as you like, but the post I linked was written by one of the Java architects who were responsible for designing them. I didn't actually invent the claim out of thin air. :) Anyway, being a client of your own code is relevant because you know the code. You're really supposed to know what you are doing when you use your own code. \$\endgroup\$ – TorbenPutkonen Jan 9 at 20:02
  • 1
    \$\begingroup\$ Using an optional requires three method calls, one object allocation and one if statement at minimum. A null check requires one if statement. You're not supposed to just whip out code for fun when you actually know you don't need an optional. Especially when the option is not a least bit worse. People like to use optionals because they have the incorrect ifea that null checks are bad. Again what is bad is not knowing if null is possible. And with your own code you always know. \$\endgroup\$ – TorbenPutkonen Jan 9 at 20:10
  • 1
    \$\begingroup\$ This is an interesting, and strongly opinionated, point of view. The linked article from Brian Goetz Does not say that "you should not use Optional" but more "do not abuse". I disagree when @TorbenPutkonen say that we are supposed to "know what you are doing", I know it for today but not in 3 months and my colleagues does not know. Again, if the probability of receiving an unknown symbol is not exceptional, then don't use an exception or, at least, provide a test method to avoid using exception to control the flow. \$\endgroup\$ – gervais.b Jan 10 at 7:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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