생성자 내부에서 너무 많은 일을 하지 말자

들어가며

나도 예전에 생성자 내부에서 이리저리 지지고 볶다보니 문제를 겪은 적이 많다.

단적인 예로 외부 통신하는 RestTemplate을 wrapping하는 클래스를 만들었었는데

생성자 내부에서 RestTemplate을 만들다 보니 테스트하기가 너무 힘든 구조가 됐었다.

 

이때 피부로 느끼고 깨달음을 얻었던 부분이 멤버변수를 생성하기 위한 로직을 생성자 내부에서 만들지 말자는 것이다.

생성자에서 뭔가 멤버변수를 생성하는 로직이 너무 많으면 이를 제어하고 테스트하기 너무 힘들어진다.

멤버변수 생성에 대한 로직이 필요하다면 별도의 Factory를 두는것이 더 적절하다고 본다.

 

그리고 부가적인 이점이 있는게 리팩터링 내성이 올라간다.

리팩터링 내성이라 함은 작성해둔 테스트 코드가 운영코드의 리팩터링이 발생했음에도 컴파일 에러와 같은 상황이 발생하지 않아

빠르게 재사용이 가능함을 말한다.

 

예제 1

간단하게 그때 상황을 더듬어 재연해보도록 하자~

RestTemplate은 외부와 통신을 하기 위한 통신 구현체중 하나로 RestTemplate의 메서드를 활용해 외부 server와 통신을 주고받는다.

외부와 통신을 할 때 비즈니스에 fit하게 사용하기 위해 한번더 wrap한 뒤 사용하는것이 일반적인것 같다.

 

따라서 아래와 같이 RestTemplate을 감싼 클래스를 생각했다고 하자.

public class BankRestTemplate {
    private final RestTemplate restTemplate;
    
...
 
}

 

RestTemplate에는 여러가지 설정이 필요하다. 외부랑 통신을 하는 녀석이다 보니 각종 timeout 설정이 필요하기 마련인데

문제가 됐던 코드는 아래와 같이 작성했었다.

public class BankRestTemplate {
    private final RestTemplate restTemplate;

    public BankRestTemplate(Duration connectionTimeout, Duration readTimeout) {
        RestTemplateBuilder builder = new RestTemplateBuilder();
        builder.setConnectTimeout(connectionTimeout);
        builder.setReadTimeout(readTimeout);
        this.restTemplate = builder.build();
    }
    
    public boolean register(String accountNumber) {
        try {
            String result = restTemplate.postForObject("/bank/register", accountNumber, String.class);
            return "success".equals(result);
        } catch (Exception e) {
            return false;
        }
    }
}

100퍼센트 그때와 동일하지는 않지만 대충 결은 비슷하다. 핵심은 RestTemplate을 주입받는게 아니라 내부에서 만들어버리고 있다는것이다.

 

그래서 이렇게 만들어서 사용하는게 뭐가 어떻느냐? 라고 생각할 수 있는데 사실 사용하는데는 큰 문제 없다.

대신 우리는 안정성을 확보해야하지 않는가? 그말은 BankRestTemplate의 register 대한 단위 테스트가 필요하다는 것이다.

 

그러나 지금 같이 작성된 BankRestTemplate는 단위 테스트를 수행하기 힘들다.

가령 계좌 등록에 성공하는 테스트 코드를 작성한다면 아래와 같은 뼈대가 먼저 작성이 될 것 같다.

class BankRestTemplateUnitTest {

    @DisplayName("은행계좌 등록성공")
    @Test
    void test_11() {
        // given
        BankRestTemplate bankRestTemplate = new BankRestTemplate(Duration.ofSeconds(1), Duration.ofSeconds(1));

        // when
        boolean result = bankRestTemplate.register("1234567890");

        // then
        assertThat(result).isTrue();
    }
}

 

이 테스트를 성공으로 이끌이 위해서는 RestTemplate이 "success"라는 응답을 내려주도록 강제해야한다.

그런데 RestTemplate은 우리의 손을 벗어났다.

왜? 생성자 내부에서 생성하고 있기 때문이다.

 

그래서 stubbing을 할수도 mockking을 할수도 없는 난처한 상황이 벌어지게 된다.

따라서 이를 해결하기 위해서는 BankRestTemplate과 같은 클래스의 생성자 내부에서 제어대상이 되는 멤버변수를 직접 생성하기 보다 외부에서 주입받는 것이 해결책이 된다.

 

아래와 같이 BankRestTemplate의 생성자 구문을 변경하고

public class BankRestTemplate {
    private final RestTemplate restTemplate;

    public BankRestTemplate(RestTemplate restTemplate) {
        this.restTemplate = restTemplate;
    }

    ...
}

 

기존 생성자에서 수행하던 로직은 별도의 factory로 분리한다.

public class RestTemplateFactory {
    public static RestTemplate create(Duration connectionTimeout, Duration readTimeout) {
        RestTemplateBuilder builder = new RestTemplateBuilder();
        builder.setConnectTimeout(connectionTimeout);
        builder.setReadTimeout(readTimeout);
        return builder.build();
    }
}

 

그리고 테스트 코드는 다음과 같이 수정하여 테스트를 수행할 수 있게 된다.

class BankRestTemplateUnitTest {

    @DisplayName("은행계좌 등록성공")
    @Test
    void test_11() {
        // given
        RestTemplate mockRestTemplate = Mockito.mock(RestTemplate.class);
        BankRestTemplate bankRestTemplate = new BankRestTemplate(mockRestTemplate);

        Mockito.when(mockRestTemplate.postForObject("/bank/register", "1234567890", String.class)).thenReturn("success");

        // when
        boolean result = bankRestTemplate.register("1234567890");

        // then
        assertThat(result).isTrue();
    }
}

 

