개발 미션과 함께 읽는 클린 코드

Mar.20.2019 오지산

Backend

{{item.name}}
- {{sub.name}}

들어가며

안녕하세요, 미래사업부문의 오지산이라고 합니다.
저는 작년 10월 우아한형제들에 신입 개발자로 입사하면서 처음으로 Spring 위에서 코드를 작성하는 경험을 했습니다.

omg

0의 지식으로 부딪혀야 하는 상황이었는데요.
거기에 신규 서비스를 개발해야 하는 만큼, 배달의민족을 비롯한 사내 다른 서비스의 코드를 참고하거나
여러 공식 문서들을 읽어가며 개발 미션을 수행하게 되었습니다.

책 표지

그 과정에서 많이 배우고 의지했던(?) 책이 <클린 코드>였는데요.
이 책 자체를 읽고, 정리하고, 저자의 의도를 파악하는 것이 저에게 주어진 미션 중 하나였습니다.

신입 개발자에게는 코드를 작성하면서 어떤 가치를 지향해야 하는지가 불명확하게 느껴집니다.
그래서 지금 생각하면 더없이 좋은 시기에 이 책을 읽을 수 있었던 것 같은데요.
이 글에서는 제가 <클린 코드>를 읽으며 배운 것들, 그리고 코드 수준에서 겪었던 변화들을 공유하려고 합니다.

어떤 코드를 깨끗하다고 말할 수 있죠?

책에서는 이름, 함수, 클래스, 오류 처리, 동시성 등 다양한 관점에서 이를 논하고 있어서
한번에 이를 요약해서 정의내리면 자칫 어설퍼질 수 있을 것 같습니다.
따라서 코드 수준에서 보여드릴 수 있도록, 몇 가지 예시를 준비해 보았습니다.

(또한 책의 내용을 직접적으로 언급하는 경우, 쪽번호를 적어 두었으니 참고하시면 좋을 것 같습니다.)

시작은 테스트부터

종종 테스트를 염두에 두면 코드가 깨끗해지기 쉽다는 것을 느낍니다.
특히 통합 테스트가 아닌, Mock과 Stub 객체를 사용하여 의존성을 격리하는 단위 테스트의 경우
해당 코드의 복잡성을 여실히 드러내어 이를 개선할 기회를 주는 것 같습니다.

혹시 테스트 코드를 짜면서 예상보다 훨씬 복잡해지고 있다는 느낌을 받으신 적 있나요?

def "GetResource: 정상 요청"() {
    given:
    def key = resolveKey(filename)
    amazonS3.doesObjectExist(bucketName, key) >> true
    def stream = new ByteArrayInputStream(content.getBytes())
    def s3Object = Mock(S3Object.class) {
        getObjectContent() >> new S3ObjectInputStream(stream, null)
    }

    when:
    def resource = s3StorageService.getResource(filename)

    then:
    1 * amazonS3.getObject(bucketName, key) >> s3Object
    resource.getInputStream().getText() == content

    where:
    filename      | content
    "sample-file" | "Sample Data"
}

def resolveKey(String filename) {
    return Paths.get(location, filename).toString()
}

이 코드는 제가 첫 개발 미션에서 Spock을 이용해 작성한 테스트인데요.
코드는 어렵지 않지만 테스트의 의도는 모호해 보입니다.
테스트 함수명도 "정상 요청"이라고 ~~퉁쳐서~~ 모호하게 써두고 있네요.

또한 ByteArrayInputStream을 S3ObjectInputStream 생성자에 끼워 넣고,
s3Object.getObjectContent()가 S3ObjectInputStream 객체를 리턴하게 한 뒤,
amazonS3.getObject(..)가 s3Object를 리턴하게 만드는 등…
인형 안에 인형을 넣는 마트료시카를 만드는 느낌이 드는데요.

마트료시카

테스트 코드의 구성 자체가, 해당 함수가 여러가지 일을 하고 있다는 강한 느낌을 줍니다.
테스트하고자 하는 함수를 보면…

@Service
@RequiredArgsConstructor
public class S3StorageService implements StorageService {
    private final String bucketName;
    private final Path rootLocation;
    private final AmazonS3 amazonS3;

