Chapter 3. Write Simple Units of Code

Each problem has smaller problems inside.

Martin Fowler

Complexity is an often disputed quality characteristic. Code that appears complex to an outsider or novice developer can appear straightforward to a developer that is intimately familiar with it. To a certain extent, what is “complex” is in the eye of the beholder. There is, however, a point where code becomes so complex that modifying it becomes extremely risky and very time-consuming task, let alone testing the modifications afterward. To keep code maintainable, we must put a limit on complexity. Another reason to measure complexity is knowing the minimum number of tests we need to be sufficiently certain that the system acts predictably. Before we can define such a code complexity limit, we must be able to measure complexity.

A common way to objectively assess complexity is to count the number of possible paths through a piece of code. The idea is that the more paths can be distinguished, the more complex a piece of code is. We can determine the number of paths unambiguously by counting the number of branch points. A branch point is a statement where execution can take more than one direction depending on a condition. Examples of branch points in Java code are if and switch statements (a complete list follows later). Branch points can be counted for a complete codebase, a class, a package, or a unit. The number of branch points of a unit is equal to the minimum number of paths needed to cover all branches created by all branch points of that unit. This is called branch coverage. However, when you consider all paths through a unit from the first line of the unit to a final statement, combinatory effects are possible. The reason is that it may matter whether a branch follows another in a particular order. All possible combinations of branches are the execution paths of the unit—that is, the maximum number of paths through the unit.

Consider a unit containing two consecutive if statements. Figure 3-1 depicts the control flow of the unit and shows the difference between branch coverage and execution path coverage.

Figure 3-1. Branch coverage and execution path coverage

Suppose the point to the left of the first if statement modifies a database, and the point to the right of the second if statement reads from that database. These are side effects and require us to test the “zigzag” paths as well (the dotted lines in Figure 3-1).

In summary, the number of branch points is the number of paths that cover all branches created by branch points. It is the minimum number of paths and can be zero (for a unit that has no branch points). The number of execution paths is a maximum, and can be very large due to combinatorial explosion. Which one to choose?

The answer is to take the number of branch points plus one. This is called cyclomatic complexity or McCabe complexity. Consequently, the guideline “limit the number of branch points per unit to 4” is equal to “limit code McCabe complexity to 5.” This is the minimum number of test cases that you need to cover a unit such that every path has a part not covered by the other paths. The cyclomatic (McCabe) complexity of a unit is at least one, which is easy to understand as follows. Consider a unit with no branch points. According to the definition, its cyclomatic complexity is one (number of branch points plus one). It also fits intuitively: a unit with no branch points has one execution path, and needs at least one test.

For the sake of completeness: only for units with one exit point, the cyclomatic or McCabe complexity is equal to the number of branch points plus one. It becomes more complex for units with more than one exit point. Do not worry about that: focus on limiting the number of branch points to four.

Now consider the following example. Given a nationality, the getFlagColors method returns the correct flag colors:

public List<Color> getFlagColors(Nationality nationality) {
    List<Color> result;
    switch (nationality) {
    case DUTCH:
        result = Arrays.asList(Color.RED, Color.WHITE, Color.BLUE);
        break;
    case GERMAN:
        result = Arrays.asList(Color.BLACK, Color.RED, Color.YELLOW);
        break;
    case BELGIAN:
        result = Arrays.asList(Color.BLACK, Color.YELLOW, Color.RED);
        break;
    case FRENCH:
        result = Arrays.asList(Color.BLUE, Color.WHITE, Color.RED);
        break;
    case ITALIAN:
        result = Arrays.asList(Color.GREEN, Color.WHITE, Color.RED);
        break;
    case UNCLASSIFIED:
    default:
        result = Arrays.asList(Color.GRAY);
        break;
    }
    return result;
}

The switch statement in the method body needs to handle all cases of the nationality enumeration type and return the correct flag colors. As there are five possible nationalities and the unclassified/default case, the number of isolated paths to be tested (control flow branches) is six.

