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.
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:14SomeEnum
and also amain()
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