위와 같이 성공에 대한 응답을 만들수 있고 테스트할 수 있게되었으니 마찬가지로 실패에 대한 테스트도 손쉽게 제어할 수 있을 것이다.

 

RestTemplate은 Spring에서 제공중인 객체라 조금 어색할 수 있을지 모르니 다른 예제로도 한번 더 이야기를 해보자

 

예제 2

사실 예제 1은 내 경험에 빗대어 설명한거고 예제 2에서는 내가 코드리뷰하며 많이 본 케이스에 대해 이야기 하고자 한다.

지금부터 다루는 케이스가 리팩터링 내성에 더 근접한 예제가 될 것 같다.

 

로또 미션을 하다보면 엄청 많이 보는 케이스인데

일련의 통계를 생성하는 코드를 작성할때 특히 많이 보게 되는 것 같다.

쉽고 간단한 예제로 설명하는게 좋으니 로또 애플리케이션을 예로 들어보자.

 

여기서 설명하고자 하는 요구사항은 다음과 같다.

1. 사용자가 뽑은 로또 티켓이 있다.
2. 당첨번호가 적힌 로또 티켓이 있다.
3. 사용자의 당첨결과를 만들어야한다.

먼저 로또 티켓과 당첨번호가 담긴 로또 티켓은 다음과 같이 만들어졌다고 가정하자.

public class LottoTicket {
    private final List<Integer> lottoNumbers;
    
    public LottoTicket(List<Integer> lottoNumbers) {
        this.lottoNumbers = lottoNumbers;
    }
}

public class WinningLottoTicket {
    public WinningLottoTicket(List<Integer> lottoNumbers, int bonusNumber) {
        this.lottoNumbers = lottoNumbers;
        this.bonusNumber = bonusNumber;
    }

    private final List<Integer> lottoNumbers;
    private final int bonusNumber;
}

 

 

> 로또 티켓에 적힌 번호가 값 객체가 아닌것부터 눈에 거슬리는 점이 몇가지 보일텐데 그냥 그러려니하고 일단 넘어가자.

 

이제 두개의 티켓을 비교해 결과를 산출해야한다.

몇개 맞은 티켓이 몇개 있는지!

 

이때 이렇게 작성하는 사람들을 많이 보았다.

public class LottoResult {
    private final Map<Integer, Integer> result;

    public LottoResult(List<LottoTicket> tickets, WinningLottoTicket winningLottoTicket) {
        this.result = this.calculateResult(tickets, winningLottoTicket);
    }
    
    private Map<Integer, Integer> calculateResult(List<LottoTicket> tickets, WinningLottoTicket winningLottoTicket) {
        // 여기서 기똥차게 로직을 적었다고 하자~
    }
}

이렇게 코드를 작성하고 나면 테스트 코드는 아래와 같이 작성하게 된다.

 

class LottoResultTest {
    @Test
    void test_10() {
        //given
        List<LottoTicket> tickets = Arrays.asList(
            new LottoTicket(Arrays.asList(1, 2, 3, 4, 5, 6)),
            new LottoTicket(Arrays.asList(10, 11, 12, 13, 14, 15))
        );

        WinningLottoTicket winningLottoTicket = new WinningLottoTicket(Arrays.asList(1, 2, 3, 4, 5, 6), 7);

        //when
        LottoResult lottoResult = new LottoResult(tickets, winningLottoTicket);

        //then
        // 여기서 결과에 대한 검증을 한다 치자
    }
}

엄청 큰 문제가 있어보이지는 않지만 다음과 같은 문제가 내포되어 있다.

 

구현 세부사항이 테스트 코드에 너무 노출되어있다.

우리가 관심있게 보는 대상은 LottoResult이다. 그러나 이를 생성하기 위해서 LottoTicket에 대해서도 알아야하고 WinningLottoTikcet에 대해서도 알아야 한다.

 

LottoResult를 만드는 개발자가 LottoTicket과 WinningLottoTicket을 작성한 사람이 아니라면 

LottoTicket에 validate하는 정책과 WinningLottoTicket의 validate하는 정책까지 빠삭하게 알고 각각의 객체를 생성해야한다.

지금은 협력에 참여하는 객체가 두개지만 세개, 아니 네개가 된다면? 해당 객체들의 정책까지 다알고 생성해야한다.

다시말해 피로도가 급격히 올라가는 포인트가 될 수 있다.

 

리팩터링 내성이 좋은 코드라고 보기 힘들다.

WinningLottoTicket을 생성하기에 여러가지 변화가 생긴다면 어떻게 될것 인가?

더 이상 bonus 번호를 허용하지 않는다는 정책의 변화가 생긴다면?

WinningLottoTicket에게서 bonusNumber가 사라지게 될것이고 코드의 제거로 인해 테스트 코드에서도 compile 에러가 발생한다.

 

 

생성자에 로직을 넣지 말고 분리하자

따라서 아래와 같이 생성에 대한 부분을 분리하는 것이 더 좋다고 본다.

public class WinningLottoTicket {
    public WinningLottoTicket(List<Integer> lottoNumbers, int bonusNumber) {
        this.lottoNumbers = lottoNumbers;
        this.bonusNumber = bonusNumber;
    }

    private final List<Integer> lottoNumbers;
    private final int bonusNumber;
    
    // new
    public LottoResult compareWith(List<LottoTicket> lottoTickets) {
        // LottoResult의 생성자에서 호출하던 로직
    }    
}

public class LottoResult {
    private final Map<Integer, Integer> result;

    // changed
    public LottoResult(Map<Integer, Integer> result) {
        this.result = result;
    }
}

 

이렇게 변경한다면 LottoResult를 생성하기 위해 다른 협력객체에 대해 알아야 할 필요가 줄어들었고 리팩터링 내성도 올라간다.