Is catching null pointer exception to handle it instead of null check beforehand always a bad practice or it depends? [closed]

Question

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.

Answer 1

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.