Encapsulating logging and throwing exceptions into a method. Bad practice?

Question

original code example

if (var1 == null) {
 String msg = "var 1 is null";
 logger.log(msg);
 throw new CustomException(msg);
}
if (var2 == null) {
 String msg = "var 2 is null";
 logger.log(msg);
 throw new CustomException(msg);
}
if (var3 == null) {
 String msg = "var 3 is null";
 logger.log(msg);
 throw new CustomException(msg);
}
return var3.toString(); // ok

new code

if (var1 == null) {
 logMessageAndThrowException("var 1 is null");
}
if (var2 == null) {
 logMessageAndThrowException("var 2 is null");
}
if (var3 == null) {
 logMessageAndThrowException("var 3 is null");
}
return var3.toString(); // NetBeans warn me here for a NPE
void logMessageAndThrowException(String msg) throws CustomException {
 logger.log(msg);
 throw new CustomException(msg);
}

Is this a bad practice or NetBeans just are not able to understand the code?

Answer 1

The second version removes some duplication, so it's better, but usually you should choose one of logging and rethrowing. Frameworks/appservers usually catch and log uncatched exceptions so you will have two (or more) entries for the same exception it the log file which makes debugging hard.

Log the exception only once!

One of the common pain points that I have noticed is logging and rethrowing an exception. As a result, the log files contains the same exceptions several times on several stack levels.

Author: Nayaki

• user219882: youre right... the best thing is probably only throw the exception and let it be logged somewhere else
• András Hummer: +1 for palacsint. First, in the spirit of "separation of concerns", have a separate entity for logging and another separate one for exception throwing. Also, the logging and message throwing invocation could be invoked from inside a loop with the argument's number as parameter.

Answer 2

if (var3 == null) {
 logMessageAndThrowException("var 3 is null");
}
return var3.toString(); // NetBeans warn me here for a NPE

Netbeans probably does not look into the method. Netbeans does not know from the name, what a human will read. So Netbeans sees an if statement, no direct return or throw and assumes that it could happen to execute return var3.toString();

In general for arguments checking:

• I would avoid custom exceptions, use runtime exceptions
• I would only check arguments if they are used as a state variable for later usage. If not, it is completely fine to get a direct NullPointer exception. Because the results are the same: I will get a notice about the variable unexpectedly being null at the moment.
• I would use a wrapper/proxy method like:

#

public static <T> T notNull(final T value) {
 if (value == null) //you could add more logic here, like logging
 throw new IllegalArgumentException("Value is null");
 return value;
}
public static <T> T notNull(final T value, final String message) {
 if (value == null)
 throw new IllegalArgumentException("Value is null, " + message);
 return value;
}

to write such code:

this.variable = notNull(variable);