Working Effectively with Legacy Startup Code II

In this second instalment in a series on Legacy Code in the Startup world, Engineering Lead, James Murphy, demonstrates how we can fall back on an end-to-end test (even when we have no other tests) and with small refactors change existing legacy code to make it more manageable.

Following on from Part One, where we discussed how and why before a refactor it's important to cover your code with tests, we'll explore how you can make small refactors to transform your code once that code is covered. Also, we highlighted previously that, no matter how bad things get, you can always find some way to cover the code with a test or number of tests that are broad enough to allow you to perform any number of refactors.

So, let's dive in! A quick recap, let's look at the code we currently have:

package literatePrimes;

public class PrintPrimes {

    public static String print() {
        final int M = 1000;
        final int RR = 50;
        final int CC = 4;
        final int WW = 10;
        final int ORDMAX = 30;
        int[] P = new int[M + 1];
        int PAGENUMBER;
        int PAGEOFFSET;
        int ROWOFFSET;
        int C;

        int J;
        int K;
        boolean JPRIME;
        int ORD;
        int SQUARE;
        int N;
        int MULT[] = new int[ORDMAX + 1];

        J = 1;
        K = 1;
        P[1] = 2;
        ORD = 2;
        SQUARE = 9;

        StringBuffer primesOutput = new StringBuffer();
        while(K < M) {
            do {
                J = J + 2;
                if (J == SQUARE) {
                    ORD = ORD + 1;
                    SQUARE = P[ORD] * P[ORD];
                    MULT[ORD - 1] = J;
                }

                N = 2;
                JPRIME = true;
                while (N < ORD && JPRIME) {
                    while (MULT[N] < J)
                        MULT[N] = MULT[N] + P[N] + P[N];
                    if (MULT[N] == J)
                        JPRIME = false;
                    N = N + 1;
                }
            } while (!JPRIME);
            K = K + 1;
            P[K] = J;
        }

        {
            PAGENUMBER = 1;
            PAGEOFFSET = 1;
            while (PAGEOFFSET <= M) {
                primesOutput.append("The First " + M +
                        " Prime Numbers --- Page " + PAGENUMBER);
                primesOutput.append("\n");
                for (ROWOFFSET = PAGEOFFSET; ROWOFFSET < PAGEOFFSET + RR; ROWOFFSET++) {
                    for (C = 0; C < CC; C++)
                        if (ROWOFFSET + C * RR <= M) {
                            String offset = String.format("%10d", P[ROWOFFSET + C * RR]);
                            primesOutput.append(offset);
                        }
                    primesOutput.append("\n");
                }
                PAGENUMBER = PAGENUMBER + 1;
                PAGEOFFSET = PAGEOFFSET + RR * CC;
            }
        }
        return primesOutput.toString();
    }
}

Clearly the above code is practically more or less unreadable, or at the very least incredibly difficult to read without thinking for a bit. The thing about well written code is; we shouldn't have to think!

The first and easiest thing to clear things up, is to begin renaming variables. We can start with the final variables defined at the top of print() method.

final int M = 1000;
final int RR = 50;
final int CC = 4;
final int WW = 10;

M is the definition of max. number of primes that we wish to calculate. RR is the number of rows per page with CC being equivalent to the number of columns per page.

That leaves us with the obvious name refactor:

final int MAX_NUM_PRIMES = 1000;
final int ROWS_PER_PAGE = 50;
final int COLS_PER_PAGE = 5;

We replace every occurrence of a variable with the newly named variable. Remember the number one important thing is that we should always run our test after each refactor. That gives us the fall-back confidence that we aren't breaking anything going forward.

Our next step is to do the same thing with instance variables. It's amazing what a few well named variables will do for code clarity:

package literatePrimes;

public class PrintPrimes {

