Writing a Better Code Narrative

Writing code is like writing a book in a lot of ways. Like a book, code is read far more often than it is written. Therefore, like a book, code should be written in a way that gives it a clear narrative. By having a clear narrative, one can follow the code and understand it without too much head scratching. There is a lot of code out there that doesn't have a clear narrative. It meanders and backtracks, leading to confusion. One of the biggest contributors to confusing structure is the complex if statement. if's and else's take the story back and forth, and all over the place.

There are many tools in good design to fight the confusing tendencies of complex if's. One that changed the way I write my code significantly is the guard clause. Rather than wrapping a whole bunch of code in a big if, we reverse the condition and return early. Rather than worrying about the condition the whole way down, we can conceptually just eliminate that case as early as possible. Guard clauses also (ideally) don't have else branches, meaning one less idea we have to hold in our heads at once.

Bad code lives out there despite our best efforts. I wrote a sample of bad code for this post (see the full repo):

package net.cbojar.blog.codenarrative;

import java.util.List;

public class CommandParser
{
  /**
   * Valid commands:
   *   n: No op (do nothing)
   *   w : Write (append) character
   *   p : Prepend character
   *   r  : Append character  times
   *   s: Append space character
   */
  public String parseCommands(List commands) {
    StringBuilder result = new StringBuilder();

    for(String command : commands) {
      command = command.trim();
      if(!command.isEmpty()) {
        String[] commandParts = command.split(" ");
        if(!commandParts[0].equals("n")) {
          if(commandParts[0].equals("p")) {
            result.insert(0, commandParts[1]);
          } else if(commandParts[0].equals("r")) {
            int times = Integer.parseInt(commandParts[1], 10);
            for(int i = 0; i < times; i++) {
              result.append(commandParts[2]);
            }
          } else if(commandParts[0].equals("s")) {
            result.append(" ");
          } else if(commandParts[0].equals("w")) {
            result.append(commandParts[1]);
          } else {
            throw new IllegalArgumentException("Invalid command");
          }
        }
      }
    }

    return result.toString();
  }
}

This code parses a list of strings for commands to build a result string. It's not the worst code ever written, but it's not good either. It's ugly, with a lot of conditional logic that is a bit twisty. The narrative gets lost quickly. Based on the fact that it is the top-most conditional, I can only assume the most important thing this code does is determine if the command is empty, and then if it's not empty, do some other stuff. I have to read all the way down to know that it doesn't do anything if command is empty. Similarly, once I know command is not empty, the next most important thing is the no-op command, denoted with n. Inside there are the least important commands, and under some condition, an exception. The narrative of this code distorts the importance of the parts. The inner conditionals are the most important part of this code, not the outer conditionals. The exception is also important, and should be less buried. At least one guard clause can be pulled out of this code. The outer check for an empty command is essentially housekeeping, skipping any blank commands (a possibility but not a probability if, say, the commands were being entered via terminal input. The nesting of the rest of the commands within the n command check artificially inflates the importance of that command as well. It is no more likely than any other command to be given. It can be pulled into the conditional inside of it. That whole conditional could also be converted to a case statement to express the simplicity and symmetry of the conditions. That gives us a simpler narrative:

package net.cbojar.blog.codenarrative;

import java.util.List;

public class CommandParser
{
  /**
   * Valid commands:
   *   n: No op (do nothing)
   *   w : Write (append) character
   *   p : Prepend character
   *   r  : Append character  times
   *   s: Append space character
   */
  public String parseCommands(List commands) {
    StringBuilder result = new StringBuilder();

    for(String command : commands) {
      command = command.trim();
      if(command.isEmpty()) {
        continue;
      }

      String[] commandParts = command.split(" ");
      switch(commandParts[0]) {
        case "n":
          break;
        case "p":
          result.insert(0, commandParts[1]);
          break;
        case "r":
          int times = Integer.parseInt(commandParts[1], 10);
          for(int i = 0; i < times; i++) {
            result.append(commandParts[2]);
          }
          break;
        case "s":
          result.append(" ");
          break;
        case "w":
          result.append(commandParts[1]);
          break;
        default:
          throw new IllegalArgumentException("Invalid command");
      }
    }

    return result.toString();
  }
}

