current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I have this enum below:

public enum TestEnum {
    h1, h2, h3, h4;

    public static String forCode(int code) {
    return (code >= 0 && code < values().length) ? values()[code].name() : null;
    }

    public static void main(String[] args) {
        System.out.println(TestEnum.h1.name());
        String ss = "h3";

        try {
           TestEnum.valueOf(ss); // but this validates with all the enum values
           System.out.println("valid");
        } catch (IllegalArgumentException e) {
           System.out.println("invalid");
        }           
    }
}

I need to check if the enum value represented by my string ss is one of my enum values h1, h2 or h4. So if h3 is being passed as a string, I would like to return false or throw IllegalArgumentException. I won't need to validate ss with h3 in the enum.

I came up with the below code to do this with the enum, but I believe there is a more elegant solution.

public enum TestEnum {
    h1, h2, h3, h4;

    public static boolean checkExcept(String el, TestEnum... except){
        boolean results = false;
        try{
            results = !Arrays.asList(except).contains(TestEnum.valueOf(el));
        }catch (Exception e){}
        return results;
    }

    public static String forCode(int code) {
        return (code >= 0 && code < values().length) ? values()[code].name() : null;
    }

    public static void main(String[] args) {

        String ss = "h3";

        if(TestEnum.checkExcept(ss,TestEnum.h1, TestEnum.h2, TestEnum.h3)){
           System.out.println("valid");
        }else{
           System.out.println("invalid");
        }
    }
}

Is there any better way of solving this problem?

share|improve this question

3 Answers 3

up vote 5 down vote accepted

I would pass a TestEnum instead of a String to your method, forcing the exception handling to be outside the method. I don't see why you are using Strings at all actually (although I suspect that this is only a small part of your code, and that you have your reasons for needing this).

I would do some refactoring and use EnumSet, which could make your code something like this: (untested code, but I believe it should work)

public static void main(String[] args) {
    String enumName = "h3";
    EnumSet<TestEnum> except = EnumSet.of(TestEnum.h1, TestEnum.h2, TestEnum.h3);

    boolean valid;
    try {
       valid = !except.contains(TestEnum.valueOf(enumName));
    } catch (IllegalArgumentException e) { valid = false; }
    System.out.println(valid ? "valid" : "invalid");
}

Note that I have gotten rid of your entire method here as I think that when you use EnumSet like this, you don't need the method. But of course you can put the relevant parts of the above code into a method.

share|improve this answer
    
What about if the String enumName is h10? It should return false as well? Correct? But it is throwing exception with your current code. –  david Feb 23 '14 at 21:00
    
@user2809564 As if that's a big problem to solve.... ;) See edit. –  Simon Forsberg♦ Feb 23 '14 at 21:12
up vote 1 down vote

Catching exception or its sub type is bad idea. This should do it more proper way as mentioned code below.

public static boolean contains(String test) {

    for (EnumType type : EnumType.values()) {
        if (type.name().equals(test)) {
            return true;
        }
    }

    return false;
}
share|improve this answer
    
This is generally a good option, but in this case it doesn't solve the same problem. The question asks for I need to check if the enum value represented by my string ss is one of my enum values h1, h2 or h4. So if h3 is being passed as a string, I would like to return false or throw IllegalArgumentException. –  Simon Forsberg♦ Mar 9 at 14:33
up vote 2 down vote

You can create an isValid or checkValid method inside the enum and override it in h3 if it's not changing runtime.

public enum TestEnum {
    h1, 
    h2, 
    h3 {
        @Override
        public boolean isValid() {
            return false;
        }
    },
    h4;

    public boolean isValid() {
        return true;
    }
}

It returns true for h1, h2 and h4, false for h3.

You could eliminate the results variable from checkExcept:

public static boolean checkExcept(String el, TestEnum... except) {
    try {
        return !Arrays.asList(except).contains(TestEnum.valueOf(el));
    } catch (Exception e) {
        return false;
    }
}

Please note that catching Exception is almost always a bad idea. I'd catch a more specific exception here (IllegalArgumentException).

share|improve this answer
    
Overriding the isValid method is not a flexible solution for different validation requirements. I agree with catching IllegalArgumentException though. –  Simon Forsberg♦ Feb 23 '14 at 21:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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