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

2 comments :: Examining Command-Query-Separation

  1. Hi

    I find using CQS to be more useful to public methods than to private ones.
    Your eval method does voilate CQS and i think it is more problematic than asking yourself if nextIf should be divided into 2 functions...

    If you decide to use nextIs and move you could minimize a risk of programming errors by changing move method to take a string param (the one you would pass to nextIs) instead of int.
    One thing that hurts me more when thinking of CQS is fact that i would like to return success/faiulure from my commands. The simplest solution is to throw exception in command if exception occured but exceptions tend to clutter code too much.

  2. I have read your blog its very attractive and impressive. I like it your blog.

    Java Training in Chennai Java Training in Chennai | Core Java Training in Chennai

    Online Java Training Online Java Training | Java J2EE Online Training | JavaEE Training Institute in Chennai m

Post a Comment