리팩터링 연습 #1 <클린 코드> 90p GeneratePrimes 클래스

클린 코드에 쌓인 먼지를 털어낸 뒤 다시 읽기 시작했다. 저자가 서문에서 이 책은 가볍게 읽을 책이 아니고 앞으로 갔다가 뒤로 갔다가 하면서, 많은 노력을 들여 코드를 최대한 이해하려고 해야 한다고 신신당부했다. 그렇게 읽어보려고 한다.

 

90p에 보면 GeneratePrimes 클래스가 나쁜 버전으로 주어지고, 이를 리팩터링 한 결과를 다음 페이지에 보여주고 있다. 그냥 보는 것보다는 나라면 어떻게 고칠지 생각해보고, 실제로 고쳐보고, 정답(?)과 비교해보는 게 더 좋을 것 같아서 한 번 리팩터링을 해보았다.

 

먼저 나쁜 버전 원본이다.

 

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];
        }
    }

}

 

원래는 여기저기 주석이 붙어있지만 주석은 모두 생략하였다.

 

로직이 더 간단한 else를 if 자리에 두자

 

generatePrimes 메서드의 로직은 if와 else로 나뉘는데 if에 모든 로직이 들어가 있고, else에서는 단순히 리턴만 하고 있다. 나는 로직이 간단한 게 먼저와야 전체적인 맥락을 파악하기 쉽다고 생각한다.

 

public class GeneratePrimes {

    public static int[] generatePrimes(int maxValue) {
        if (maxValue < 2) {
            return new int[0];
        }

        int s = maxValue + 1;
        boolean[] f = new boolean[s];
        int i;
        
        // ...
    }

}

 

"아 generatePrimes라는 메서드에서는 maxValue가 2 보다 작은 경우 예외적 상황에 해당하고 2 이상일 때 어떤 로직을 수행하는구나" 하고 수정하기 전보다 더 빨리 캐치할 수 있게 되었다.

 

성의 있는 변수 이름을 붙이자

 

반복변수 i, j는 그렇다고 치자. 그런데 배열의 길이를 s로, 인덱스에 대응되는 숫자가 소수인지 아닌지에 대한 정보를 담고 있는 불린 배열을 f라고 부르는 것은 근본적으로는 변수 이름 자체가 의미 있는 정보를 담고 있지 않기 때문에 나쁘다. 뿐만 아니라 너무 성의가 없어 보여서 보는 사람을 속상하게 만든다. 성의를 더해서 아래와 같이 수정하였다.

 

public class GeneratePrimes {

    public static int[] generatePrimes(int maxValue) {
        if (maxValue < 2) {
            return new int[0];
        }

        int size = maxValue + 1;
        boolean[] isPrimeNumberArray = new boolean[size];
        int i;

        for (i = 0; i < size; i++)
            isPrimeNumberArray[i] = true;

        isPrimeNumberArray[0] = isPrimeNumberArray[1] = false;

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

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

        int[] primes = new int[count];
        for (i = 0, j = 0; i < size; i++) {
            if (isPrimeNumberArray[i])
                primes[j++] = i;
        }

        return primes;
    }

}

 

더 고민하면 더 좋은 이름을 떠올릴 수 있겠지만, 일단 변수 네이밍이 문제인 것을 파악했고 더 나은 변수 이름을 나름대로 붙여보았으므로 일단 넘어가도록 한다.

 

행동에 이름을 붙여보자

 

행동에 이름을 붙이지 않고 여러 행동들을 코드로 나열하게 되면 가독성이 매우 떨어진다. 위의 코드가 기껏해야 중첩 반복문이 하나 있는 매우 단순해 보이는 구조임에도 술술 읽히지 않는 이유는 행동들에 이름을 붙이지 않아서라고 생각한다. 아래와 같이 행동에 이름을 붙여보았다.

 

import java.util.Arrays;

public class GeneratePrimes {

    public static int[] generatePrimes(int maxValue) {
        if (maxValue < 2) {
            return new int[0];
        }

        int size = maxValue + 1;
        boolean[] isPrimeNumberArray = new boolean[size];
        Arrays.fill(isPrimeNumberArray, true);

        markPrimeNumber(isPrimeNumberArray);
        return getPrimeNumberArray(isPrimeNumberArray);
    }

    private static void markPrimeNumber(boolean[] isPrimeNumberArray) {
        isPrimeNumberArray[0] = isPrimeNumberArray[1] = false;
        for (int number = 2; number < Math.sqrt(isPrimeNumberArray.length) + 1; number++) {
            if (isPrimeNumberArray[number]) {
                markMultipleOfNumber(isPrimeNumberArray, number);
            }
        }
    }

    private static void markMultipleOfNumber(boolean[] isPrimeNumberArray, int number) {
        for (int i = 2 * number; i < isPrimeNumberArray.length; i += number)
            isPrimeNumberArray[i] = false;
    }

    private static int[] getPrimeNumberArray(boolean[] isPrimeNumberArray) {
        int count = countPrimeNumber(isPrimeNumberArray);
        int[] primes = new int[count];
        for (int i = 0, j = 0; i < isPrimeNumberArray.length; i++) {
            if (isPrimeNumberArray[i]) {
                primes[j++] = i;
            }
        }
        return primes;
    }

    private static int countPrimeNumber(boolean[] isPrimeNumberArray) {
        int count = 0;
        for (boolean isPrimeNumber : isPrimeNumberArray) {
            if (isPrimeNumber) {
                count++;
            }
        }
        return count;
    }

}

 

 

전체 코드의 길이는 늘어났지만 가독성이 훨씬 좋아졌다. 이제 generatePrimes 메서드를 읽어보면, "아 소수인지 아닌지 판단하는 배열을 만들고, 해당 배열에 소수인지 아닌지를 표시한 뒤, 그중에서 소수로 표시한 것만 추출해서 반환하는 메서드구나"라고 보다 빠르게 파악할 수 있게 되었다.

 

정답(?)과 비교

 

책에 있는 리팩터링 결과와 비교해보았다. 꽤나 비슷하게 리팩터링이 이루어졌는데 내 리팩터링에서 더 개선할 수 있는 부분만 보자면 다음과 같다.

 

- 클래스 필드로 두기

isPrimeNumberArray라는 배열을 모든 private 메서드에서 들고 다니면서 사용하고 있다. 이 배열이 굳이 generatePrimes 메서드의 지역변수로 남아있을 이유가 있을까? 소수를 생성하는 유틸성 클래스 GeneratePrimes 안에서 소수를 생성할 때 사용되는 것이므로 클래스 필드로 있어도 자연스럽다. 이렇게 되면 번거롭게 매개변수로 들고 다닐 필요가 없어진다.

 

- markPrimeNumber에서 반복문의 종료조건 속 수식에 대해 설명하기

숫자의 검사 범위가 왜 저렇게 되는지에 대해서는 추가적인 설명이 필요하므로 주석을 달거나 별도의 메서드로 분리해서 더 설명해주는 게 낫다.

 

- 클래스 이름 수정하기

여기도 리팩터링 대상임을 놓쳤다. 클래스 이름이 GeneratePrimes인 것을 보고 "분명 클래스 이름은 명사나 명사구가 좋다고 했는데 얘는 유틸성이라 동사구로 지은건가?" 라고만 생각하고 그냥 넘어갔는데 책의 리팩터링 결과를 보니 클래스 이름도 PrimeGenerator로 수정이 되어 있었다. 클래스 이름은 명사나 명사구가 자연스럽다.