    public Resource getResource(String filename) {
        String key = rootLocation.resolve(filename).toString();
        if (!amazonS3.doesObjectExist(bucketName, key)) {
            throw new FileNotFoundException();
        }
        S3Object object = amazonS3.getObject(bucketName, key);
        return new InputStreamResource(object.getObjectContent());
    }
}

역시 이 함수가 많은 일을 한꺼번에 하고 있다는 것을 알 수 있습니다.

전달받은 filename에서 key 만들기
S3 버킷에 해당 key를 가진 객체가 존재하는지 확인하기
S3 버킷에서 key로 객체 가져오기
S3Object에서 InputStream을 꺼내와서, Resource에 담아 돌려주기

4가지 일에 대한 사전 작업을 해야 했기 때문에, 테스트 코드가 복잡해질 수밖에 없었던 것 같네요.
이제부터는 <클린 코드>의 내용을 몇 가지 소개하면서, 위 함수를 점진적으로 개선해 보겠습니다.

하나의 함수가 하나의 일을 하게

<클린 코드>에서는 좋은 함수의 조건 중 하나로 한 가지 일을 잘 하는 것을 제시합니다. (p.44)
우선 각각의 일에 대하여 함수를 나누어 보겠습니다.

public Resource getResource(String filename) {
    String key = resolveKey(filename);
    S3Object s3Object = getS3Object(key);
    return makeResource(s3Object);
}

private String resolveKey(String filename) {
    return rootLocation.resolve(filename).toString();
}

private S3Object getS3Object(String key) {
    checkIfObjectExists(key);
    return amazonS3.getObject(bucketName, key);
}

private void checkIfObjectExists(String key) {
    if (!amazonS3.doesObjectExist(bucketName, key)) {
        throw new FileNotFoundException();
    }
}

private Resource makeResource(S3Object object) {
    return new InputStreamResource(object.getObjectContent());
}

전보다 getResource(..) 함수가 더 요약적으로 변한 것을 알 수 있습니다.
또한 getResource(..)에서 호출되는 함수를 호출 순서대로 아래에 나열했는데요.

<클린 코드>에서는 이와 같은 코드 형식을 신문 기사에 비유하고 있습니다. (p.98)
신문 기사가 요약을 위로, 세세한 사실을 아래로 배치하는 것처럼,
코드도 가장 추상적인 개념을 위로, 저차원 함수를 아래로 배치하여 독자의 빠른 이해를 돕습니다.

하나의 클래스가 한 가지 책임을 갖게

함수를 분리했다고 해도 아직 테스트 코드에 손을 댈 수는 없습니다.
getResource(..)에서 아래 네 가지 일이 모두 일어나는 것은 동일한데요.

전달받은 filename에서 key 만들기
S3 버킷에 해당 key를 가진 객체가 존재하는지 확인하기
S3 버킷에서 key로 객체 가져오기
S3Object에서 InputStream을 꺼내와서, Resource에 담아 돌려주기

애초에 제가 S3StorageService를 만든 이유는 AWS와 통신하여 S3 버킷에 저장된 객체를 가져오기 위함이었습니다.
2번과 3번 정도가 제 의도에 부합하고, 나머지는 미처 생각하지 못한 부가 작업이네요.

이때 재밌는 것은, 함수를 나눴더니 1번과 4번이 겉도는 느낌이 든다는 것입니다.
방금 전 코드에서 1번과 4번에 해당하는 resolveKey(..), makeResource(..)만 한번 보면…

@Service
public class S3StorageService implements StorageService {
    private final String bucketName;
    private final Path rootLocation;
    private final AmazonS3 amazonS3;

    private String resolveKey(String filename) {
        return rootLocation.resolve(filename).toString();
    }

    private Resource makeResource(S3Object object) {
        return new InputStreamResource(object.getObjectContent());
    }
}