On first sight, the getFlagColors method might seem harmless. Indeed, the method is quite readable, and its behavior is as expected. Still, if we want to test the behavior of this method, we would need six unique test cases (one for each nationality plus one for the default/unclassified case). Writing automated tests might seem excessive for the getFlagColors method, but suppose a developer adds the flag of Luxembourg (which is very similar to the Dutch flag) as a quick fix:

...
case DUTCH:
    result = Arrays.asList(Color.RED, Color.WHITE, Color.BLUE);
case LUXEMBOURGER:
    result = Arrays.asList(Color.RED, Color.WHITE, Color.LIGHT_BLUE);
    break;
case GERMAN:
....

Being in a hurry, the developer copied the constructor call for the Dutch flag and updated the last argument to the right color. Unfortunately, the break statement escaped the developer’s attention, and now all Dutch nationalities will see the flag from Luxembourg on their profile page!

This example looks like a forged scenario, but we know from our consultancy practice that this is what happens to complex code in reality. These types of simple mistakes are also responsible for many “trivial” bugs that could be easily prevented.

To understand why complex code is such a problem for maintenance, it is important to realize that code that starts out quite straightforward tends to grow much more complex over time. Consider the following snippet taken from the codebase of the open source build server Jenkins. There are 20 control flow branches in this code snippet. Imagine having to modify or test this method:

/**
 * Retrieve a user by its ID, and create a new one if requested.
 * @return
 *      An existing or created user. May be {@code null} if
 *      a user does not exist and {@code create} is false.
 */
private static User getOrCreate(String id, String fullName,
    boolean create) {
    String idkey = idStrategy().keyFor(id);

    byNameLock.readLock().lock();
    User u;
    try {
        u = byName.get(idkey);
    } finally {
        byNameLock.readLock().unlock();
    }
    final File configFile = getConfigFileFor(id);
    if (!configFile.isFile() && !configFile.getParentFile().isDirectory()) {
        // check for legacy users and migrate if safe to do so.
        File[] legacy = getLegacyConfigFilesFor(id);
        if (legacy != null && legacy.length > 0) {
            for (File legacyUserDir : legacy) {
                final XmlFile legacyXml = new XmlFile(XSTREAM,
                    new File(legacyUserDir, "config.xml"));
                try {
                    Object o = legacyXml.read();
                    if (o instanceof User) {
                        if (idStrategy().equals(id, legacyUserDir.getName())
                            && !idStrategy()
                                .filenameOf(legacyUserDir.getName())
                                .equals(legacyUserDir.getName())) {
                            if (!legacyUserDir
                                .renameTo(configFile.getParentFile())) {
                                LOGGER.log(Level.WARNING,
                                    "Failed to migrate user record " +
                                    "from {0} to {1}",
                                    new Object[] {legacyUserDir,
                                        configFile.getParentFile()});
                            }
                            break;
                        }
                    } else {
                        LOGGER.log(FINE,
                            "Unexpected object loaded from {0}: {1}",
                            new Object[] {legacyUserDir, o});
                    }
                } catch (IOException e) {
                    LOGGER.log(Level.FINE,
                        String.format(
                            "Exception trying to load user from {0}: {1}",
                            new Object[] {legacyUserDir, e.getMessage()}),
                        e);
                }
            }
        }
    }
    if (u == null && (create || configFile.exists())) {
        User tmp = new User(id, fullName);
        User prev;
        byNameLock.readLock().lock();
        try {
            prev = byName.putIfAbsent(idkey, u = tmp);
        } finally {
            byNameLock.readLock().unlock();
        }
        if (prev != null) {
            u = prev; // if someone has already put a value in the map, use it
            if (LOGGER.isLoggable(Level.FINE)
                && !fullName.equals(prev.getFullName())) {
                LOGGER.log(Level.FINE,
                    "mismatch on fullName (‘" + fullName + "’ vs. ‘"
                        + prev.getFullName() + "’) for ‘" + id + "’",
                    new Throwable());
            }
        } else
            if (!id.equals(fullName) && !configFile.exists()) {
            // JENKINS-16332: since the fullName may not be recoverable
            // from the id, and various code may store the id only, we
            // must save the fullName
            try {
                u.save();
            } catch (IOException x) {
                LOGGER.log(Level.WARNING, null, x);
            }
        }
    }
    return u;
}

