Rule 1: Leave No-Unit Level Code Smells Behind

At this point in the book you are familiar with nine guidelines for building maintainable software, discussed in the previous nine chapters. Of those nine guidelines, three deal with smells at the unit level: long units (Chapter 2), complex units (Chapter 3), and units with large interfaces (Chapter 5). For modern programming languages, there is really no good reason why any of these guidelines should be violated when you are writing new code.

To follow this rule is to refactor “smelly” code in time. By “in time,” we mean as soon as possible but certainly before the code is committed to the version control system. Of course, it is OK to have small violations when you are working on a development ticket—for example, a method of 20 lines of code or a method with 5 parameters. But these violations should be refactored out before you commit your changes.

Of course, the other guidelines, such as avoiding duplicated code and preventing tight coupling, are equally important to building a maintainable system. However, as a responsible software developer, you will find the first three guidelines are easy to integrate with your daily way of working. Violations of unit length, complexity, and parameters are easy to detect. It is very common to have these checks available in modern integrated development environments. We actually advise you to turn on this feature and make sure your code is free from unit-level code smells before each commit.

Rule 2: Leave No Bad Comments Behind

Comments are sometimes considered the anti-pattern of good code. From our experience we can confirm that inline comments typically indicate a lack of elegant engineering solutions. Consider the following method taken from the Jenkins codebase:

public HttpResponse doUploadPlugin(StaplerRequest req)
    throws IOException, ServletException {
    try {
        Jenkins.getInstance().checkPermission(UPLOAD_PLUGINS);

        ServletFileUpload upload = new ServletFileUpload(
            new DiskFileItemFactory());

        // Parse the request
        FileItem fileItem = (FileItem)upload.parseRequest(req).get(0);
        String fileName = Util.getFileName(fileItem.getName());
        if ("".equals(fileName)) {
            return new HttpRedirect("advanced");
        }
        // we allow the upload of the new jpi's and the legacy hpi's
        if (!fileName.endsWith(".jpi") && !fileName.endsWith(".hpi")) {
            throw new Failure("Not a plugin: " + fileName);
        }

        // first copy into a temporary file name
        File t = File.createTempFile("uploaded", ".jpi");
        t.deleteOnExit();
        fileItem.write(t);
        fileItem.delete();

        final String baseName = identifyPluginShortName(t);

        pluginUploaded = true;

        // Now create a dummy plugin that we can dynamically load
        // (the InstallationJob will force a restart if one is needed):
        JSONObject cfg = new JSONObject().element("name", baseName)
            .element("version", "0"). // unused but mandatory
            element("url", t.toURI().toString())
            .element("dependencies", new JSONArray());
        new UpdateSite(UpdateCenter.ID_UPLOAD, null).new Plugin(
            UpdateCenter.ID_UPLOAD, cfg).deploy(true);
        return new HttpRedirect("../updateCenter");
    } catch (IOException e) {
        throw e;
    } catch (Exception e) {// grrr. fileItem.write throws this
        throw new ServletException(e);
    }
}

Although the doUploadPlugin is not very hard to maintain (it has only 1 parameter, 31 lines of code, and a McCabe index of 6), the inline comments indicate separate concerns that could easily be addressed outside this method. For example, copying the fileItem to a temporary file and creating the plugin configuration are tasks that deserve their own methods (where they can be tested and potentially reused).

Comments in code may reveal many different problems:

• Lack of understanding of the code itself

  // I don't know what is happening here, but if I remove this line
  // an infinite loop occurs

• Issue tracking systems not properly used

  // JIRA-1234: Fixes a bug when summing negative numbers

• Conventions or tooling are being bypassed

  // CHECKSTYLE:OFF
  // NOPMD

• Good intentions

  // TODO: Make this method a lot faster some day

Comments are valuable in only a small number of cases. Helpful API documentation can be such a case, but always be cautious to avoid dogmatic boilerplate commentary. In general, the best advice we can give is to keep your code free of comments.

Rule 3: Leave No Code in Comments Behind

Although there might be rare occasions where there is a good reason to use comments in your code, there is never an excuse for checking in code that is commented out. The version control system will always keep a record of old code, so it is perfectly safe to delete it. Take a look at the following example, taken from the Apache Tomcat codebase:

private void validateFilterMap(FilterMap filterMap) {
    // Validate the proposed filter mapping
    String filterName = filterMap.getFilterName();
    String[] servletNames = filterMap.getServletNames();
    String[] urlPatterns = filterMap.getURLPatterns();
    if (findFilterDef(filterName) == null)
        throw new IllegalArgumentException(
            sm.getString("standardContext.filterMap.name", filterName));

    if (!filterMap.getMatchAllServletNames() &&
        !filterMap.getMatchAllUrlPatterns() &&
        (servletNames.length == 0) && (urlPatterns.length == 0))
        throw new IllegalArgumentException(
            sm.getString("standardContext.filterMap.either"));
    // FIXME: Older spec revisions may still check this
    /*
    if ((servletNames.length != 0) && (urlPatterns.length != 0))
        throw new IllegalArgumentException
            (sm.getString("standardContext.filterMap.either"));
    */
    for (int i = 0; i < urlPatterns.length; i++) {
        if (!validateURLPattern(urlPatterns[i])) {
            throw new IllegalArgumentException(
                sm.getString("standardContext.filterMap.pattern",
                    urlPatterns[i]));
        }
    }
}

The FIXME note and accompanying code are understandable from the original developer’s perspective, but to a new developer they act as a distractor. The original developer had to make a decision before leaving this commented-out code: either fix it at the spot, create a new ticket to fix it at some other time, or reject this corner case altogether.