다른 함수가 인스턴스 변수인 amazonS3와 bucketName을 모두 사용하고 있는 반면,
resolveKey(..)는 rootLocation이라는 인스턴스 변수 하나만 사용하고 있습니다.
rootLocation 또한 resolveKey(..)에서밖에 쓰이지 않고요.
게다가 makeResource(..)는 인스턴스 변수를 하나도 안 쓰고 있네요.

즉 두 메서드로 인해 클래스의 응집도가 낮아진다는 것을 알 수 있습니다. (p.177)
한 클래스가 갖는 응집도는 개별 메서드가 얼마나 많은 인스턴스 변수를 사용하는지에 따라 결정되는데요.
응집도가 높을수록 해당 메서드와 인스턴스 변수가 논리적으로 묶이는 효과를 갖습니다.

그러고 보니 AWS와 통신하는 클래스에 key를 만드는 로직이나,
InputStream을 빼내서 Resource에 담아주는 로직은 어울리지 않아 보입니다.
두 함수를 조심스럽게 떼내어 볼까요?

먼저 resolveKey(..)를 떼내어 보겠습니다.
KeyResolver라는 인터페이스를 만들어 resolveKey(..)를 정의해 두고…

public interface KeyResolver {
    String resolveKey(String filename);
}

S3KeyResolver를 만들어 KeyResolver를 구현합니다.
좀 전의 resolveKey(..)는 여기로 옮겨지게 됩니다.

@RequiredArgsConstructor
public class S3KeyResolver implements KeyResolver {
    private final Path rootLocation;

    public String resolveKey(String filename) {
        return rootLocation.resolve(filename).toString();
    }
}

그리고 S3StorageService가 KeyResolver에 의존하게 해 보았습니다.
이렇게 하면 나중에라도 KeyResolver를 구현하는 새로운 클래스를 만들었을 때,
S3StorageService와 자유롭게 결합해서 쓸 수 있습니다.

@Service
@RequiredArgsConstructor
public class S3StorageService implements StorageService {
    private final String bucketName;
    private final AmazonS3 amazonS3;
    private final KeyResolver keyResolver;

    @Override
    public Resource getResource(String filename) {
        String key = keyResolver.resolveKey(filename);
        S3Object s3Object = getS3Object(key);
        return makeResource(s3Object);
    }

    private S3Object getS3Object(String key) {
        checkIfObjectExists(key);
        return amazonS3.getObject(bucketName, key);
    }

    private void checkIfObjectExists(String key) {
        if (!amazonS3.doesObjectExist(bucketName, key)) {
            throw new FileNotFoundException();
        }
    }

    private Resource makeResource(S3Object object) {
        return new InputStreamResource(object.getObjectContent());
    }
}

그 결과 테스트 코드에서도 resolveKey(..)를 떼어버릴 수 있게 되었습니다.
사실 제가 테스트하려고 하는 건 AWS API를 정상적으로 호출하는지 정도일 뿐,
filename으로 key를 잘 만드는지는 테스트의 관심사가 아니었거든요.
즉 아무 키나 받아서 써도 테스트의 맥락에는 영향을 주지 않습니다.

def "GetResource: 정상 요청"() {
    given:
    keyResolver.resolveKey(filename) >> key
    amazonS3.doesObjectExist(bucketName, key) >> true
    def stream = new ByteArrayInputStream(content.getBytes())
    def s3Object = Mock(S3Object.class) {
        getObjectContent() >> new S3ObjectInputStream(stream, null)
    }

    when:
    def resource = s3StorageService.getResource(filename)

    then:
    1 * amazonS3.getObject(bucketName, key) >> s3Object
    resource.getInputStream().getText() == content

    where:
    filename      | key          | content
    "sample-file" | "sample-key" | "Sample Data"
}

그래도 아직 테스트 코드는 복잡한데요.
다음으로 makeResource(..)를 S3Resource라는 새로운 클래스를 만들어 분리해 보겠습니다.

public class S3Resource extends InputStreamResource {
    public S3Resource(S3Object object) {
        super(object.getObjectContent());
    }
}

@Service
@RequiredArgsConstructor
public class S3StorageService implements StorageService {
    private final String bucketName;
    private final AmazonS3 amazonS3;
    private final S3KeyResolver keyResolver;

