Code symmetry

Today, Peter Kofler and I have been pairing over the Internet to do deliberate practice. To improve our coding skills. We have been working on the word wrap kata, to practise the Transformation Priority Premise, this time using JavaScript. We've checked-in the code after every little change so the evolution can be seen in the "commits" in the repo.

What I have learned in this session:

  • I will always use curly braces in "if" statements even when they only contain a "return" statement: I used to think that, even without the braces,  developers would notice the "return" and thus avoid the mistake of adding lines afterwards. But Peter suggested that some developers might add lines before the "return" statement. This is something I didn't think of before!
  • Code symmetry helps to explain the intention of the code.

Code symmetry started to grab my attention when I saw Kent Beck in his TDD screencasts. In the second episode he prefers to leave some duplication in favor of symmetry.

Peter and I were working on a simple function to find out the position where we had to wrap the line. We had a green test with this implementation:

  1. function wrapPosition(paragraph, maxLineLen){
  2. if (paragraph.indexOf(" ") > -1){
  3. return paragraph.indexOf(" ");
  4. } else {
  5. return maxLineLen;
  6. }
  7. }

Then I noticed that the "else" was not necessary so I removed it:

  1. function wrapPosition(paragraph, maxLineLen){
  2. if (paragraph.indexOf(" ") > -1){
  3. return paragraph.indexOf(" ");
  4. }
  5. return maxLineLen;
  6. }

The Peter said, "now the 'if' statement looks like a guard clause, but in reality we don't have such a guard clause".
This is true, the reader may wonder why is that guard clause in there. Actually we know that any of
the two paths is possible, so the "else" expresses the intent of the code better.

But we still thought that the "else" was not necessary, so Peter came up with the idea of inverting the condition:

  1. var lineBreak = "\n";
  2. var notFound = -1;
  3. function wrapPosition(paragraph, maxLineLen){
  4. var hasNoBlank = paragraph.indexOf(" ") == notFound;
  5. if (hasNoBlank){
  6. return maxLineLen;
  7. }
  8. return paragraph.indexOf(" ");
  9. }

Now it does not look that much like a guard clause. What do you think?

Some related talks and posts that Peter has sent to me, related to our discussion today:

It's been a very interesting discussion, I want to say thank you to Peter for arranging this session. There will be more soon 🙂

 

 

Enjoyed reading this post?
Subscribe to the RSS feed and have all new posts delivered straight to you.
  • Cristian

    I prefer the second option, almost all javascript developpers understands that this if isn’t a guard clause. The third option is more complicated to read.

  • http://rchavarria.github.io/ R. Chavarría T.

    The first option looks beautiful, it has symmetry, but I agree with you on the ‘else’ clause is not necessary.
    I think I would end up with a mix of the two last solutions:


    var notFound = -1;
    function wrapPosition(paragraph, maxLineLen) {
    var hasNoBlank = paragraph.indexOf(" ") == notFound;
    return hasNoBlank ?
    maxLineLen :
    paragraph.indexOf(" ");
    }

    Or even:


    function wrapPosition(paragraph, maxLineLen) {
    var blankPosition = paragraph.indexOf(" "),
    blankNotFound = (blankPosition == -1);

    return blankNotFound ? maxLineLen : blankPosition;
    }

  • http://www.carlosble.com/ Carlos Ble

    I don’t like the ternary operator at all mate 🙂

  • http://www.carlosble.com/ Carlos Ble

    What’s the difference in this snippet, between JavaScript and any other common language? 🙂

  • Cristian

    mande?
    xD

  • http://rchavarria.github.io/ R. Chavarría T.

    What a pity! I love it, but I have to admit that an if-else statement is more readable. I love it because it’s shorter, lazy me. 😛

  • http://www.carlosble.com/ Carlos Ble

    Then if you acknowledge it reads better using if-else, go for it, use it. Remember that clean code is about the reader, not you. (it could be you in the future ;-))

  • http://www.carlosble.com/ Carlos Ble

    jeje, que por que dices que un programador javascript se da cuenta? es que uno de java no?

  • Cristian

    Ah, jeje. Me refiero a que esa construcción es muy común en javascript.

  • http://www.jjcoellov.es Juanjo Coello


    var NOT_FOUND = -1;
    function wrapPosition(paragraph, maxLineLen) {
    var blankPosition = paragraph.indexOf(" ");
    return blankPosition == NOT_FOUND ? maxLineLen : blankPosition
    }

    Don’t like to transverse twice 🙂

  • http://www.carlosble.com/ Carlos Ble

    oh man, I just hate the ternary operator 😉

  • http://www.jjcoellov.es Juanjo Coello

    I like them when the operands are straightforward statements (one variable, one simple expression). They usually got out of hands when they become more complex!

  • http://www.carlosble.com/ Carlos Ble

    Complex expressions come out of simple expressions 🙂

  • http://www.code-cop.org/ Peter Kofler

    Carlos, also thank you for taking time to pair with me. I loved our discussions. In fact it does not matter which one of these three concrete versions of the method is the best, as they are all very short and easy to understand, but what matters it that we thought about it, discussed it, tried different things and considered readability. This is the essence of deliberate practice.

    I personally like the first version most, because as you say “the else expresses the intent of the code better”, at least for me. It is interesting to read also the comments of your readers and see what is better for them.

    In some way version 3 could be considered a guard clause. In real scenarios the usual case will be that there is a space somewhere in the line because normal language does not contain words that are 60 or 80 characters long. So the case that there is no space, is a special case that does not need logic. Unfortunately, even with your help, I could not make it clear in the code because we need the “indexOf” before the first if, thus fuzzing out the potential guard clause nature. It looks we have to continue on this one, because I am not satisfied with it yet 😉