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).

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.

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...