    @Override
    public Resource getResource(String filename) {
        String key = keyResolver.resolveKey(filename);
        S3Object s3Object = getS3Object(key);
        return new S3Resource(s3Object);
    }

    private S3Object getS3Object(String key) {
        checkIfObjectExists(key);
        return amazonS3.getObject(bucketName, key);
    }

    private void checkIfObjectExists(String key) {
        if (!amazonS3.doesObjectExist(bucketName, key)) {
            throw new FileNotFoundException();
        }
    }
}

S3Resource는 S3Object에서 InputStream을 꺼내와서, Resource에 담아 돌려주는 책임만 맡습니다.
그 결과 해당 부분을 테스트하는 코드를 별도의 테스트 클래스로 떼어낼 수 있게 됩니다.

class S3ResourceTest extends Specification {
    def "S3Object의 내용을 정상적으로 가져옴"() {
        given:
        def stream = new ByteArrayInputStream(content.getBytes())
        def s3Object = Mock(S3Object) {
            getObjectContent() >> new S3ObjectInputStream(stream, null)
        }

        when:
        def resource = new S3Resource(s3Object)

        then:
        resource.inputStream.text == content

        where:
        content       | _
        "Sample Data" | _
    }
}

생각해 보니 두 개로 나눌 수 있는 테스트를 하나의 함수 안에서 진행했다면,
테스트 함수가 필연적으로 복잡해질 수밖에 없었을 것 같네요.

<클린 코드>에서는 단위 테스트를 작성할 때 활용할 수 있는 지침을 여럿 제시하고 있는데요.
아래의 규칙도 그 중 일부입니다. (p.167)

함수 하나가 개념 하나만 테스트하도록 한다.
개념 당 assert문의 수는 최소로 줄인다.

좀 전의 테스트 함수를 규칙에 부합하는 방향으로 개선하면서,
명확한 의도를 갖는 테스트 함수를 하나 더 만들 수 있었습니다.

원래 테스트는 반쪽이 되었고요. 🙂
given절이 눈에 띄게 짧아졌고, AWS API 호출 여부만 명확하게 테스트하고 있네요.

def "GetResource: S3에서 객체를 가져옴"() {
    given:
    keyResolver.resolveKey(filename) >> key
    amazonS3.doesObjectExist(bucketName, key) >> true

    when:
    s3StorageService.getResource(filename)

    then:
    1 * amazonS3.getObject(bucketName, key) >> Stub(S3Object)

    where:
    filename      | key
    "sample-file" | "sample-key"
}

또한 응집도를 유지하면서 큰 함수를 쪼개다 보니, 작은 클래스 여러 개를 만들 수 있었습니다.
그 결과 클래스들이 책임을 나눠 가질 수 있게 되었는데요. (p.176)

KeyResolver
- 전달받은 filename에서 key 만들기
S3StorageService
- S3 버킷에 해당 key를 가진 객체가 존재하는지 확인하고, 객체 가져오기
S3Resource
- S3Object에서 InputStream을 꺼내와서, Resource에 담아 돌려주기

이렇듯 클래스가 하나의 책임을 가지면 개별 테스트를 만들기도 쉬워지고,
클래스가 가진 비즈니스 로직을 교체하거나 확장하는 것도 쉬워집니다.

가령 key를 만들기 위해 암호화를 적용해야 한다는 요구사항이 주어지면,
기존의 코드에서는 속절없이 S3StorageService에 암호화 관련 의존성을 추가해야 하겠죠.
만들어야 할 Mock 객체도 늘어나고 테스트도 필연적으로 복잡해지게 될 것입니다.

반면 지금의 코드에서는 KeyResolver를 구현하는 새 클래스를 만들어 S3StorageService와 결합시키면 됩니다.
전보다 적은 코드를 수정하면서 버그가 발생할 가능성을 낮출 수 있습니다.

경계 분리하기

이번에는 조금 더 복잡한 코드를 가져와 보았습니다.
아래 Analyzer 클래스는 문자열에서 금칙어를 찾아 대체어로 치환하는 로직을 구현한 클래스인데요.
물론 훌륭하게 작동하지만, 자세히 읽기보다는 테스트를 만들어야 한다고 생각하고 훑어 보아 주세요.
morpheme(..) 함수를 테스트하려고 하면 어떤 문제가 발생할까요?

