Clean Code is Not Threadsafe Code?

I am currently reading Clean Code: A Handbook of Agile Software Craftsmanship by Robert C. Martin and I’ve noticed a pattern.

In Listing 2-1, Martin recommends refactoring this code:

private void printGuessStatistics(char candidate, int count)
{
    String number;
    String verb;
    String pluralModifier;
    if (count == 0) {
        number = "no";
        verb = "are";
        pluralModifier = "s";
    } else if (count == 1) {
        number = "1";
        verb = "is";
        pluralModifier = "";
    } else {
        number = Integer.toString(count);
        verb = "are";
        pluralModifier = "s";
    }
    String guessMessage = String.format(
        "There %s %s %s%s", verb, number, candidate, pluralModifier
    );
    print(guessMessage);  
}

into this:

public class GuessStatisticsMessage {
    private String number;
    private String verb;
    private String pluralModifier;

    public String make(char candidate, int count) {
        createPluralDependentMessageParts(count);
        return String.format(
            "There %s %s %s%s",
            verb, number, candidate, pluralModifier);
    }

    private void createPluralDependentMessageParts(int count) {
        if (count == 0) {
            thereAreNoLetters();
        } else if (count == 1) {
            thereIsOneLetter();
        } else {
            thereAreManyLetterscount();
        }
    }

    private void thereAreManyLetters(int count) {
        number = Integer.toString(count);
        verb = "are";
        pluralModifier = "s";
    }

    private void thereIsOneLetter() {
        number = "1";
        verb = "is";
        pluralModifier = "";
    }

    private void thereAreNoLetters() {
        number = "no";
        verb = "are";
        pluralModifier = "s";
    }
}

In listing 4-7, Martin recommends refactoring this code (comments elided):

public class GeneratePrimes
{
    public static int[] generatePrimes(int maxValue)
    {
        if (maxValue >= 2) {
            int s = maxValue + 1;
            boolean f = new boolean[s];
            int i;

            for (i = 0; i < s; i++)
                f[i] = true;

            f[0] = f[1] = false;

            int j;
            for (i = 2; i < Math.sqrt(s) + 1; i++) {
                if (f[i]) {
                    for (j = 2 * i; j < s; j += i)
                        f[j] = false;
                }
            }

            int count = 0;
            for (i = 0; i < s; i++) {
                if (f[i]) {
                    count++;
                }
            }

            int[] primes = new int[count];

            for (i = 0, j = 0; i < s; i++) {
                if (f[i])
                    primes[j++] = i;
            }

            return primes;
        } else {
            return new int[0];
        }
    }
}

into this:

public class PrimeGenerator
{
    public static boolean[] crossedOut;
    public static int[] result;

    public static int[] generatePrimes(int maxValue)
    {
        if (maxValue < 2) {
            return new int[0];
        } else {
            uncrossIntegersUpTo(maxValue);
            crossOutMultiples();
            putUncrossedIntegersIntoResult();
            return result;
        }
    }

    private static void uncrossIntegersUpTo(int maxValue)
    {
        crossedOut = new boolean[maxValue + 1];
        for (int i = 2; i < crossedOut.length; i++)
            crossedOut[i] = false;
    }

    private static void crossOutMultiples()
    {
        int limit = determineIterationLimit();
        for (int i = 2; i <= limit; i++) {
            if (notCrossed(i))
                crossOutMultiplesOf(i);
        }
    }

    private static int determineIterationLimit()
    {
        double iterationLimit = Math.sqrt(crossedOut.length);
        return (int)iterationLimit;
    }

    private static void crossOutMultiplesOf(int i)
    {
        for (int multiple = 2*i; multiple < crossedOut.length; multiple += i)
            crossedOut[multiple] = true;
    }

    private static boolean notCrossed(int i)
    {
        return crossedOut[i] == false;
    }

    private static void putUncrossedIntegersIntoResult()
    {
        result = new int[numberOfUncrossedIntegers()];
        for (int j = 0, i = 2; i < crossedOut.length; i++) {
            if (notCrossed(i))
                result[j++] = i;
        }
    }

    private static int numberOfUncrossedIntegers()
    {
        int count = 0;
        for (int i = 2; i < crossedOut.length; i++)
            if (notCrossed(i))
                count++;

        return count;
    }
}

In both cases, Martin has transformed thread-safe, nearly pure functions into thread-unsafe functions. Even worse, while the former example can be used on multiple threads by having each thread instantiate a local instance of GuessStatisticsMessage, the latter example, due to its use of static member variables, cannot be used on multiple threads without some form of locking.

While I agree that Martin’s examples are more readable, it seems to me that there may be a fundamental tension between object-oriented programming and thread safety, and this tension does not exist in functional programming.

About Steven Engelhardt, CFA, AIF
Adjunct Professor of Software Engineering at DePaul University • Software Engineering, Data & Analytics in FinTech • Lives in Chicago, IL

3 Responses to Clean Code is Not Threadsafe Code?

  1. Andy says:

    Agreed. Easy OO refactor to make them instance fields and methods — that would be good footnote. I was looking for a policy recommendation in CC about use of static methods, didn’t see one yet, and so searched for examples, and the PrimeGenerator.

    Like

  2. Manu says:

    man! I thought exactly the same when I read that code, I could not believe it.

    very bad example of how to refactor a code. Instead, Martin should create a static method that allocates an instance of the class.

    Also, I also do not like how martin, uses static (and instance variables) as a way to pass “information” between functions.

    Martin is clearly not a mathematician.

    Like

  3. Chris M says:

    I’m reading Clean Code right now and while I’m finding many great insights, I was also flabbergasted to find such an “irresponsible” refactor. In the prime generator case, you could keep the methods static and pass the arrays around into the methods and it would be better.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

w

Connecting to %s