    public static String print() {
        final int MAX_NUM_PRIMES = 1000;
        final int ROWS_PER_PAGE = 50;
        final int COLS_PER_PAGE = 4;
        final int ORDMAX = 30;
        int[] primes = new int[MAX_NUM_PRIMES + 1];
        int pageNumber;
        int pageOffset;
        int rowOffset;
        int currentColumnNum;

        int nextNumber;
        int currentPrimeNumber;
        boolean isPrime;
        int ordinal;
        int square;
        int N;
        int squares[] = new int[ORDMAX + 1];

        nextNumber = 1;
        currentPrimeNumber = 1;
        primes[1] = 2;
        ordinal = 2;
        square = 9;

        StringBuffer primesOutput = new StringBuffer();
        while(currentPrimeNumber < MAX_NUM_PRIMES) {
            do {
                nextNumber = nextNumber + 2;
                if (nextNumber == square) {
                    ordinal = ordinal + 1;
                    square = primes[ordinal] * primes[ordinal];
                    squares[ordinal - 1] = nextNumber;
                }

                N = 2;
                isPrime = true;
                while (N < ordinal && isPrime) {
                    while (squares[N] < nextNumber)
                        squares[N] = squares[N] + primes[N] + primes[N];
                    if (squares[N] == nextNumber)
                        isPrime = false;
                    N = N + 1;
                }
            } while (!isPrime);
            currentPrimeNumber = currentPrimeNumber + 1;
            primes[currentPrimeNumber] = nextNumber;
        }

        {
            pageNumber = 1;
            pageOffset = 1;
            while (pageOffset <= MAX_NUM_PRIMES) {
                primesOutput.append("The First " + MAX_NUM_PRIMES +
                        " Prime Numbers --- Page " + pageNumber);
                primesOutput.append("\n");
                for (rowOffset = pageOffset; rowOffset < pageOffset + ROWS_PER_PAGE; rowOffset++) {
                    for (currentColumnNum = 0; currentColumnNum < COLS_PER_PAGE; currentColumnNum++)
                        if (rowOffset + currentColumnNum * ROWS_PER_PAGE <= MAX_NUM_PRIMES) {
                            String offset = String.format("%10d", primes[rowOffset + currentColumnNum * ROWS_PER_PAGE]);
                            primesOutput.append(offset);
                        }
                    primesOutput.append("\n");
                }
                pageNumber = pageNumber + 1;
                pageOffset = pageOffset + ROWS_PER_PAGE * COLS_PER_PAGE;
            }
        }
        return primesOutput.toString();
    }
}

As you can see, it is significantly clearer with cleaned up variable names.

Now we can see more clearly what the intention of the code is, we can begin to tackle it in bigger and better ways.

Let's think of what each part of the code does. This segment of the code is the code that calculates the list of prime numbers:

int squares[] = new int[ORDMAX + 1];

nextNumber = 1;
currentPrimeNumber = 1;
primes[1] = 2;
ordinal = 2;
square = 9;

while(currentPrimeNumber < MAX_NUM_PRIMES) {
    do {
        nextNumber = nextNumber + 2;
        if (nextNumber == square) {
            ordinal = ordinal + 1;
            square = primes[ordinal] * primes[ordinal];
            squares[ordinal - 1] = nextNumber;
        }

        N = 2;
        isPrime = true;
        while (N < ordinal && isPrime) {
            while (squares[N] < nextNumber)
                squares[N] = squares[N] + primes[N] + primes[N];
            if (squares[N] == nextNumber)
                isPrime = false;
            N = N + 1;
        }
    } while (!isPrime);
    currentPrimeNumber = currentPrimeNumber + 1;
    primes[currentPrimeNumber] = nextNumber;
}

Further down we have a static code block that contains logic for printing out prime numbers in rows, columns and pages:

{
    pageNumber = 1;
    pageOffset = 1;
    while (pageOffset <= MAX_NUM_PRIMES) {
        primesOutput.append("The First " + MAX_NUM_PRIMES +
                " Prime Numbers --- Page " + pageNumber);
        primesOutput.append("\n");
        for (rowOffset = pageOffset; rowOffset < pageOffset + ROWS_PER_PAGE; rowOffset++) {
            for (currentColumnNum = 0; currentColumnNum < COLS_PER_PAGE; currentColumnNum++)
                if (rowOffset + currentColumnNum * ROWS_PER_PAGE <= MAX_NUM_PRIMES) {
                    String offset = String.format("%10d", primes[rowOffset + currentColumnNum * ROWS_PER_PAGE]);
                    primesOutput.append(offset);
                }
            primesOutput.append("\n");
        }
        pageNumber = pageNumber + 1;
        pageOffset = pageOffset + ROWS_PER_PAGE * COLS_PER_PAGE;
    }
}
return primesOutput.toString();