public class Analyzer {

    private static final String BLANK = "\s";
    private final String originalText;
    private String changedText;
    private boolean isFine;

    private Analyzer(String originalText) {
        this.originalText = originalText;
        this.changedText = originalText;
        this.isFine = true;
    }

    public static Analyzer create(String text) {
        return new Analyzer(text);
    }

    [...]

    public Analyzer morpheme(Map<String, String> bannedWords) {
        StringBuilder tmpText = new StringBuilder();
        int startIndex = 0;
        boolean isChanged = false;

        Seq<KoreanTokenizer.KoreanToken> tokenSeqs = TwitterKoreanProcessorJava.tokenize(changedText);
        List<KoreanTokenJava> tokens = TwitterKoreanProcessorJava.tokensToJavaKoreanTokenList(tokenSeqs);
        tokens.sort(comparingInt(KoreanTokenJava::getOffset));

        for (KoreanTokenJava token : tokens) {
            if (EnumSet.of(KoreanPosJava.Noun, KoreanPosJava.NounPrefix).contains(token.getPos())) {
                String find = bannedWords.get(token.getText());
                if (find != null) {
                    isChanged = true;
                }
            }

            if (isChanged) {
                String prefix = changedText.substring(startIndex, token.getOffset());
                String replace = bannedWords.get(token.getText());
                tmpText.append(prefix).append(replace);
                isChanged = false;
            } else {
                tmpText.append(changedText, startIndex, token.getOffset() + token.getLength());
            }

            startIndex = token.getOffset() + token.getLength();
        }

        changedText = tmpText.toString();

        return this;
    }

    public Result result() {
        isFine = originalText.equalsIgnoreCase(changedText);
        return isFine ? Result.ofFine() : Result.of(false, originalText, changedText);
    }
}

우선 TwitterKoreanProcessorJava 없이 morpheme(..) 함수를 테스트할 방법이 없습니다.
야속하게도 morpheme(..)이 TwitterKoreanProcessorJava의 static 메서드를 열심히 호출하고 있어,
이 로직을 바꾸거나 호출을 가로챌 수 있는 방법이 마땅히 없습니다.

이 상태로 테스트를 만드려면, twitter-korean-text가 정확하게 토큰화할 수 있는 문장을 준비해줘야 합니다.
만약 형태소 분석기를 은전한닢으로 바꾸면 테스트가 실패할 수도 있겠죠…?

그리고 혹시, 조금 전 코드에서 Seq를 보셨나요?

Seq<KoreanTokenizer.KoreanToken> tokenSeqs = TwitterKoreanProcessorJava.tokenize(changedText);

Seq는 Scala 표준 라이브러리의 일부입니다.
twitter-korean-text가 Scala로 쓰여져 있다보니 상상도 하지 못한 의존성이 묻어오게 되었습니다.

Scala의 Seq 인터페이스가 변경된다고 해서 우리 코드가 영향을 받기를 원하는 사람은 없겠죠.
누군가 morpheme(..) 함수를 수정하다 tokenSeqs의 특정 메서드를 사용하기라도 하면…
우리 코드는 Scala 표준 라이브러리에 종속되게 됩니다.
그리고 Seq 인터페이스에 변화가 일어난다면…

smh

이처럼 외부 라이브러리를 사용할 때는 경계 인터페이스를 노출하지 않는 것이 좋습니다. (p.146)
그렇지 않으면 언제 바뀔지 모르는 외부 코드에 대한 의존성이 증가하고,
정작 중요한 코드, 즉 비즈니스 로직이 담긴 코드를 테스트하는 것이 어려워집니다.

다행히 위의 코드는 초안이었고, 몇 번의 수정을 거쳐 아래와 같은 코드로 바꿀 수 있었는데요.
의존성 주입이 편하도록 해당 함수를 MorphemeAnalyzer라는 클래스로 분리했고,
토큰화 관련 로직은 모두 Tokenizer 인터페이스에 의존하도록 했습니다.

