Verifying value before parsing

Question

I am writing some Java code and am coming across a try/catch block in which I decided to verify the value is not null before parsing it:

String parsedValue = DEFAULT_VALUE;
try {
 if (myValue != null) {
 parsedValue = parse(myValue);
 }
} catch (ParseException e) {
 log(e);
}

Regarding the code above, my question is, is it better to nest the if loop into the try/catch block or vice-versa?

In other words, is the previous code better or the following:

String parsedValue = DEFAULT_VALUE;
if (myValue != null) {
 try {
 parsedValue = parse(myValue);
 } catch (ParseException e) {
 log(e);
 }
}

EDIT: As @Brythan raised the issue, for simplicity, let's assume here that log is a method in a random logging library, and has the 2 signatures log(Exception) and log(String). Like that we avoid creating an Exception to log a message, it makes the code clearer.

As @h.j.k raised another issue, here myValue could be null, this is no exceptional case, but if myValue has been assigned a value (not null), it should be parseable, and only in that case I expect an Exception to be thrown by parse(String).

• Blackbelt: why don't you move the check in parse ?
• Vince: @Blackbelt why not, as long as at the end of the try block, if myValue is null, parsedValue will have the value DEFAULT_VALUE.

Answer 1

I would put the try/catch over as little code as possible. This also makes it easier to add an else case to the if statement if necessary. For example:

String parsedValue = DEFAULT_VALUE;
if (myValue != null) {
 try {
 parsedValue = parse(myValue);
 } catch (ParseException e) {
 log(e);
 }
} else {
 log(new Exception("Empty myValue!"));
}

This allows for more flexibility than the other version.

As a general rule, it's best to put as little code inside a try block as possible. Only things that can throw an exception that should be caught by the catch and connecting code. One reason is that other code than you originally intended may throw the exception and the solution in the catch block may not apply to the new case.

• Vince: Same comment as the other answer: throwing exceptions is costly, so throwing an exception if myValue is null is not very nice.
• Brythan: @Vince But I didn't throw an exception if myValue is null. I logged a message. For whatever reason, you made log take an exception as an argument. So I generated one without throwing. And really, that's beside the point. It doesn't matter what you put in the else block. The point is that this way you can easily add else code outside the try. With the other version, you have to rewrite the code in order to do that.
• Vince: my bad, I didn't know that the costly part is the throw. Also the log method is for clarity purpose (everyone has its own favourite logging system). I'll update the question to make that clearer but log(String) would look clearer to me.

Answer 2

Short (and hopefully sweet) advice:

Are you able to modify parse() to throw a ParseException (I assuming you're talking about the linked one) if its argument is null? Something like:

String parse(Object input) {
 if (input == null) {
 throw new ParseException("null argument", -1);
 }
 ...
}

It then becomes the duty of parse() to validate its argument, and you can also eliminate the null check on the caller's side in this context.

I propose tweaking it slightly further (if you have no wish or intention to accidentally reassign parsedValue later):

final String parsedValue;
try {
 parsedValue = parse(myValue);
} catch (ParseException e) {
 log(e);
 parsedValue = DEFAULT_VALUE;
}

edit:

If you think that artificially throwing a ParseException for a null argument is fishy and you happen to be on >= Java 7, I will suggest this:

String parse(Object input) {
 Objects.requireNonNull(input);
 ...
}

and then

final String parsedValue;
try {
 parsedValue = parse(myValue);
} catch (ParseException | NullPointerException e) {
 log(e);
 parsedValue = DEFAULT_VALUE;
}

edit 2:

I have a line above that reads:

It then becomes the duty of parse() to validate its argument, and you can also eliminate the null check on the caller's side in this context.

The first part is well-understood I hope, the "in this context" in the second part refers to whether to call parse() depending if myValue is null. Should you have additional logic in the current code block that depends on that condition, then yeah I guess it is also ok to be doing the explicit null check before calling parse(). However, you may then want to consider how to better handle multiple null checks...

• Vince: Yeah but I rank the fact that an Exception is costly higher than the look of a try block and an if loop.
• h.j.k.: @Vince two points: 1) How widely used is parse() then? If it is used widely, then having to do a null check before every call may start to develop into a form of code smell. In fact, the simplest solution I can think of compared to what I put in my edit section is to simply call a method on input, and let the JVM throw the NPE by default when it really is null. Just remember to catch it on the caller side. This then becomes a trade-off between an implicit understanding and the explicit check using Objects.requireNonNull.
• h.j.k.: @Vince 2) A try-catch block incurs no cost so long as it doesn't throw an Exception, e.g. see here or here (which happens to be closely related to your question). My advice simply begins with moving your if check on the caller side into the called method, and as long as myValue is not null, I believe it will be no costlier than what you have proposed in your question.
• Vince: I understand your point, but I am worried in the case myValue is null. I am processing a list of values I parse, so if they are all null I may lose a noticeable fraction of second, and this is enough not to choose to throw the Exception (even if it is null). The Exception, I learned, is for exceptional cases, when you have no idea why it would do that. While having a null value can be common - and typically when no value has been assigned. But in the case null is not expected at all, I agree that would be a good solution.