Throw exception if value is not found

Question

I want to create ENUM which maps different statuses:

public enum BusinessCustomersStatus {
 A("active"),
 O("onboarding"),
 NV("not_verified"),
 
 private String status;
 BusinessCustomersStatus(String status)
 {
 this.status = status;
 }
 public String getStatus() {
 return status;
 }
 public static BusinessCustomersStatus getStatusByText(String statusText) {
 BusinessCustomersStatus response = null;
 for (BusinessCustomersStatus status : values()) {
 if (status.getStatus().equalsIgnoreCase(statusText)) {
 response = status;
 break;
 }
 }
 if(response == null)
 {
 throw new UnsupportedOperationException(String.format("Unknown status: '%s'", statusText));
 }
 return response;
 }
}

I want to improve the Java method getStatusByText. The code is working but I want to improve the logic for throwing exception.

Answer 1

Few suggestions:

• Naming: A, O, and NV are not clear names. Would be better to use their extended names: ACTIVE, ONBOARDING, and NOT_VERIFIED.
• Missing semicolon: NV("not_verified"), should be NV("not_verified");.
• return instead of break as @dariosicily suggested.

Alternative using Streams:

public static BusinessCustomersStatus getStatusByText(String text) {
 return Stream.of(BusinessCustomersStatus.values())
 .filter(bcs -> bcs.getStatus().equalsIgnoreCase(text))
 .findAny()
 .orElseThrow(() -> new UnsupportedOperationException(String.format("Unknown status: '%s'", text)));
}

• Simon Forsberg: Personally I prefer adding the semicolon on the line after the last entry, it makes the git history cleaner
• Simon Forsberg: UnsupportedOperationException is unnecessary, IllegalArgumentException is clearer. For other points, see tucuxi's answer, which I think has the best approach.
• Simon Forsberg: If the edge-case of an invalid text is something that can actually happen and needs to be handled properly, then I would use an Optional<BusinessCustomersStatus> instead.

Answer 2

About the for in your getStatusByText method :

BusinessCustomersStatus response = null;
for (BusinessCustomersStatus status : values()) {
 if (status.getStatus().equalsIgnoreCase(statusText)) {
 response = status;
 break;
 }
}
if(response == null) {
 throw new UnsupportedOperationException(String.format("Unknown status: '%s'", statusText));
}
return response;

You can return directly the status as your response instead of breaking the for loop and after return the response. This implies the rewritting of your method like below :

public static BusinessCustomersStatus getStatusByText(String statusText) {
 for (BusinessCustomersStatus status : values()) {
 if (status.getStatus().equalsIgnoreCase(statusText)) {
 return status;
 }
 }
 throw new UnsupportedOperationException(String.format("Unknown status: '%s'", statusText));
}

The final part of your method changes too ending directly with the exception in case no value in your for loop satisfies your condition.

Answer 3

Your code can be simpler if you take advantage of valueOf() (see javadoc):

public static BusinessCustomersStatus getStatusByText(String text) {
 return Enum.valueOf(
 BusinessCustomersStatus.class, 
 text.toUpperCase());
}

Note that this throws an IllegalArgumentException if the text does not correspond to a BusinessCustomersStatus value - which actually looks better to me than an UnsupportedOperationException, which is better used when the operation you are attempting (string-to-enum-value) is optional and, in this specific class, not implemented for any argument at all -- and not just "unsupported" for the argument you have just entered.

• dariosicily: Probably the best approach.

Answer 4

The previous answers by dariosicily and marc cover most of the ground. But I have a few other comments.

• If you go for more meaningful names for your enum values, you probably don't need a "status" field - you can simply map the name to lower case.
• I'm not sure UnsupportedOperationException is the best choice here, and can see no good reason not to create your own Exception for the purpose.
• If your list of enum values grows significantly bigger, I'd suggest maintaining a Map of the values rather than doing a sequential search. My example below does this, just to illustrate how I'd do that, though for three values it's probably overkill.

package codeReview;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import codeReview.Penzov.BusinessCustomersStatus.NoSuchStatusException;
public class Penzov {
 public enum BusinessCustomersStatus {
 ACTIVE, //
 ONBOARDING, //
 NOT_VERIFIED;
 static Map <String,BusinessCustomersStatus> statusLookup = new HashMap<>();
 static {
 for (BusinessCustomersStatus status: values()) {
 statusLookup.put(status.getStatus(), status);
 }
 }
 public String getStatus() {
 return name().toLowerCase();
 }
 public static BusinessCustomersStatus getStatusByText(String statusText) throws NoSuchStatusException {
 BusinessCustomersStatus status = statusLookup.get(statusText.toLowerCase());
 if (status != null) {
 return status;
 }
 // Didn't find a match
 throw new NoSuchStatusException(String.format("Unknown status: '%s'", statusText));
 }
 public static class NoSuchStatusException extends Exception {
 private static final long serialVersionUID = -2003653625428537073L;
 public NoSuchStatusException(String message) {
 super(message);
 }
 }
 }
 public static void main(String[] args) {
 for (String testString: Arrays.asList("active", "ONBOARDING", "NoT_VERified", "dubious")) {
 try {
 System.out.format("testString = '%s', status found = '%s'%n", testString, BusinessCustomersStatus.getStatusByText(testString));
 } catch (NoSuchStatusException e) {
 System.out.format("testString = '%s', exception thrown = '%s'%n", testString, String.valueOf(e));
 }
 }
 }
}

• Mark Bluemel: Thanks to Marc for the edit.
• J. Dimeo: I usually use IllegalArgumentException in this context since someone was trying to parse a string to the enum (but the argument to that parsing was illegal)
• Mark Bluemel: Note that having seen tucuxi's post, I'd use his approach.