Examining Command-Query-Separation

I have already written at least once on the command-query-separation (CQS) principle. Today, I would like to show a class which benefits from NOT following this principle.

So, here is a snippet from a class that parses and evaluates arithmetic expressions.

public class Solver1 {
   private String input;

   public Solver1(String input_) {
      input = input_.trim();
   }

   private boolean consumeIf(String s) {
      if (!input.startsWith(s))
         return false;
      input = input.substring(1).trim();
      return true;
   }

   public double eval() {
      double lhs = multiplication();
      if (consumeIf("+"))
         return lhs + eval();
      else if (consumeIf("-"))
         return lhs - eval();
      return lhs;
   }

   private double multiplication() {
      double lhs = unary();
      if (consumeIf("*"))
         return lhs * multiplication();
      else if (consumeIf("/"))
         return lhs / multiplication();
      return lhs;
   }
   ...
}

Looking at consumeIf() we note that it violates the CQS principle. This method is a query (it computes and returns a result - either true of false) which is also a command (it changes the value of a field). Let's see the looks of the code after decomposing consumeIf() into two methods: a query and a command:

public class Solver2 {
   private String input;

   public Solver2(String input_) {
      input = input_.trim();
   }

   private boolean nextIs(String s) {
      return input.startsWith(s);
   }
   
   private void move(int numChars) {
      input = input.substring(numChars).trim();
   }
   
   public double eval() {
      double lhs = multiplication();
      if (nextIs("+")) {
         move(1);
         return lhs + eval();
      }
      else if (nextIs("-")) {
         move(1);
         return lhs - eval();
      }
      return lhs;
   }

   private double multiplication() {
      double lhs = unary();
      if (nextIs("*")) {
         move(1);
         return lhs * multiplication();
      }
      else if (nextIs("/")) {
         move(1);
         return lhs / multiplication();
      }
      return lhs;
   }
   ...
}

There is a 25% increase in code size, 42 vs. 33 lines of code, but I will put this aside because I think that #LOC is not a very good estimate of code quality. (Actually, I don't believe in any code quality metric)

Nonetheless there is a another issue which is much more significant: in the second program we have this recurring idiom of calling nextIs() and then calling move(), as in: if (nextIs("/")) { move(1); ... }. These two calls are mutually dependent: the parameter passed to move() must be the length of the string passed to nextIs(). This idiom is error-prone as the programmer may accidentally change one of the parameters while leaving the other intact.

This shows that arbitrarily applying the CQS principle may yield bad results. A good programmer should be aware of that and should make judgment calls rather than blindly following principles. In fact, this happens a lot with many other programming principles. They are usually much closer to guidelines (and therefore require judgment) than to absolute rules. I think that Lars summarized it pretty well: "I generally dislike '-isms' because they set up rules that people try to apply without understanding the preconditions that need to be met."