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
add comment

2 Answers

up vote 4 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. –  user2809564 Feb 23 at 21:00
    
@user2809564 As if that's a big problem to solve.... ;) See edit. –  Simon André Forsberg Feb 23 at 21:12
add comment
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 André Forsberg Feb 23 at 21:14
add comment

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.