10

I am currently reading and working through "Clean Code: A Handbook of Agile Software Craftsmanship" by Robert C. Martin. The author talks about how a function should do one thing only, and thus be relatively short. Specifically Martin writes:

This implies that the blocks within if statements, else statements, while statements, and so on should be one line long. Probably that line should be a function call. Not only does this keep the enclosing function small, but it also adds documentary value because the function called within the block can have a nicely descriptive name.

This also implies that functions should not be large enough to hold nested structures. Therefore, the indent level of a function should not be greater than one or two. This, of course, makes the functions easier to read and understand

This makes sense, but seems to conflict with examples of what I see as clean code. Take the following method for example:

    public static boolean millerRabinPrimeTest(final int n) {
        final int nMinus1 = n - 1;
        final int s = Integer.numberOfTrailingZeros(nMinus1);
        final int r = nMinus1 >> s;
        //r must be odd, it is not checked here
        int t = 1;
        if (n >= 2047) {
            t = 2;
        }
        if (n >= 1373653) {
            t = 3;
        }
        if (n >= 25326001) {
            t = 4;
        } // works up to 3.2 billion, int range stops at 2.7 so we are safe :-)
        BigInteger br = BigInteger.valueOf(r);
        BigInteger bn = BigInteger.valueOf(n);

        for (int i = 0; i < t; i++) {
            BigInteger a = BigInteger.valueOf(SmallPrimes.PRIMES[i]);
            BigInteger bPow = a.modPow(br, bn);
            int y = bPow.intValue();
            if ((1 != y) && (y != nMinus1)) {
                int j = 1;
                while ((j <= s - 1) && (nMinus1 != y)) {
                    long square = ((long) y) * y;
                    y = (int) (square % n);
                    if (1 == y) {
                        return false;
                    } // definitely composite
                    j++;
                }
                if (nMinus1 != y) {
                    return false;
                } // definitely composite
            }
        }
        return true; // definitely prime
    }
}

This code is taken from the Apache Commons source code repo at: https://github.com/apache/commons-math/blob/master/src/main/java/org/apache/commons/math4/primes/SmallPrimes.java

The method looks very readable to me. For algorithm implementations like this one (implementation of Miller-Rabin Probabilistic Primality Test), is it suitable to keep the code as is and still consider it 'clean', as defined in the book? Or would even something already as readable as this benefit from extracting methods to make the algorithm essentially a series calls to functions that "do one thing only"? One quick example of a method extraction might be moving the first three if-statements to a function like:

private static int getTValue(int n)
    {
        int t = 1;
        if (n >= 2047) {
            t = 2;
        }
        if (n >= 1373653) {
            t = 3;
        }
        if (n >= 25326001) {
            t = 4;    
        }
        return t;
    }

Note: This question is different that the possible duplicate (though that question is helpful to me too), because I am trying to determine if I am understanding the intention of the author of Clean Code and I am providing a specific example to make things more concrete.

  • 3
    as far as I can see this function does only one thing... it has no side effects that I can see. What makes you think it might not be clean ? Which part of this function you would consider worthy of being put in another function to make it cleaner ? – Newtopian Aug 11 '17 at 16:31
  • 14
    Your question title asks for "real life" situation, and then your example looks to me like a perfect example of a non-real-life function (at least, for 99.9% of application or web developers). It may be a real-life function for number theorists, mathematicians or computer scientists working in that specific field, of course. – Doc Brown Aug 11 '17 at 16:39
  • 1
  • 2
    Yes, for me this is real life as I currently develop in the field of computational algebraic number theory :) – 1west Aug 11 '17 at 16:44
  • 2
    I would likely refactor the getTFactor() as you describe. – user949300 Aug 11 '17 at 18:09
17

"Clean code" is not an end in itself, it is a means to an end. The main purpose of refactoring bigger functions into smaller ones and cleaning up the code in other ways is to keep the code evolvable and maintainable.

When picking such a very specific mathematical algorithm like the "Miller-Rabin" prime test from a text book, most programmers do not want to evolve it. Their standard goal is to transfer it from the pseudo code of the text book correctly into the programming language of their environment. For this purpose, I would recommend to follow the text book as close as possible, which typically means not to refactor.

However, for someone working as mathematician in that field who is trying to work on that algorithm and change or improve it, IMHO splitting up this function into smaller, well named ones, or replacing the bunch of "magic numbers" by named constants, can help to make changes to the code easier like for any other kind of code as well.

  • 1
    This is exactly what I was looking for. I was having trouble determining when to use the clean code practices in the field I am developing in. Your answer provides the clarity I was looking for. Thanks! – 1west Aug 11 '17 at 17:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.