A Side-Affecting toString() is Evil

1 comments
The principle of command-query-separation states that a method should either return a computed result or change the state of the receiver object. It should not do both. The rationale is two fold:
  • A method that does only one things is more coherent and thus easier to write/read/debug
  • The overall structure of a class that follows that principle tends to be simpler (i.e.: better).

Although there is a public consensus as to the value of this principle, there a few exceptions. For example, in java.util.List the remove(int) method removes an element from the underlying collection and returns that element. This design clearly violates command-query-separation but for a good reason: it enables implementing methods to be thread-safe. Anyway, there's one place where this principle should be followed blindly: the toString() method.

I had the misfortune to come across side-affecting toString() methods a few times. Every time my flow was interrupted due to unexpected side effects that popped up out of no where. It always took me a lot of time to find why the object's state changes.

This bad-enough problem is gravitated by two factors:
First, some toString() calls are implicit, as in:
   "x=" + x;

If x is not a String then the expression above has an implicit method call, like this:
   "x=" + x.toString();


Second, toString() calls are performed by the debugger when it shows the contents of variables. Hence, in the presence of a side-affecting toString(), your program will behave differently in debug-time. Moreover, this change in behavior is not consistent: it depends on which variables you decide to inspect. A programmer's living hell.

Real-life Example #1:

In Sun's Java compiler, ClassType.toString() invokes the typarams() method which in-turn invokes the side-affecting complete() method. I realized this only when I worked on the following fragment:

ClassType t = ....
if(t.interfaces_field.size() > 0) {
... // Do something
}

I got a null error since t.interfaces_field was null. When I opened the debugger the problem vanished: the debugger invoked t.toString() which started the chain reaction of completing the object and assigning values to its fields.

Real-life Example #2:

public static String toCsv(String[] line) {
String result = "";
final Separator comma = new Separator(",");
for (String s : line)
result += comma + s;

return result;
}


Looking at this method you'd expect it to return a comma-separated string with a comma before the very first item. After all, what else could "result += comma + s;" mean? Examining the Separator class reveals a counter-intuitive implementation:

public class Separator {
private final String s;
boolean first = true;

public Separator(String s) { this.s = s; }

@Override public String toString() {
if (!first)
return s;
first = false;
return "";
}
}


Of course, the final keyword attached to the comma variable just adds to the confusion. For sake of credibility, let me say that this is an exact copy of real code from a real project. I didn't invent it to make my point....