A Side-Affecting toString() is Evil

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....