2
\$\begingroup\$

I wrote a static method for an enum that returns the corresponding enum instance based on the source string, trimming any whitespaces and ignoring case as:

/**
 * Convert the passed {@code source} string value to respective {@link SomeEnum} instance.
 * Ignore any white-spaces present in the beginning/end of the passed {@code source} string.
 * Ignore casing of the {@code source} string.
 *
 * @param source {@link String}
 * @return converted {@link SomeUnit} instance if conversion is possible, null otherwise.
 */
public static SomeEnum fromString(String source) {
    try {
        return source == null ? null : SomeEnum.valueOf(source.trim().toUpperCase());
    } catch (IllegalArgumentException e) {
        return null;
    }
}

Then considering the fact that there is only one source for a NPE here, I changed the above to:

/**
 * Convert the passed {@code source} string value to respective {@link SomeEnum} instance.
 * Ignore any white-spaces present in the beginning/end of the passed {@code source} string.
 * Ignore casing of the {@code source} string.
 *
 * @param source {@link String}
 * @return converted {@link SomeUnit} instance if conversion is possible, null otherwise.
 */
public static SomeEnum fromString(String source) {
    try {
        return SomeEnum.valueOf(source.trim().toUpperCase());
    } catch (NullPointerException | IllegalArgumentException e) {
        return null;
    }
}

Question:

Is there any difference (significant/insignificant) between the two in terms of efficiency, best practices, coding style, readability or any other possible factor that I am unaware of? Or am I just overthinking?

I personally feel that instead of relying on exceptions (NPE here) to take some action, better to test it beforehand if possible, which favors the first approach. But then I feel that in this particular case I am anyway having a catch block here for IAE so better to club the exceptions together instead of performing a null check.

Update: This code is a piece of already written application, and I have to return null in case there is no match, so as to respect the various client calls.

Edit: This code is fully owned by me. When I stated its a piece of already written application, I meant, way earlier I wrote the code as first approach(where again returning null was needed), and just a few days back going through this I refactored it to second approach.

Also this is no pseudo code, this is the exact code being used with just the class and variables name changed.

\$\endgroup\$

closed as off-topic by 200_success Sep 22 '17 at 5:08

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.

  • \$\begingroup\$ I think both is OK and I did catch-NPE instead of null checks as well. However you need to be careful that this must be a very unlikely case as the catch is pretty slow. And you have to be absolute sure that any place inside the try which encountered a NPE should be covered, otherwise you might hide corrupted state. \$\endgroup\$ – eckes Sep 22 '17 at 5:14
  • 1
    \$\begingroup\$ added javadocs as asked in one of the answers \$\endgroup\$ – Vikas Prasad Sep 22 '17 at 5:38
  • 1
    \$\begingroup\$ Why this is on hold? This is a real code that I own/maintain, with only the variables and enum name changed. There is no pseudocode or stub code here?? \$\endgroup\$ – Vikas Prasad Sep 22 '17 at 5:40
  • \$\begingroup\$ Welcome to Code Review! You'll receive better reviews if you show a complete example; for example, it would be better if you include the definition of SomeEnum and also a main() that shows how your code is used. During your edit, you should also change the question title: the site standard is to simply state the task accomplished by the code. See How to get the best value out of Code Review: Asking Questions for guidance on writing good titles. \$\endgroup\$ – Toby Speight Sep 22 '17 at 10:05
2
\$\begingroup\$

The correct behavior of your code depends on the related code around it.

If that code doesn't expect nullvalues in strings, you should not even check for null. And, to be consistent, you should not return null either but throw an exception instead. Therefore, you should remove the whole catch clause.

On the other hand, if you follow the Apache Commons style of expecting null everywhere and passing it through unmodified, your first code is the way to go. Since null is an expected value, it should not throw exceptions, for performance reasons. The second code is nicer to read, but it will be slower unless the optimizer were really fancy and would transform the second code into the first.

\$\endgroup\$

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