Also, I'm not mad keen on for-loops without brackets either; in my eyes it's pretty bad form.

for (rowOffset = pageOffset; rowOffset < pageOffset + ROWS_PER_PAGE; rowOffset++) {
    for (currentColumnNum = 0; currentColumnNum < COLS_PER_PAGE; currentColumnNum++) {
        if (rowOffset + currentColumnNum * ROWS_PER_PAGE <= MAX_NUM_PRIMES) {
            String offset = String.format("%10d", primes[rowOffset + currentColumnNum * ROWS_PER_PAGE]);
            primesOutput.append(offset);
        }
    }
    primesOutput.append("\n");
}

As we mentioned earlier, the latter part of the primes method is used to generate some string output. So we can put it in a private method for the time being. Also, said private method has far too many parameters; many of which are to do with the print configuration. So let's put it in a PrintConfiguration object:

package literatePrimes;

public class PrintPrimes {

    public static String print() {
        final int MAX_NUM_PRIMES = 1000;
        final int ROWS_PER_PAGE = 50;
        final int COLS_PER_PAGE = 4;
        final int ORDMAX = 30;

        PrintConfiguration configuration = new PrintConfiguration(MAX_NUM_PRIMES, ROWS_PER_PAGE, COLS_PER_PAGE);

        int[] primes = new int[configuration.getMaxNumPrimes() + 1];

        int nextNumber;
        int currentPrimeNumber;
        boolean isPrime;
        int ordinal;
        int square;
        int primeNumber;
        int squares[] = new int[ORDMAX + 1];

        nextNumber = 1;
        currentPrimeNumber = 1;
        primes[1] = 2;
        ordinal = 2;
        square = 9;

        StringBuffer primesOutput = new StringBuffer();
        while(currentPrimeNumber < configuration.getMaxNumPrimes()) {
            do {
                nextNumber = nextNumber + 2;
                if (nextNumber == square) {
                    ordinal = ordinal + 1;
                    square = primes[ordinal] * primes[ordinal];
                    squares[ordinal - 1] = nextNumber;
                }

                primeNumber = 2;
                isPrime = true;
                while (primeNumber < ordinal && isPrime) {
                    while (squares[primeNumber] < nextNumber) {
                        squares[primeNumber] = squares[primeNumber] + primes[primeNumber] + primes[primeNumber];
                    }
                    if (squares[primeNumber] == nextNumber) {
                        isPrime = false;
                    }
                    primeNumber = primeNumber + 1;
                }
            } while (!isPrime);
            currentPrimeNumber = currentPrimeNumber + 1;
            primes[currentPrimeNumber] = nextNumber;
        }

        return generateOutput(configuration, primes, primesOutput);
    }

    private static String generateOutput(PrintConfiguration configuration, int[] primes, StringBuffer primesOutput) {
        int pageNumber;
        int pageOffset;
        int rowOffset;
        int currentColumnNum;
        pageNumber = 1;
        pageOffset = 1;

        while (pageOffset <= configuration.getMaxNumPrimes()) {
            primesOutput.append("The First " + configuration.getMaxNumPrimes() + " Prime Numbers --- Page " + pageNumber);
            primesOutput.append("\n");
            for (rowOffset = pageOffset; rowOffset < pageOffset + configuration.getRowsPerPage(); rowOffset++) {
                for (currentColumnNum = 0; currentColumnNum < configuration.getColsPerPage(); currentColumnNum++)
                    if (rowOffset + currentColumnNum * configuration.getRowsPerPage() <= configuration.getMaxNumPrimes()) {
                        String offset = String.format("%10d", primes[rowOffset + currentColumnNum * configuration.getRowsPerPage()]);
                        primesOutput.append(offset);
                    }
                primesOutput.append("\n");
            }
            pageNumber = pageNumber + 1;
            pageOffset = pageOffset + configuration.getRowsPerPage() * configuration.getColsPerPage();
        }
        return primesOutput.toString();
    }
}

