Good Software Design - Part 3: Refactoring

Would you eat here if the kitchen were a mess?

“Refactoring” is the practice of improving the design of the code without altering its behaviors. I’ve always likened it to cleaning up a publicly-visible restaurant kitchen: you can’t cook without making a small mess, and you don’t want to let the mess grow out of control. Otherwise it could become unappetizing, or even dangerous.

Remember this code from Part 2?

public static ProductBean installProduct(String p)  {
    ProductBean pi = new ProductBean(p);
    Connection c = new Connection(“$Updates”);

    for (Record x : c.getAllForKey(“PLUG_INS”, “P_NAME”, p)) {
        // validate serial number
        if (“SNUM”.equals(x.getString(“RECORD_TYPE”))) 
        {
            SecurityHelper.validateProduct(x.getString(“SERIAL_NUMBER”),
                getEncryptionCertificate());
            pi.addSerialNumber(x.getString(“SERIAL_NUMBER”));
        }

        // install license
        if (“LIC”.equals(x.getString(“RECORD_TYPE”))) {
            SecurityHelper.validateLicense(x.getString(“USER_LICENSE”),
                getEncryptionCertificate());
            SecurityHelper.registerLicense(x.getString(“USER_LICENSE”));
            pi.addLicense(x.getString(“USER_LICENSE”));
        }

        // install driver
        if (“PDRV”.equals(x.getString(“RECORD_TYPE”)))  {
            Drivers.hotInstall(x.getBlob(“BIN_DRIVER”));
        }
    }

    return pi;
}

There are at least three ways this code could be made better. The choice depends on any known history for this code, the goal of the refactoring efforts, as well as the confidence to try a refactoring or two and get a sense for which will get you most quickly to your goals.

Let’s listen in as Bob and Alice refactor the snot out of the method above.

Alice: “Phew! Long Method!”

Bob: “Yeah. And the loop is doing three separate things…”

Alice: “Okay, let’s try Split Loop.”

Alice opens a browser tab to https://www.refactoring.com/catalog/splitLoop.html just to be sure they don’t miss a tiny but critical step.

Bob: “Hey, cool: We made it worse.”

public static ProductBean installProduct(String p) {
    ProductBean pi = new ProductBean(p);
    Connection c = new Connection("$Updates");

    for (Record x : c.getAllForKey("PLUG_INS", "P_NAME", p)) {
        // validate serial number
        if ("SNUM".equals(x.getString("RECORD_TYPE"))) {
            SecurityHelper.validateProduct(
                    x.getString("SERIAL_NUMBER"),
                    getEncryptionCertificate());
            pi.addSerialNumber(x.getString("SERIAL_NUMBER"));
        }
    }

    for (Record x : c.getAllForKey("PLUG_INS", "P_NAME", p)) {
        // install license
        if ("LIC".equals(x.getString("RECORD_TYPE"))) {
            SecurityHelper.validateLicense(
                    x.getString("USER_LICENSE"),
                    getEncryptionCertificate());
            SecurityHelper.registerLicense(
                    x.getString("USER_LICENSE"));
            pi.addLicense(x.getString("USER_LICENSE"));
        }
    }

    for (Record x : c.getAllForKey("PLUG_INS", "P_NAME", p)) {
        // install driver
        if ("PDRV".equals(x.getString("RECORD_TYPE"))) {
            Drivers.hotInstall(x.getBlob("BIN_DRIVER"));
        }

    }

    return pi;
}

Bob and Alice now run all the tests to make sure they pass. Refactorings aren’t supposed to change the behaviors, so if a test fails, it wasn’t really a refactoring. In that case, they would undo all changes (e.g., revert from their git repo).

Did they really make it worse, or did they just make some of the duplication even more obvious?

Alice: “This code is going to give me a headache. Let’s do some general housekeeping on all that duplicated code and all the abbreviated variable names.”

Code smells often hide other code smells, so it’s a good idea to trust your own judgment (or that of your Pair-Programming partner) regarding the most obvious or most potent stink; clean that up, then take another sniff.

Also check in frequently with your intended goal. Are you refactoring to better understand the code, to find and fix a defect, or to add new functionality? It’d be great if we could spend months rewriting all our old code so it was “perfect,” but that alone won’t provide demonstrable business value. Without a purposeful destination, even refactoring can become a waste of time.

Bob proceeds to use his knowledge of the IDE’s refactoring hotkeys (mostly Rename Variable and Extract Local Variable), and runs the tests after each refactoring. (Alternatively, there are awesome IDE plug-ins, like NCrunch for VisualStudio.Net, that will run tests in parallel with your editing activities.)