private static Map<Nationality, List<Color>> FLAGS =
    new HashMap<Nationality, List<Color>>();

static {
    FLAGS.put(DUTCH, Arrays.asList(Color.RED, Color.WHITE, Color.BLUE));
    FLAGS.put(GERMAN, Arrays.asList(Color.BLACK, Color.RED, Color.YELLOW));
    FLAGS.put(BELGIAN, Arrays.asList(Color.BLACK, Color.YELLOW, Color.RED));
    FLAGS.put(FRENCH, Arrays.asList(Color.BLUE, Color.WHITE, Color.RED));
    FLAGS.put(ITALIAN, Arrays.asList(Color.GREEN, Color.WHITE, Color.RED));
}

public List<Color> getFlagColors(Nationality nationality) {
    List<Color> colors = FLAGS.get(nationality);
    return colors != null ? colors : Arrays.asList(Color.GRAY);
}

public interface Flag {
    List<Color> getColors();
}

public class DutchFlag implements Flag {
    public List<Color> getColors() {
        return Arrays.asList(Color.RED, Color.WHITE, Color.BLUE);
    }
}

public class ItalianFlag implements Flag {
    public List<Color> getColors() {
        return Arrays.asList(Color.GREEN, Color.WHITE, Color.RED);
    }
}

private static final Map<Nationality, Flag> FLAGS =
    new HashMap<Nationality, Flag>();

static {
    FLAGS.put(DUTCH, new DutchFlag());
    FLAGS.put(GERMAN, new GermanFlag());
    FLAGS.put(BELGIAN, new BelgianFlag());
    FLAGS.put(FRENCH, new FrenchFlag());
    FLAGS.put(ITALIAN, new ItalianFlag());
}

public List<Color> getFlagColors(Nationality nationality) {
    Flag flag = FLAGS.get(nationality);
    flag = flag != null ? flag : new DefaultFlag();
    return flag.getColors();
}

public static int calculateDepth(BinaryTreeNode<Integer> t, int n) {
    int depth = 0;
    if (t.getValue() == n) {
        return depth;
    } else {
        if (n < t.getValue()) {
            BinaryTreeNode<Integer> left = t.getLeft();
            if (left == null) {
                throw new TreeException("Value not found in tree!");
            } else {
                return 1 + calculateDepth(left, n);
            }
        } else {
            BinaryTreeNode<Integer> right = t.getRight();
            if (right == null) {
                throw new TreeException("Value not found in tree!");
            } else {
                return 1 + calculateDepth(right, n);
            }
        }
    }
}

public static int calculateDepth(BinaryTreeNode<Integer> t, int n) {
    int depth = 0;
    if (t.getValue() == n)
        return depth;
    if (n < t.getValue() && t.getLeft() != null)
        return 1 + calculateDepth(t.getLeft(), n);
    if (n > t.getValue() && t.getRight() != null)
        return 1 + calculateDepth(t.getRight(), n);
    throw new TreeException("Value not found in tree!");
}

public static int calculateDepth(BinaryTreeNode<Integer> t, int n) {
    int depth = 0;
    if (t.getValue() == n)
        return depth;
    else
        return traverseByValue(t, n);
}

private static int traverseByValue(BinaryTreeNode<Integer> t, int n) {
    BinaryTreeNode<Integer> childNode = getChildNode(t, n);
    if (childNode == null) {
        throw new TreeException("Value not found in tree!");
    } else {
        return 1 + calculateDepth(childNode, n);
    }
}

private static BinaryTreeNode<Integer> getChildNode(
    BinaryTreeNode<Integer> t, int n) {
    if (n < t.getValue()) {
        return t.getLeft();
    } else {
        return t.getRight();
    }
}