Menu
I have a domain model class which has a toString implementation that looks like this:
public String toString() {
try {
return getX() + "\n"
getY() + "\n"
getZ(); //etc.
} catch(Exception e) {
throw new RuntimeException(e);
}
}
The methods getX(), getY() and getZ() are not simple getters, they can perform lookups in the background, generally a lookup to a static map of predefined key-value pairs. Some of them had throws SomeCheckedException in their signatures.
My impression is that this is bad practice and a "code smell". The fact that toString() even needs this check is to me a symptom of bad design. But I'm asked by a colleague, what exactly is wrong with catching the generic Exception in a toString(), since the caught Exception is propagated further.
I believe it violates at least the KISS principle, since a simple method like toString() here is indicated as requiring special exception handling.
So is it code smell to have a catch-all block in a toString()?
Answers I found were either for the general scenario of catching generic Exception and I agree with most of them, that if you're doing a generic error handling mechanism or a batch then it's expected to work on generic exceptions. This argument was unconvincing in our discussion, so I'm curious of other opinions.
527
So in general, catching generic exceptions is bad unless you are 100% sure that you know exactly which kinds of exceptions will be thrown and under which circumstances. If in doubt, let them bubble up to the top level exception handler instead. A similar rule here is never throw exceptions of type System. Exception.
As you can see in the following code snippet, even if you know which exceptions the method might throw, you can't simply catch them. You need to catch the generic Exception class and then check the type of its cause. This code is not only cumbersome to implement, but it's also hard to read.
It is an essential concept never catch any exception so catch any exception only if you can handle it you can give additional contextual data in that exception. If you can't handle it in the catch block, then the best advice is don't catch it only to re-throw it.
Because when you catch exception you're supposed to handle it properly. And you cannot expect to handle all kind of exceptions in your code. Also when you catch all exceptions, you may get an exception that cannot deal with and prevent code that is upper in the stack to handle it properly.
For the toString() method, catching Exception is not necessarily a bad practice. However, re-throwing it is the problematic part.
The contract for toString() is:
... In general, the toString method returns a string that "textually represents" this object. The result should be a concise but informative representation that is easy for a person to read...
In Effective Java 3rd Edition (Item 12), Bloch further insists:
When practical, the toString method should return all of the interesting information contained in the object.
So, if this requires calling methods that may throw checked exceptions, then so be it, and it makes a lot of sense to catch these exceptions.
However: raised checked exceptions provide information about the state of the object. Consistently with the goal of toString, it may be a good idea to include the exceptional condition in the message returned by toString.
As for why it's a bad idea to throw exceptions from toString, this post provides a great answer.
Recommendation: Catch the checked exceptions using their specific exception type, and integrate this fact in the toString() message, instead of propagating it.
153
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With
