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.

Tulains Córdova
  • 39,201
  • 12
  • 97
  • 154
1west
  • 111
  • 6
  • 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
  • 15
    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
    Possible duplicate of [How would you know if you've written readable and easily maintainable code?](https://softwareengineering.stackexchange.com/questions/141005/how-would-you-know-if-youve-written-readable-and-easily-maintainable-code) – Greg Burghardt Aug 11 '17 at 16:44
  • 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
  • 1
    Clean Code also talks about magic numbers and the sample code has a few. – Tulains Córdova Aug 11 '17 at 21:49
  • I'd write int t = (n < 2047 ? 1 : n < 1373653 ? 2 : n < 25326001 ? 3 : 4); Would be good to add a comment what these numbers are good for though. – gnasher729 Aug 12 '17 at 12:40
  • @user949300: Would you also add an explanation what the hell this code is trying to achieve (no need to tell me, I know how Miller-Rabin works)? Because these constants are not "magic", they have a very specific reason to have exactly those values. – gnasher729 Aug 12 '17 at 17:55
  • 1
    Keep in mind that the type of recommendations in this book fall under the category of cargo cult design, or cargo cult implementation: using the same techniques for all code, regardless of the circumstances. Clean code is not a phrase to be taken out and turned into a cult; it had a meaning before the book was written. – Frank Hileman Aug 13 '17 at 23:49
  • Besides magic numbers, and cryptic variable names, that function is definitely too long, and contains deep nests as well. Would absolutely not pass a code review at my shop. – Eternal21 Aug 15 '17 at 18:18
  • @Eternal21 What would you change? How far would you go in extracting methods? – 1west Aug 16 '17 at 02:35

1 Answers1

18

"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.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 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