Rule 4: Leave No Dead Code Behind

Dead code comes in different shapes. Dead code is code that is not executed at all or its output is “dead”: the code may be executed, but its output is not used elsewhere in the system. Code in comments, as discussed in the previous section, is an example of dead code, but there are many other forms of dead code. In this section, we give three more examples of dead code.

public Transaction getTransaction(long uid) {
    Transaction result = new Transaction(uid);
    if (result != null) {
        return result;
    } else {
        return lookupTransaction(uid); 
    }
}

Rule 5: Leave No Long Identifiers Behind

Good identifiers make all the difference between code that is a pleasure to read and code that is hard to wrap your head around. A famous saying by Phil Karlton is “There are only two hard problems in computer science: cache invalidation and naming things.” In this book we won’t discuss the first, but we do want to say a few things about long identifiers.

Identifiers name the items in your codebase, from units to modules to components to even the system itself. It is important to choose good names so that developers can find their way through the codebase without great effort. The names of most of the identifiers in a codebase will be dependent on the domain in which the system operates. It is typical for teams to have a formal naming convention or an informal, but consistent, use of domain-specific terminology.

It is not easy to choose the right identifiers in your code, and unfortunately there are no guidelines for what is and what isn’t a good identifier. Sometimes it may even take you a couple of iterations to find the right name for a method or class.

As a general rule, long identifiers must be avoided. A maximum length for an identifier is hard to define (some domains have longer terminology than others), but in most cases there is little debate within a development team when an identifier is considered too long. Identifiers that express multiple responsibilities (such as generateConsoleAnnotationScriptAndStylesheet) or contain too many technical terms (e.g., GlobalProjectNamingStrategyConfiguration) are always a violation of this rule.

Rule 6: Leave No Magic Constants Behind

Magic constants are number or literal values that are used in code without a clear definition of their meaning (hence the name magic constant). Consider the following code example:

float calculateFare(Customer c, long distance) {
    float travelledDistanceFare = distance * 0.10f;
    if (c.getAge() < 12) {
        travelledDistanceFare *= 0.25f;
    } else
        if (c.getAge() >= 65) {
        travelledDistanceFare *= 0.5f;
    }
    return 3.00f + travelledDistanceFare;
}

All the numbers in this code example could be considered magic numbers. For instance, the age thresholds for children and the elderly may seem like familiar numbers, but remember they could be used at many other places in the codebase. The fare rates are constants that are likely to change over time by business demands.

The next snippet shows what the code looks like if we define all magic constants explicitly. The code volume increased with six extra lines of code, which is a lot compared to the original source, but remember that these constants can be reused in many other places in the code:

private static final float BASE_RATE = 3.00f;
private static final float FARE_PER_KM = 0.10f;
private static final float DISCOUNT_RATE_CHILDREN = 0.25f;
private static final float DISCOUNT_RATE_ELDERLY = 0.5f;
private static final int MAXIMUM_AGE_CHILDREN = 12;
private static final int MINIMUM_AGE_ELDERLY = 65;

float calculateFare(Customer c, long distance) {
    float travelledDistanceFare = distance * FARE_PER_KM;
    if (c.getAge() < MAXIMUM_AGE_CHILDREN) {
        travelledDistanceFare *= DISCOUNT_RATE_CHILDREN;
    } else
        if (c.getAge() >= MINIMUM_AGE_ELDERLY) {
        travelledDistanceFare *= DISCOUNT_RATE_ELDERLY;
    }
    return BASE_RATE + travelledDistanceFare;
}

Rule 7: Leave No Badly Handled Exception Behind

Three guidelines for good exception handling are discussed here specifically because in our practice we see many flaws in implementing exception handling:

• Always catch exceptions. You are logging failures of the system to help you understand these failures and then improve the system’s reaction to them. That means that exceptions must always be caught. Also, in some cases an empty catch block compiles, but it is bad practice since it does not provide information about the context of the exception.

• Catch specific exceptions. To make exceptions traceable to a specific event, you should catch specific exceptions. General exceptions that do not provide information specific to the state or event that triggered it fail to provide that traceability. Therefore, you should not catch Throwable, Exception, or RuntimeException directly.

• Translate specific exceptions to general messages before showing them to end users. End users should not be “bothered” with detailed exceptions, since they are mostly confusing and a security bad practice (i.e., providing more information than necessary about the inner workings of the system).

Objection: Comments Are Our Documentation

“We use comments to document the workings of the code.”

Comments that tell the truth can be a valuable aid to less experienced developers who want to understand how a piece of code works. In practice, however, most comments in code lie—not on purpose, of course, but comments often tell an outdated version of the truth. Outdated comments become more and more common as the system gets older. Keeping comments in sync with code is a task that requires precision and a lot of times is overlooked during maintenance.

Code that “tells the story” itself does not require lengthy comments to document its workings. By keeping units small and simple, and by using descriptive names for identifiers, using comments for documentation is seldom necessary.

Objection: Exception Handling Causes Code Additions

“Implementing exception classes forces me to add a lot of extra code without visible benefits.”

Exception handling is an important part of defensive programming: coding to prevent unstable situations and unpredictable system behavior. Anticipating unstable situations means trying to foresee what can go wrong. This does indeed add to the burden of analysis and coding. However, this is an investment. The benefits of exception handling may not be visible now, but they definitely will prove valuable in preventing and absorbing unstable situations in the future.

By defining exceptions, you are documenting and safeguarding your assumptions. They can later be adjusted when circumstances change.