This narrative is clearer, but not as clear as it could be. It contains a lot of details that we might not be worried about, and that might change. In here we see both the mappings from strings to algorithms, and the algorithms themselves. Object-oriented design gives us some powerful tools to help us separate and encapsulate these kinds of details. We can wrap the behavior into objects, first, to hide the implementation details of the algorithms:

package net.cbojar.blog.codenarrative;

import java.util.List;

public class CommandParser
{
  /**
   * Valid commands:
   *   n: No op (do nothing)
   *   w : Write (append) character
   *   p : Prepend character
   *   r  : Append character  times
   *   s: Append space character
   */
  public String parseCommands(List commands) {
    StringBuilder result = new StringBuilder();

    for(String command : commands) {
      command = command.trim();
      if(command.isEmpty()) {
        continue;
      }

      String[] commandParts = command.split(" ");
      Command c;
      switch(commandParts[0]) {
        case "n":
          c = new NoOpCommand();
          c.apply(result);
          break;
        case "p":
          c = new PrependCommand(commandParts[1]);
          c.apply(result);
          break;
        case "r":
          int times = Integer.parseInt(commandParts[1], 10);
          c = new RepeatCommand(commandParts[2], times);
          c.apply(result);
          break;
        case "s":
          c = new WriteCommand(" ");
          c.apply(result);
          break;
        case "w":
          c = new WriteCommand(commandParts[1]);
          c.apply(result);
          break;
        default:
          throw new IllegalArgumentException("Invalid command");
      }
    }

    return result.toString();
  }
}

Then, we can hide away the algorithm to connect them into a factory:

package net.cbojar.blog.codenarrative;

public class CommandFactory
{
  public Command getCommand(String command) {
    if(command.isEmpty()) {
      return new NoOpCommand();
    }

    String[] commandParts = command.split(" ");
    switch(commandParts[0]) {
      case "n":
        return new NoOpCommand();
      case "p":
        return new PrependCommand(commandParts[1]);
      case "r":
        int times = Integer.parseInt(commandParts[1], 10);
        return new RepeatCommand(commandParts[2], times);
      case "s":
        return new WriteCommand(" ");
      case "w":
        return new WriteCommand(commandParts[1]);
      default:
        throw new IllegalArgumentException("Invalid command");
    }
  }
}

Giving us:

package net.cbojar.blog.codenarrative;

import java.util.List;

public class CommandParser
{
  /**
   * Valid commands:
   *   n: No op (do nothing)
   *   w : Write (append) character
   *   p : Prepend character
   *   r  : Append character  times
   *   s: Append space character
   */
  public String parseCommands(List commands) {
    StringBuilder result = new StringBuilder();

    for(String command : commands) {
      command = command.trim();

      Command c = new CommandFactory().getCommand(command);
      c.apply(result);
    }

    return result.toString();
  }
}

We don't worry about how the commands are created, or even what they are. The narrative is simple: We get a list of string commands, loop over them, trim them, convert them to a command object, and apply the command to the result. If we want to know how commands are hooked together, we can look into the factory. If we want to know what each command does, that behavior is now isolated into each command class. We can change behaviors without worrying about affecting other code. We can add behavior by creating a new class and adding it to the factory, and doing so does not require parsing mind-bending conditional logic. Is this better? The parseCommands method is certainly shorter and more direct, but there are now more places to go to understand how it all works. We can no longer just look at the method (ignoring the documentation above it) and see what commands are available. Those commands are now spread out into their own classes. We also cannot see how the commands connect with the command objects. That's hidden in the factory. But this is better. We have less to reason about at any given time. If there is an error with a particular behavior, there is one, isolated place to go deal with that, and it can be dealt with without affecting any other commands. If there is an issue with the way commands are mapped to command objects, that's all within the factory. We don't need to worry about the layers above or below, just the level of abstraction we need to deal with.

This code is not perfect, but it shows the importance and power of writing a better narrative in our code. When code is clearer, bugs and confusion have less places to hide.