public static final String RECORD_TYPE = "RECORD_TYPE";

public static ProductBean installProduct(String productID) {
    ProductBean product = new ProductBean(productID);
    Connection connection = new Connection("$Updates");
    List<Record> allRecords = connection.getAllForKey("PLUG_INS", "P_NAME", productID);

    for (Record next : allRecords) {
        // validate serial number
        if ("SNUM".equals(next.getString(RECORD_TYPE))) {
            String serialNumber = next.getString("SERIAL_NUMBER");
            SecurityHelper.validateProduct(
                    serialNumber,
                    getEncryptionCertificate());
            product.addSerialNumber(serialNumber);
        }
    }

    for (Record next : allRecords) {
        // install license
        if ("LIC".equals(next.getString(RECORD_TYPE))) {
            String license = next.getString("USER_LICENSE");
            SecurityHelper.validateLicense(
                    license,
                    getEncryptionCertificate());
            SecurityHelper.registerLicense(
                    license);
            product.addLicense(license);
        }
    }

    for (Record next : allRecords) {
        // install driver
        if ("PDRV".equals(next.getString(RECORD_TYPE))) {
            Drivers.hotInstall(next.getBlob("BIN_DRIVER"));
        }
    }

    return product;
}

Alice: “Wow. Headache cured. Now let’s clean up that Long Method by doing an Extract Method on each loop.”

Bob: “Okay…Validate Serial Numbers, Install Licenses, Install Drivers… ”

Alice: “And let’s delete those comments now that they’re extraneous.”

public static final String RECORD_TYPE = "RECORD_TYPE";

public static ProductBean installProduct(String productID) {
    ProductBean product = new ProductBean(productID);
    Connection connection = new Connection("$Updates");
    List<Record> allRecords = connection.getAllForKey("PLUG_INS", "P_NAME", productID);

    validateSerialNumber(product, allRecords);
    installLicense(product, allRecords);
    installDrivers(allRecords);

    return product;
}

private static void validateSerialNumber(ProductBean product, List<Record> allRecords) {
    for (Record next : allRecords) {
        if ("SNUM".equals(next.getString(RECORD_TYPE))) {
            String serialNumber = next.getString("SERIAL_NUMBER");
            SecurityHelper.validateProduct(
                    serialNumber,
                    getEncryptionCertificate());
            product.addSerialNumber(serialNumber);
        }
    }
}

private static void installLicense(ProductBean product, List<Record> allRecords) {
    for (Record next : allRecords) {
        if ("LIC".equals(next.getString(RECORD_TYPE))) {
            String license = next.getString("USER_LICENSE");
            SecurityHelper.validateLicense(
                    license,
                    getEncryptionCertificate());
            SecurityHelper.registerLicense(
                    license);
            product.addLicense(license);
        }
    }
}

private static void installDrivers(List<Record> allRecords) {
    for (Record next : allRecords) {
        // install driver
        if ("PDRV".equals(next.getString(RECORD_TYPE))) {
            Drivers.hotInstall(next.getBlob("BIN_DRIVER"));
        }
    }
}

Alice: “Much better, yes?”

Bob: “Yeah, but we’ve increased the amount of structural duplication with that Split Loop. I’m tempted to create some lambdas to remove the loops.”

Alice: “I see what you mean. The trouble is, we have teams who use this code but aren’t yet lambda-savvy. It’s a lot more readable than it was, and we’ve made our next enhancements easier to achieve.”

Bob: “We’ve also made it slower by tripling the times we loop…oh, pretend I didn’t say that!”

Alice: “Right. Very few records for each product, and the most time-consuming part of this is the driver installation. The code now has less cognitive load—easier to maintain!”

Bob: “Okay! I’m pushing our commits to the trunk!”

Refactoring Is Design

Refactoring is software design. It’s reshaping the design of the code, not rewriting the code. Typically, refactoring reduces duplication, resulting in less code.

Software design, in turn, is often a mystery to those outside the developer community. It has something to do with how the code accomplishes its functionality, but the tests or specifications do an even better job of that, so what exactly is software design?

A good software design is one that helps the team understand what the code is doing, and allows the team to introduce new functionality without disturbing old. So good design includes robustness (through tests, and by isolating changes), clarity (ubiquitous naming, simple structures, modularity), and maintainability. Less code and less duplication make the system much easier to maintain and extend.

A good design is, therefore, code that passes all the tests, and that the team agrees facilitates change.

In the finale of this mini-series about Good Design, we’ll take a closer look at the role of tests in design, and we’ll also explore how some Agile team practices have a direct impact on the quality of your software design.