[우아한 테크코스 3기] LEVEL 1 회고 - 로또 1단계 미션의 1차 피드백을 받아보다 (19일차)

안녕하세요? 제이온입니다.

 

오늘은 기다리던 로또 미션의 1차 피드백을 받아 보았습니다. 그 외에 독서도 하기는 하였지만 메인은 역시 코드 리뷰 내용이겠죠? 제가 피드백 반영한 부분을 위주로 말씀드리겠습니다.

 

 

1차 피드백

이번 리뷰어 님은 '데이브'였습니다. 주말까지 이렇게 열심히 코드리뷰를 해 주셔서 정말 감사합니다.

 

 

 

 

이렇게 먼저 객체 분리하려고 노력하였다고 칭찬해 주셔서 기분이 좋았습니다 ㅎㅎ. 이번 코드는 최대한 객체를 분리하고 역할을 분배하기 위해서 많이 신경을 썼습니다.

 

지금부터 피드백 내용과 함께 제가 생각한 부분을 말씀드리겠습니다. 제 걱정과는 달리 막 많은 코드에서 지적을 받지는 않았습니다.

 

 

 

 

첫 번째는 메소드명 수정입니다. 위 코드는 구입금액을 입력받는 것이므로 getInt()보다는 inputMoney()와 같이 돈의 의미를 살려주라는 것이었죠. 하지만.. 제가 Int()라고 지은 것은 이유가 있습니다.

 

그것은 바로 입력 문구때문인데요, 돈을 입력받을 때와 보너스 볼을 입력받을 때의 출력되어야 할 문구가 서로 다르지만, 기본적으로 정수를 입력받아야 하는 '기능'은 동일합니다. 그래서 InputView에서 입력 문구때문에 getMoney()와 getBonus()로 분리하여 중복된 코드는 쓰는 것은 바람직하지 않아보여서 Int()로 뭉뚱그렸습니다.

 

이 부분은 데이브의 생각을 들어보고 더 나은 방향으로 개선하려고 합니다.

 

 

 

 

두 번째는 도메인에서 View가 출력할 포맷을 만들지 말라는 것입니다. 이 부분은 페어와 고민한 결과 뷰는 모델의 존재를 몰라야한다고 생각하여, 아래와 같이 코드를 작성하였습니다.

 

 

    public String ratingToString(final Rating rating, final int count) {
        if (rating == Rating.SECOND) {
            return String
                .format(SECOND_PRINT_FORMAT, rating.getMatchCount(), rating.getReward(), count);
        }
        return String
            .format(PRINT_FORMAT, rating.getMatchCount(), rating.getReward(), count);
    }

 

 

그런데, 리뷰어 님 말씀대로 만약에 출력 문구가 바뀐다면 View가 아니라 도메인에서 바꿔야하는 문제가 생깁니다. 그래서 어떻게 해야할지 찾다보니까 'DTO'라는 것을 활용해야한다고 합니다. 하지만... 저는 DTO가 완전히 처음이었고, 오늘 개념 정도만 어렴풋이 익힐 수 있었습니다. 제가 알아본 결과, 짧게 말하면 DTO는 가변 객체이되, getterr 또는 setter만 허용하는 객체라고 합니다.

 

그래서 이 내용은 리뷰어 님과 이야기하면서 어떻게 개선하는 것이 좋을지 조언을 구하려고 합니다.

 

 

 

 

세 번째는 생성자 오버로딩이 불가능한 경우 정적 팩토리 메소드 패턴을 사용한다는 것입니다. 먼저, 생성자 오버로딩이 불가능한 코드를 보겠습니다.

 

    public Lotto(final List<LottoNumber> numbers) {
        validateNumberCount(numbers);
        validateDistinct(numbers);
        this.numbers = numbers;
    }

    public Lotto(final List<Integer> numbers) {
        validateNumberCount(numbers);
        validateDistinct(numbers);
        this.numbers = numbers;
    }

 

 

이렇게 생성자를 두 개 만들면, List의 제네릭 타입이 다르므로 오버로딩이 가능하지 않을까 생각할 수 있습니다. 하지만 이것은 'Lotto(List<LottoNumber>)' clashes with 'Lotto(List<Integer>)'; both methods have same erasure' 컴파일 에러가 발생합니다. 따라서 다른 방식을 사용해야 합니다.

 

저는 dummy라는 boolean 타입 변수를 임의로 넣어줌으로써 오버로딩 문제를 해결하였는데, 이것보다는 따로 생성자를 만드는 정적 메소드를 정의하는 것이 낫다고 합니다.

 

 

    public static Lotto getInstanceByInt(final List<Integer> numbers) {
        return new Lotto(numbers.stream()
            .map(LottoNumber::new)
            .collect(Collectors.toList()));
    }

    public Lotto(final List<LottoNumber> numbers) {
        validateNumberCount(numbers);
        validateDistinct(numbers);
        this.numbers = numbers;
    }

 

 

위와 같이 List<Integer> 타입은 정적 메소드로 객체를 생성해 주는 것이죠. 자세한 정적 팩토리 메소드 패턴은 이곳을 참고하시길 바랍니다.

 

 

 

 

