debugjavaMinor
Use of Exception to log execution stack trace
Viewed 0 times
stackexceptionlogusetraceexecution
Problem
I've been back and forth with a colleague over the use of
But it is being used thusly:
When reviewing this, my comments included things like the "Exceptions are exceptional" idiom.
But when I mentioned that creating a stack trace is an expensive operation and should be avoided, I was told that this used to be true in the past [e.g. with JVM implementations] but is now longer true [i.e. This has been fixed/optimized].
I was not aware of this development, so I checked around and most of the SO questions seem to indicate that making a stack trace is expensive.
Does this code seem reasonable? Would you consider this bad practice?
Throwable.fillInStackTrace. This Log class is meant to wrap the Simple Logging Facade for Java.public class Log {
private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(Log.class);
private String stackTrace(Exception cause) {
if (cause == null)
return "";
StringWriter sw = new StringWriter(1024);
final PrintWriter pw = new PrintWriter(sw);
cause.printStackTrace(pw);
pw.flush();
return sw.toString();
}
public void warn(Exception cause, String message) {
String output = message + "\n" + stackTrace(cause);
log.warn(output);
}
// debug(...), info(...), etc.
}But it is being used thusly:
new Log()
.warn((Exception)new RuntimeException().fillInStackTrace(), "I'm warning you.");When reviewing this, my comments included things like the "Exceptions are exceptional" idiom.
But when I mentioned that creating a stack trace is an expensive operation and should be avoided, I was told that this used to be true in the past [e.g. with JVM implementations] but is now longer true [i.e. This has been fixed/optimized].
I was not aware of this development, so I checked around and most of the SO questions seem to indicate that making a stack trace is expensive.
Does this code seem reasonable? Would you consider this bad practice?
Solution
It seems like you're working harder than you need to. Instead of this:
I think you could do this, and no need for the
Moreover, notice how common libraries use a
public void warn(Exception cause, String message) {
String output = message + "\n" + stackTrace(cause);
log.warn(output);
}I think you could do this, and no need for the
stackTrace method:public void warn(Exception cause, String message) {
log.error(cause.getMessage() + " " + message, cause);
}Moreover, notice how common libraries use a
String as the first parameter of logging methods, and a Throwable as the second. I suggest to follow that example and reorder your parameters.Code Snippets
public void warn(Exception cause, String message) {
String output = message + "\n" + stackTrace(cause);
log.warn(output);
}public void warn(Exception cause, String message) {
log.error(cause.getMessage() + " " + message, cause);
}Context
StackExchange Code Review Q#68233, answer score: 8
Revisions (0)
No revisions yet.