Lastly, the final refactor for this session is to tidy up the remaining parameters. Sometimes we find ourselves initialising a parameter prior to calling a method, however, it makes sense to initialise the parameter within the context of the method instead. Hence why I moved the stringBuffer for building the output.

Also the generate output method was put in a static method inside PrimesPrinter class, since it makes it a class that has separate responsibility.

package literatePrimes;

public class PrintPrimes {

    public static String print() {
        final int MAX_NUM_PRIMES = 1000;
        final int ROWS_PER_PAGE = 50;
        final int COLS_PER_PAGE = 4;
        final int MAX_ORDINAL = 30;

        PrintConfiguration configuration = new PrintConfiguration(MAX_NUM_PRIMES, ROWS_PER_PAGE, COLS_PER_PAGE);

        int[] primes = new int[configuration.getMaxNumPrimes() + 1];

        int nextNumber;
        int currentPrimeNumber;
        boolean isPrime;
        int ordinal;
        int square;
        int primeNumber;
        int squares[] = new int[MAX_ORDINAL + 1];

        nextNumber = 1;
        currentPrimeNumber = 1;
        primes[1] = 2;
        ordinal = 2;
        square = 9;

        while(currentPrimeNumber < configuration.getMaxNumPrimes()) {
            do {
                nextNumber = nextNumber + 2;
                if (nextNumber == square) {
                    ordinal = ordinal + 1;
                    square = primes[ordinal] * primes[ordinal];
                    squares[ordinal - 1] = nextNumber;
                }

                primeNumber = 2;
                isPrime = true;
                while (primeNumber < ordinal && isPrime) {
                    while (squares[primeNumber] < nextNumber) {
                        squares[primeNumber] = squares[primeNumber] + primes[primeNumber] + primes[primeNumber];
                    }
                    if (squares[primeNumber] == nextNumber) {
                        isPrime = false;
                    }
                    primeNumber = primeNumber + 1;
                }
            } while (!isPrime);
            currentPrimeNumber = currentPrimeNumber + 1;
            primes[currentPrimeNumber] = nextNumber;
        }

        return PrimePrinter.generateOutput(configuration, primes);
    }
}

PrimePrinter.class

package literatePrimes;

public class PrimePrinter {

    public static String generateOutput(PrintConfiguration configuration, int[] primes) {
        StringBuffer primesOutput = new StringBuffer();

        int pageNumber;
        int pageOffset;
        int rowOffset;
        int currentColumnNum;
        pageNumber = 1;
        pageOffset = 1;

        while (pageOffset <= configuration.getMaxNumPrimes()) {
            primesOutput.append("The First " + configuration.getMaxNumPrimes() + " Prime Numbers --- Page " + pageNumber);
            primesOutput.append("\n");
            for (rowOffset = pageOffset; rowOffset < pageOffset + configuration.getRowsPerPage(); rowOffset++) {
                for (currentColumnNum = 0; currentColumnNum < configuration.getColsPerPage(); currentColumnNum++)
                    if (rowOffset + currentColumnNum * configuration.getRowsPerPage() <= configuration.getMaxNumPrimes()) {
                        String offset = String.format("%10d", primes[rowOffset + currentColumnNum * configuration.getRowsPerPage()]);
                        primesOutput.append(offset);
                    }
                primesOutput.append("\n");
            }
            pageNumber = pageNumber + 1;
            pageOffset = pageOffset + configuration.getRowsPerPage() * configuration.getColsPerPage();
        }
        return primesOutput.toString();
    }
}

And so we're done for this session. I hope you agree, that despite having sometimes terrible legacy code, it is possible to improve it. Little by little, small refactor by small refactor we can turn once unreadable code into code that makes sense.

If you'd like to take a look at the example, the code can be found on Github

Until next time.