네 번째는 stream의 가독성 문제입니다. 위와 같이 한 줄로 쭉 다 쓰는 것보다는 분리를 하는 것이 가독성이 좋습니다. 원래 띄어서 쓰는 편이었는데 제가 실수로 쭉 나열해 버렸군요. 바로 수정하였습니다.

 

 

 

 

다섯 번째는 불필요한 요소 복사입니다. 이 코드는 원래 numbers 리스트를 오름차순 정렬하여 반환하는 것이어서 numbers 원본 리스트의 요소 순서가 바뀌는 것을 고려하여 다른 리스트에 copy를 하였습니다.

 

하지만, 다시 생각해보니 화면에 출력할 때만 오름차순으로 정렬하면 되므로 굳이 getNumbers()에서 정렬할 필요는 없다고 느꼈습니다. 그래서 위 객체도 지우고 정렬도 안 하는 방향으로 고쳤습니다.

 

 

 

 

여섯 번째는 모호한 LottoManager의 역할입니다. 우선, 제가 작성한 LottoManager 클래스의 코드를 보겠습니다.

 

 

public class LottoManager {

    private final RatingInfo ratingInfo = new RatingInfo();
    private final LottoRepository lottoRepository = new LottoRepository();
    private LottoMachine lottoMachine;

    public LottoManager(final LottoMachine lottoMachine) {
        this.lottoMachine = lottoMachine;
    }

    public LottoRepository buyLotto(final Ticket ticket) {
        lottoRepository.generateLottoByTicket(lottoMachine, ticket.getCount());
        return lottoRepository;
    }

    public RatingInfo scratchLotto(WinningLotto winningLotto) {
        for (Lotto lotto : lottoRepository.toList()) {
            int match = winningLotto.compareLottoNumber(lotto);
            boolean hasBonusBall = winningLotto.compareBonusBall(lotto);
            ratingInfo.update(Rating.getRating(match, hasBonusBall));
        }
        return ratingInfo;
    }
}

 

 

로또를 구매하고 당첨 결과를 반환하는 두 가지 메소드가 있습니다. 그리고 LottoController에서 이 LottoManager 클래스를 사용합니다. LottoController 클래스도 보겠습니다.

 

 

public class LottoController {

    private static final String INPUT_MONEY_MESSAGE = "구입금액을 입력해 주세요.";
    private static final String INPUT_BONUS_BALL_MESSAGE = "보너스 볼을 입력해 주세요.";

    public void start() {
        LottoManager lottoManager = new LottoManager(new RandomLottoMachine());
        Ticket ticket = buyLotto();

        OutputView.printBuyLotto(ticket.getCount());
        OutputView.printLottoResults(lottoManager.buyLotto(ticket));

        WinningLotto winningLotto = buyWinningLotto();
        RatingInfo ratingInfo = lottoManager.scratchLotto(winningLotto);
        OutputView.printWinningStats(new LottoStatistics(ratingInfo), ticket.getPrice());
    }

    private Ticket buyLotto() {
        try {
            return tryBuyLotto();
        } catch (IllegalArgumentException e) {
            OutputView.getMessage(e.getMessage());
            return buyLotto();
        }
    }

    private Ticket tryBuyLotto() {
        OutputView.getMessage(INPUT_MONEY_MESSAGE);
        int money = InputView.getInt();
        return new Ticket(new Money(money));
    }

    private WinningLotto buyWinningLotto() {
        try {
            return tryBuyWinningLotto();
        } catch (IllegalArgumentException e) {
            OutputView.getMessage(e.getMessage());
            return buyWinningLotto();
        }
    }

    private WinningLotto tryBuyWinningLotto() {
        Lotto lotto = Lotto.getInstanceByInt(InputView.getWinningNumbers());
        OutputView.getMessage(INPUT_BONUS_BALL_MESSAGE);
        LottoNumber bonusNumber = new LottoNumber(InputView.getInt());
        return new WinningLotto(lotto, bonusNumber);
    }
}

 

 

LottoController는 View만 추가로 사용하는 것이지, LottoManager의 역할과 겹친다는 기분이 듭니다. 이 부분도 더 나은 방향으로 설계하기위해서 데이브와 소통하려고 합니다.

 

 

 

 

마지막으로 패키지 분리입니다. 저는 유사한 기능 별로 패키지를 묶어서 클래스를 관리하였는데, LottoNumber, Money, Ticket를 둘 데가 마땅치 않아서 primitive라는 패키지에 몰아넣어놨었습니다.

 

그런데 원시 값 포장 전 해당 값을 사용하는 도메인과 함께 있으면 좋겠다는 의견을 듣고, LottoNumber는 lotto 패키지에, Money와 Ticket는 manager 패키지에 LottoManger와 함께 넣었습니다. 하지만.. LottoManager가 Money와 Ticket를 쓰는 것은 맞지만 이 둘이 같이 묶는 것이 바람직한 것인지는 의문이 듭니다.

 

 

정리

전체적으로 크게 지적받은 코드는 없어서 만족스럽지만, 위에서 언급한 3가지가 여전히 찜찜합니다. 특히, DTO의 필요성을 느꼈지만 정확히 어떻게 이용하는지는 여전히 어렵습니다.. 이 부분을 중점적으로 공부하여 어떻게든 피드백을 반영해 보아야겠습니다.

 

위 미션의 코드가 궁금하신 분은 이곳을 참고하시길 바랍니다.