8

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.

SDJ
  • 4,083
  • 1
  • 17
  • 35
generickycdeveloper
  • 83
  • 4
  • 2
    I'd consider it somewhat bad practice to do more that simple getter calls for `toString()` actually. If you require some kind of lookup, you should describe the lookup rather than perform it. – daniu Jun 21 '19 at 16:51

4 Answers4

4

Yes this is bad practice.

The intention of the toString method is to provide a programmer readable representation of your class. You shouldn't include any method calls in this method, including getters.

In fact, I would consider not autogenerating these methods smelly, but assuming you are not comfortable or able to use an IDE that would produce them for you, I would recommend including a reference to all the fields on the object, and the class name of the object, as is done by the intellij toString method

Maus
  • 1,791
  • 1
  • 17
  • 28
  • 2
    There is no reason to use a StringBuilder in place of a one-line concatenation. It makes the code harder to read while providing no benefit. – VGR Jun 21 '19 at 20:21
  • and yet some people like and use it. :shrug: – Maus Jun 21 '19 at 20:23
  • 1
    huh: I'll edit my answer to remove that line: https://stackoverflow.com/questions/4645020/when-to-use-stringbuilder-in-java – Maus Jun 21 '19 at 20:24
4

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.

SDJ
  • 4,083
  • 1
  • 17
  • 35
  • A very helpful answer. Allowing catching general exceptions, but only if unchecked ones are re-thrown, while checked ones are logged, was something the team agreed on being the best approach. – generickycdeveloper Jul 29 '19 at 09:37
3

The main reason not to catch generic Exception at any place is that it will include RuntimeExceptions too, which should not be catched on normal circumstances, because they always represent a bug in the program. It's better to let them be propagated and arise, so that the developer can notice it and eventually fix it.

I don't know if any additional good practice checks shall be applied in the case of toString methods, but I'm sure at least the general rule should be applied.

So, the best practice is always to catch only checked exceptions, and then either recover, either rethrow them, or either rewrap them into another exception (which is your case).

Little Santi
  • 8,563
  • 2
  • 18
  • 46
0

Should the "normal flow" be interrupted by a failing toString() method? If the answer is no, you should make the toString() method "work". Catching an exception an reflecting this in the result is one possibility, or a simple log output.

KoW
  • 784
  • 5
  • 12