9
votes
\$\begingroup\$

Is there a way to reduce this lengthy if condition:

if(
    statusCode.equals( NO_ERRORS_NO_WARNINGS ) ||
    statusCode.equals( HAS_ERRORS_NO_WARNINGS )  ||
    statusCode.equals( HAS_ERRORS_HAS_WARNINGS ) ||
    statusCode.equals( NO_ERRORS_HAS_WARNINGS )
){
    //do something
}

The purpose of it is to see if there is a status code set within a log file. So the String statusCode could be a null value, one of the 4 status codes above or an unpredicatable String.

\$\endgroup\$

closed as off-topic by Jamal Oct 24 '15 at 23:56

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

locked by Jamal Jan 11 '16 at 21:20

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • \$\begingroup\$ I'm not sure you could. You might convert to an enum to use a switch statement, but that doesn't buy you much. \$\endgroup\$ – Michael K Apr 23 '12 at 13:53
  • \$\begingroup\$ You mention that statusCode may be null...have you null checked before this statement? \$\endgroup\$ – Michael K Apr 23 '12 at 13:54
  • \$\begingroup\$ @MichaelK, yes the null is checked. I should have said empty really. \$\endgroup\$ – My Head Hurts Apr 23 '12 at 14:11
  • \$\begingroup\$ Why do you have HAS_ERRORS_HAS_WARNINGS in there twice? It looks like that statusCode holds teh state of two boolean flag ("errors" and "warning"). It may make sense to create to boolean fields (or atleast temporary variables) to hold the two boolean flags. \$\endgroup\$ – RoToRa Apr 25 '12 at 15:49
  • \$\begingroup\$ @RoToRa I would normally agree but the error code is a String taken from an external source. There is the potential for the String to be any array of characters - so the condition is to check whether the string we have matches any of the status codes we have. I am open to better ways of doing this if you have ideas? \$\endgroup\$ – My Head Hurts Apr 25 '12 at 17:53
17
votes
\$\begingroup\$

First of all, if statusCode can be null, the code above could possibly throw a null pointer exception.

To directly answer your question though, you could use a Set, such as a HashSet.

HashSet<String> errorOrWarning = new HashSet<String>();
errorOrWarning.add(HAS_ERRORS_HAS_WARNINGS);
errorOrWarning.add(HAS_ERRORS_NO_WARNINGS);
errorOrWarning.add(HAS_ERRORS_HAS_WARNINGS);
errorOrWarning.add(NO_ERRORS_HAS_WARNINGS);

Which you can then use in a single call.

if (errorOrWarning.contains(statusCode) {
    // do something
}

This has the advantage that statusCode can be null and it would produce no exception.

\$\endgroup\$
  • \$\begingroup\$ +1 thanks. It was a mistake to say that the status code could be null, I should have said empty. However I really like this answer as it does handle the NULL value nicely should it be there. \$\endgroup\$ – My Head Hurts Apr 23 '12 at 14:13
  • \$\begingroup\$ Very interesting solution! I'll have to add this to my bag o' tricks - I'm sure it will come in handy sometime. \$\endgroup\$ – Michael K Apr 23 '12 at 18:36
  • \$\begingroup\$ Yup, that will work. It's less readable, though, because now you've got a (probably static and probably distant in the code file) HashSet that you need to find all references to and analyze before you can know what the outcome of if will be. Of course, you could build the HashSet inside the method that needs it, but someone's gonna refactor it out as an invariant sooner or later. \$\endgroup\$ – Ross Patterson Apr 26 '12 at 11:01
  • \$\begingroup\$ @RossPatterson: You shouldn't need to see the contents of the HashSet if the name describes it properly. That's the whole point of the agile naming philosophy and the corresponding rename refactoring. \$\endgroup\$ – Donald.McLean Apr 26 '12 at 13:52
4
votes
\$\begingroup\$

I'm not sure about your class design, but I recommend refactoring this into some easier to use properties. I'm assuming we're within a class:

public boolean hasErrors() {
    return statusCode != null && (statusCode.equals(HAS_ERRORS_HAS_WARNINGS)
        || statusCode.equals(HAS_ERRORS_NO_WARNINGS));
}

public boolean hasWarnings() {
    return statusCode != null && (statusCode.equals(HAS_ERRORS_HAS_WARNINGS)
        || statusCode.equals(NO_ERRORS_HAS_WARNINGS));
}

That does not buy you much in withing those functions, but later on comes in handy:

if(hasErrors() || hasWarnings()) {
    // Do something
}
\$\endgroup\$
2
votes
\$\begingroup\$

If you have a large number of such conditions, it may be just easier to use arrays for the same, (as in the above comment)

for(T i : new T[]{HAS_ERRORS_HAS_WARNINGS, HAS_ERRORS_NO_WARNINGS, HAS_ERRORS_HAS_WARNINGS, NO_ERRORS_HAS_WARNINGS}) 
  if statusCode.equals(i) {
    // do
    break;
  }

Note - not as efficient as above, - O(n)

(you could also assign that to a variable and reuse the array if the same pattern is expected elsewhere.)

\$\endgroup\$
  • \$\begingroup\$ Note that it doesn't have to be an array. Any structure that can be iterated over in a for loop will work. Not at all a bad solution though. \$\endgroup\$ – Donald.McLean Apr 24 '12 at 18:05
  • \$\begingroup\$ I made it an array so that it can be initialized on the fly :) (Your solution is better in terms of perf. - O(1) vs O(n)) \$\endgroup\$ – rahul Apr 24 '12 at 18:07
1
vote
\$\begingroup\$

Use StringUtils.equalsAny:

if (StringUtils.equalsAny(statusCode, NO_ERRORS_NO_WARNINGS, 
        HAS_ERRORS_NO_WARNINGS, HAS_ERRORS_HAS_WARNINGS, 
        NO_ERRORS_HAS_WARNINGS)) {
    //do something
}

Unfortunately this method is not part any official release but the code is in the JIRA, you can copy it.

/**
 * Verifies if the tested string is equal with any of the provided strings.
 * This method is null safe and case sensitive.
 * 
 * @param str Tested string
 * @param any Strings to be tested against.
 * @return true if tested string is equal to any of the provided strings. false otherwise.
 */
public static boolean equalsWithAny(final String str, final String... any) {
    if (str == null) {
        return false;
    }

    if (any == null) {
        return false;
    }

    for (final String s: any) {
        if (str.equals(s)) {
            return true;
        }
    }

    return false;
}
\$\endgroup\$
  • \$\begingroup\$ +1 nice find - thanks. I wonder if this will ever be implemented or whether it has been put on the back burner for some reason. \$\endgroup\$ – My Head Hurts May 7 '12 at 6:40

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