public class MorphemeAnalyzer implements Analyzer {
    private Map<String, String> dictionary;
    private Tokenizer tokenizer;

    public MorphemeAnalyzer(Map<String, String> dictionary, Tokenizer tokenizer) {
        this.dictionary = dictionary;
        this.tokenizer = tokenizer;
    }

    @Override
    public String analyze(String text) {
        List<Token> tokens = tokenizer.tokenize(text);

        for (Token token : tokens) {
            if (token.isNounLike()) {
                String originalText = token.getText();
                String replacement = dictionary.getOrDefault(originalText, originalText);
                token.setText(replacement);
            }
        }

        return tokenizer.detokenize(tokens);
    }
}

public interface Tokenizer {
    List<Token> tokenize(String text);
    String detokenize(List<Token> tokens);
}

그리고 해당 Tokenizer 인터페이스를 구현한 TwitterKoreanTokenizer를 만들었습니다.
twitter-korean-text와 관련된 의존은 여기에서만 일어나고, 다른 곳에 옮겨붙지 않습니다.

public class TwitterKoreanTokenizer implements Tokenizer {
    @Override
    public List<Token> tokenize(String text) {
        Seq<KoreanToken> tokenSeqs = TwitterKoreanProcessorJava.tokenize(text);
        List<KoreanTokenJava> tokens = TwitterKoreanProcessorJava.tokensToJavaKoreanTokenList(tokenSeqs, true);
        tokens.sort(comparingInt(KoreanTokenJava::getOffset));
        return tokens.stream()
                     .map(this::convertToToken)
                     .collect(toList());
    }

    @Override
    public String detokenize(List<Token> tokens) {
        return tokens.stream()
                     .map(Token::getText)
                     .collect(joining());
    }

    [...]
}

만약 MorphemeAnalyzer의 단위 테스트가 필요하다면 tokenizer만 Mock 객체 등으로 갈아끼워주면 되겠죠.
이렇듯 경계를 나누면 설계가 유연해지고, 불확실한 외부 코드에 의존하는 위험이 줄어듭니다.
책에서는 이렇게 표현하고 있네요.

통제가 불가능한 외부 패키지에 의존하는 대신 통제가 가능한 우리 코드에 의존하는 편이 훨씬 좋다. (p.152)

마무리하며

저는 이 책을 몇 번에 걸쳐서 나눠 읽으면서 팀 내에서 리뷰를 받았었습니다.
이제 기술 블로그에 글을 남기면서 길었던 1회독(!)을 마무리하려고 하는데요.

리뷰 때마다 "동료 개발자들에게는 당연한 얘기를 길게 늘어놓는 게 아닌가?"라는 고민을 자주 했었습니다.
글을 쓰는 지금도 이런 고민을 하지만, 나름 용기를 내어 마무리를 짓는 이유는…

이게 다 좋은 얘기들인데, 우리는 꽤 오래 전에 이런 지식을 접하다 보니 더 이상 와닿지 않고, 자주 잊어버리곤 해요.
그때마다 지산님이 "이건 아니죠"라고 우리에게 지적해 준다면, 우리도 반성할 수 있어서 좋을 것 같아요.

라고 해주신 팀원분이 있었기 때문입니다. 팀 내 리뷰에서 들었던 피드백인데요.
말씀처럼 앎이 실천으로 이어지는 건 참 어렵다는 것을 자주 느낍니다.

가령 테스트만 해도 모두가 짜야 한다고 생각하지만, 코드를 보다 보면 여러 이유로 @Ignore되거나,
then절이 주석 처리된 상태로 방치된 테스트 함수들을 만날 때가 있거든요. 그래서…

지속적인 개선이야말로 전문가 정신의 본질이 아니던가? (p.19)

이 말을 빌려, 지속적인 환기 또한 전문가 집단 사이에서의 중요한 가치가 아닐까 생각해 봅니다.
그런 의미에서 이 글도 환기의 일환으로, 귀엽게 봐주시면 감사하겠습니다. 🙂

bow