로또 - TDD 회고

Step1 문자열 계산기

Step1 문자열 계산기 PR

 

미션1의 계산기와 비슷한 요구사항이여서 쉽게 진행했다.

 

import java.util.regex.Pattern;
import java.util.stream.Collectors;

public class Input {

 

Input은 validation과 parsing을 수행하고 있는 객체다.

 

아래와 같은 피드백을 받았다.

💡 validation과 parsing을 수행하고 있습니다. Input 보다는 더 잘 표현할만한 명칭은 없을까요?

 

 

입력 값 검증만 했다면 -Validator 등을 붙혔겠지만 parsing 역할도 있어서 범용적인 클래스명을 붙히는게 좋다는 생각이 들었다. 그래서 클래스명을 CalculatorInputProcessor 로 수정하였다.

 

public enum Operation {
    PLUS("+", (x, y) -> x + y),
    MINUS("-", (x, y) -> x - y),
    TIMES("*", (x, y) -> x * y),
    DIVIDE("/", (x, y) -> x / y);

    private final String symbol;
    private final IntBinaryOperator operator;

 

사칙연산을 수행하는 역할을 enum에서 정의해서 하고 있다.

 

아래와 같은 피드백을 받았다.

💡 0으로 나뉘는 경우에 대해서는 별도의 처리가 필요하지 않을까요?

 

리뷰어님이 요구사항에 없는것도 지적해주셔서 감사하다.

    private static Map<String, Operation> stringToEnum = Stream.of(values())
            .collect(toMap(Operation::getSymbol, Function.identity()));

    public static Optional<Operation> fromString(String symbol) {
        return Optional.ofNullable(stringToEnum.get(symbol));
    }

 

 

나눗셈 enum을 통해 apply()를 호출할 때 0으로 나눌려고 하는지 체크하는 로직을 추가하였다.

 

아래와 같이 수정하였다.

public enum Operation {
    PLUS("+", (x, y) -> x + y),
    MINUS("-", (x, y) -> x - y),
    TIMES("*", (x, y) -> x * y),
    DIVIDE("/", (x, y) -> x / y);

    private final String symbol;
    private final IntBinaryOperator operator;
    
    //...

    public int apply(int x, int y) {
        validateDivideZero(y);
        return operator.applyAsInt(x, y);
    }

    private void validateDivideZero(int y) {
        if(this == DIVIDE && y == 0){
            throw new IllegalArgumentException("나눗셈은 0으로 나눌 수 없습니다.");
        }
    }

 

 

문자열로된 연산자를 받아서 해당하는 enum값을 반환해주는 메소드가 있다.

해당 메소드를 호출할때 마다 Map<문자열, enum> 변수로 만들어주는 로직이 있다.

    public static Optional<Operation> fromString(String symbol) {
        Map<String, Operation> stringToEnum = Stream.of(values())
                .collect(toMap(Operation::getSymbol, Function.identity()));

        return Optional.ofNullable(stringToEnum.get(symbol));
    }

 

아래와 같은 피드백을 받았다.

💡 매번 Map 을 생성하기보다는 상수화하여 사용하는게 시간 복잡도나 메모리 관리 차원에서 도움이 됩니다.

 

아래와 같이 수정하였다.

    private static Map<String, Operation> stringToEnum = Stream.of(values())
            .collect(toMap(Operation::getSymbol, Function.identity()));

    public static Optional<Operation> fromString(String symbol) {
        return Optional.ofNullable(stringToEnum.get(symbol));
    }

 

 

Step2 로또 자동

Step2 로또 자동

 

자동차 경주는 이전에도 많은 예시 코드를 본 적이 있어서 어렵진 않았지만 로또 부터는 요구사항이 복잡하여 어렵다는 느낌을 받았다.

 

public class InputValidator {

    private static final int MIN_PRICE = 1000;
    private static final int ZERO = 0;

    public void validatePurchasePrice(int price) {
    
    //..

 

상태 없이 기능만 하는 클래스를 사용시 new 키워드를 이용해 인스턴스롤 만들어서 사용 했다.

 

아래와 같은 피드백을 받았다.

💡 이 클래스와 LottoFactory 등 상태 없이 기능만 하는 클래스가 다소 있습니다. 별도의 인스턴스가 필요해보이지는 않네요.

static method는 방법 중 한가지고, singleton을 이용하는 방법도 있습니다 :)
스스로 판단 하에 괜찮다고 생각되는 방법을 사용해주시면 됩니다.

그리고 static method만을 사용하는 클래스라면 private 생성자로 추가 인스턴스 생성을 막아주시면 좋습니다.

 

 

그래서 모든 메소드에 static 키워드를 붙히고 private 생성자로 인스턴스 생성을 막았다.

public class InputValidator {

    private InputValidator() {
    }

    public static void validatePurchasePrice(int price) {
        //...
    }

 

 

public interface LottoGenerator {
    List<LottoNumber> lotto();
}

public class RandomLottoGenerator implements LottoGenerator {

    private static final Random random = new Random();
    private static final int MAX_NUMBER = 45;
    private static final int LOTTO_NUMBER_SIZE = 6;

    @Override
    public List<Integer> lotto() {
        return IntStream.generate(() -> random.nextInt(MAX_NUMBER) + 1)
                .distinct()
                .limit(LOTTO_NUMBER_SIZE)
                .boxed()
                .collect(Collectors.toList());
    }
}

 

LottoGenerator으로 로또 번호 생성을 인터페이스화 하였다. 

 

public class Lotto {
    private LottoNumbers lottoNumbers;

    public Lotto(LottoGenerator lottoGenerator) {
        this.lottoNumbers = new LottoNumbers(lottoGenerator);
    }
    
    //..

public class LottoNumbers {
    private static final int MIN_NUMBER = 1;
    private static final int MAX_NUMBER = 45;

    private final List<Integer> numbers;

    public LottoNumbers(LottoGenerator lottoGenerator) {
        List<Integer> numbers = lottoGenerator.lotto();
        validateNumberRange(numbers);
        this.numbers = numbers;
    }

 

Lotto -> LottoNumbers 관계에서 Lotto 생성시 LottoGenerator을 생성자 인자로 넘겨주고 그걸 다시 LottoNumbers의 생성자 인자로 넘겨주고 있었다.

 

아래와 같은 피드백을 받았다.

💡 Lotto 가 LottoGenerator를 의존하지 않아도 괜찮을 것 같은데 어떤가요?

 

public class Lotto {
    private LottoNumbers lottoNumbers;

    public Lotto(LottoNumbers lottoNumbers) {
        this.lottoNumbers = lottoNumbers;
    }

 

LottoGenerator 대신 LottoNumbers를 직접 넣어주는 구조로 변경했다. LottoNumbers은 외부에서 만들어진 상태로 주입을 받는 방법이다.

 

public class Lottos {
    private final Map<Integer, List<Lotto>> lottos;

    public Lottos(Map<Integer, List<Lotto>> lottos) {
        this.lottos = lottos;
    }

 

Lottos는 X개의 로또를 가지고 있는 일급 컬렉션이다.

 

아래와 같은 피드백을 받았다.

💡 Lottos를 선언한 위치에서 Map을 수정하면 이 클래스의 데이터도 변경됩니다.
방어적 복사 를 참조해주세요.

 

※ 방어적 복사

 

아래와 같이 수정하였다.

public class Lottos {
    private final Map<Integer, List<Lotto>> lottos;

    public Lottos(Map<Integer, List<Lotto>> lottos) {
        this.lottos = new HashMap(lottos);
    }

 

 

public class LottoNumbers {
    private static final int MIN_NUMBER = 1;
    private static final int MAX_NUMBER = 45;

    private final List<Integer> numbers;
    
    //..
    
    public long matchCount(List<Integer> winLottoNumbers) {
       return numbers.stream()
                .filter(winLottoNumbers::contains)
                .count();
    }

당첨 번호를 포장하지 않고 있었다.

 

아래와 같은 피드백을 받았다.

💡 당첨 번호도 하나의 책임으로 분리하는건 어떤가요?

 

당첨 번호를 가지고 있는 입글 컬렉션을 만들고 matchCount 로직을 해당 책임으로 옮겼다.

public class LottoWinNumbers {

    private List<Integer> winningLottoNumbers;

    public LottoWinNumbers(List<Integer> winningLottoNumbers) {
        this.winningLottoNumbers = new ArrayList<>(winningLottoNumbers);
    }

    public long matchCount(List<Integer> lottoNumbers) {
        return lottoNumbers.stream()
                .filter(winningLottoNumbers::contains)
                .count();
    }
}

 

 

 private void validateNumberRange(List<Integer> numbers) {
        boolean isNumberRange = numbers.stream()
                .allMatch(number -> number >= MIN_NUMBER && number <= MAX_NUMBER);

로또 숫자의 범위를 체크하는 로직이다.

 

아래와 같은 피드백을 받았다.

💡 람다로 구현되는 내용은 메소드로 빼주시는게 가독성 면에서 좋다고 생각합니다 :)

 

스트림 & 람다 자체를 private 메소드로 빼주었다.

    private void validateNumberRange(List<Integer> numbers) {
        if (!isNumberRange(numbers)) {
            throw new NotNumberRangeException();
        }
    }

    private boolean isNumberRange(List<Integer> numbers) {
        return numbers.stream()
                .allMatch(number -> number >= MIN_NUMBER && number <= MAX_NUMBER);
    }

 

지금 돌이켜 보면 allMatch의 람다를 private 메소드로 빼도 괜찮지 않았을까 싶다.

 

https://edu.nextstep.camp/

당첨 통계를 출력하는 요구 사항 있다.

3개 이상 일치부터는 개수 별 당첨 개수를 출력해야했다.

여기서 주의할 점은 당첨 개수가 없더라더 '0개'로 출력을 해야 했다.

 

그래서 Map에 미리 값을 넣어놨다.

public class Lottos {
    private final Map<Integer, List<Lotto>> lottos;

    public Lottos(Map<Integer, List<Lotto>> lottos) {
        this.lottos = lottos;
    }
    
     public Map<Long, Long> winCounts(List<Integer> winLottoNumbers) {
        Map<Long, Long> initLottoResults = Stream.of(3L, 4L, 5L, 6L)

 

 

아래와 같은 피드백을 받았다.

💡 왜 3 4 5 6일까요?

저는 enum을 보고 그 의도를 파악했어요.
3개 - a장, 4개 - b장, ... 을 표현하시려는 걸로 보이네요.

3L, 4L, 5L, 6L 은 상수로 포장하면 이해하기 더 쉬울 것 같습니다.

 

말씀하신 enum은 아래 코드이다.

public enum LottoPrice {
    THREE_MATCH(3, 5000),
    FOUR_MATCH(4, 50000),
    FIVE_MATCH(5, 1500000),
    SIX_MATCH(6, 2000000000);

 

말씀하신 대로 상수로 포장 하여 아래와 같이 수정을 하였다.

private final List<Long> LOTTO_MATCH_COUNTS = List.of(3L, 4L, 5L, 6L);

    private final Map<Integer, List<Lotto>> lottos;

    public Lottos(Map<Integer, List<Lotto>> lottos) {
        this.lottos = new HashMap<>(lottos);
    }

    public LottoResult winCounts(LottoWinNumbers lottoWinNumbers) {
        Map<Long, Long> initLottoResults = LOTTO_MATCH_COUNTS.stream()
                .collect(Collectors.toMap(matchCount -> matchCount, matchCount -> 0L));

 

 

로또 통계를 구하는 역할을 LottoStatistics에 부여했다.

rate 메소드에서 LottoRate를 생성하고 있었다.

public class LottoStatistics {

    public LottoRate rate(int price, List<LottoWinResult> lottoWinResults) {
        int lottoSum = lottoSum(lottoWinResults);
        double rate = Math.floor((double) lottoSum / price * 100) / 100.0;
        return new LottoRate(rate, isLowerStandard(rate));
    }
    
    //..

 

 

아래와 같은 피드백을 받았다.

💡 LottoRate 는 getter가 핵심 로직이라 봐도 될 정도로 데이터 위주의 클래스가 되어 있습니다.

이 메소드의 역할을 넘겨주는건 어떤가요?

 

 

LottoRate 로직은 아래와 같았다.

    
public class LottoRate {
    private static final double EPSILON = 0.0001;

    private double rate;
    private boolean isLowerStandard;

    public LottoRate(double rate, boolean isLowerStandard) {
        this.rate = rate;
        this.isLowerStandard = isLowerStandard;
    }

    public double getRate() {
        return rate;
    }

    public boolean isLowerStandard() {
        return isLowerStandard;
    }
    
    //..

 

말씀하신대로 데이터만 세팅 하고 getter 메소드만 있을뿐 로직이 없다.

추가로 LottoRate는 아래와 같은 피드백을 받았다.

 

💡  이 클래스의 이름은 로또 비율보다는 당첨 확률이 조금 더 어울리지 않을까요?

 

LottoRate -> LottoWinPercentage로 변경하였다.

 

LottoWinPercentae의 생성자에서 필요한 데이터를 받아서 해당 객체에서 자기 자신을 생성할 수 있도록 책임을 옮겼다.

public class LottoWinPercentage {
    private static final double EPSILON = 0.0001;

    private double rate;
    private boolean isLowerStandard;

    public LottoWinPercentage(int price, List<LottoWinResult> lottoWinResults) {
        this.rate = calculateRate(price, lottoWinResults);
        this.isLowerStandard = isLowerStandard(rate);
    }

 

 

public class LottoStatistics {

    private static final int LOTTO_MIN_MATCH_COUNT = 3;

    public List<LottoWinResult> statistics(Map<Long, Long> lottoResult) {

 

메소드의 인자로 생성된 로또를 넘기고 있었다. Map으로 키값은 당첨 기준, 값은 당첨 개수였다.

 

아래와 같은 피드백을 받았다.

💡  Map<Long, Long> 도 하나의 책임으로 묶을 수 있겠네요.

Map같은 경우 매개변수를 넘나들며 사용하면 코드를 이해하기 어렵다는 단점이 있습니다.
이전에 어느 경로부터 어떻게 세팅되어 넘어올지 파악하는데 시간이 걸리거든요.

 

위 내용을 듣고 회사에서 Map을 남발한 자신이 부끄러웠다고 했더니 아래와 같이 말씀 해주셨따.. (천사)

💡  아닙니다. 지금 잘 하고 계신걸요.

Map 위주의 로직이 많은게 코드를 못짰다고 말하기에는 서로 모르는 비지니스적인 상황이 너무나 많아요.
솔직히 Map 으로 개발하면 시간도 단축되고 편하니까요.
아니면 레거시 프로젝트라 전반적인 코드 스타일 자체가 아직 예전에 머무를 수도 있겠구요.

차차 개선해나가면 되는 일이죠 :)

 

아래와 같이 LottoResult 객체로 포장을 했다.

포장을 해보니 상태값을 이용해서 만들면 되서 LottoStatistics가 필요 없어져서 해당 클래스를 지우고 LottoResult로 책임을 옮겼다.

public class LottoResult {

    private static final int LOTTO_MIN_MATCH_COUNT = 3;

    private final Map<Long, Long> lottoResult;

    public LottoResult(Map<Long, Long> lottoResult) {
        this.lottoResult = lottoResult;
    }

    public List<LottoWinResult> lottoStatistics() {
        List<LottoWinResult> lottoResults = new ArrayList<>();
        lottoResult.entrySet().stream()
                .filter(entry -> entry.getKey() >= LOTTO_MIN_MATCH_COUNT)
                .forEach(entry -> lottoResults.add(new LottoWinResult(entry.getKey(), entry.getValue())));
        return lottoResults;
    }

    public Map<Long, Long> getLottoResult() {
        return Collections.unmodifiableMap(lottoResult);
    }
}

 

 

Step3 로또(2등)

Step3 로또(2등)

 

Step2에서 보너스 볼로 2등이 당첨되는 경우를 대비하지 못해 코드의 변화가 많이 되었다.

 

    @ParameterizedTest
    @MethodSource("provideMatchCountAndIsBonusAndLottoRank")
    @DisplayName("성공 - 당첨 개수와 보너스 볼에 일치하는 로또 등수를 반환한다.")
    void test(int matchCount, boolean isBonus, LottoRank lottoRank){
        assertThat(LottoRank.of(matchCount, isBonus)).isEqualTo(lottoRank);
    }

    private static Stream<Arguments> provideMatchCountAndIsBonusAndLottoRank() {
        return Stream.of(
                Arguments.of(3, false, LottoRank.FIFTH),
                Arguments.of(4, false, LottoRank.FOURTH),
                Arguments.of(5, false, LottoRank.THIRD),
                Arguments.of(5, true, LottoRank.SECOND),
                Arguments.of(6, false, LottoRank.FIRST)
        );
    }

 

당첨 개수와 보너스 볼에 따른 등수를 반환 하는 테스트 코드다.

💡 이 조건으로 테스트 실패하지 않나요? 보너스는 오직 2등일 때에만 영향을 끼쳐야 합니다.

 

말씀 하신 조건은 아래와 같다.

                Arguments.of(3, false, LottoRank.FIFTH),
                Arguments.of(3, true, LottoRank.FIFTH),
                Arguments.of(4, false, LottoRank.FOURTH),
                Arguments.of(4, true, LottoRank.FOURTH),

 

2등의 당첨 기준은 당첨 개수가 5개이며 보너스 볼이 당첨됬을때 이다.

하지만 LottoRank 에서는 아래와 같이 당첨 개수에 상관없이 보너스 볼을 검사하고 있어서 리뷰어님이 제공해 주신 테스트 케이스에서 실패가 난다.

public enum LottoRank {
    FIRST(6, 2_000_000_000, false),
    SECOND(5, 30_000_000, true),
    THIRD(5, 1_500_000, false),
    FOURTH(4, 50_000, false),
    FIFTH(3, 5_000, false),
    MISS(0, 0, false);

    private final int matchCount;
    private final int prizeAmount;
    private final boolean isBonus;

   //...

    public static LottoRank of(long countOfMatch, boolean matchBonus) {
        return Arrays.stream(LottoRank.values())
                .filter(lottoRank -> lottoRank.getMatchCount() == countOfMatch && lottoRank.isBonus == matchBonus)
                .findFirst()
                .orElse(MISS);
    }
    
    //...
    
}

 

 

그래서 아래와 같이 보너스볼 검사는 당첨 개수가 5개 일때만 검사하도록 로직을 수정하였다.

    public static LottoRank of(long countOfMatch, boolean matchBonus) {
        return Arrays.stream(LottoRank.values())
                .filter(lottoRank -> filterLottoRank(lottoRank, countOfMatch, matchBonus))
                .findFirst()
                .orElse(MISS);
    }

    private static boolean filterLottoRank(LottoRank lottoRank, long countOfMatch, boolean matchBonus) {
        if (countOfMatch == SECOND.getMatchCount()) {
            return lottoRank.getMatchCount() == countOfMatch && lottoRank.isBonus() == matchBonus;
        }
        return lottoRank.getMatchCount() == countOfMatch;
    }

 

 

로또 결과 통계를 아래와 같이 구하고 있었다.

public List<LottoWinResult> lottoStatistics() {
  List<LottoWinResult> lottoWinResults = new ArrayList<>();
  lottoResult.entrySet().stream()
    .filter(entry -> entry.getKey().getMatchCount() >= LOTTO_MIN_MATCH_COUNT)
    .forEach(entry -> lottoWinResults.add(new LottoWinResult(entry.getKey().getMatchCount(), entry.getValue(), entry.getKey().isBonus())));
    return lottoWinResults;
}

 

아래와 같은 피드백을 받았다.

💡 MISS 빼고는 전부 당첨이니 이런 조건이 낫지 않을까요?

 

말씀 하신 조건은 아래와 같다.

.filter(entry -> entry.getKey() != LottoRank.MISS)

 

이와 같은 피드백을 주신건 코드 가독성 측면에서 주신거지 않을까 싶다.

 

구매한 로또 개수만큼 Map에 담고 있었다.

public class Lottos {
    private final Map<Integer, List<Lotto>> lottos;

 

아래와 같은 피드백을 받았다.

💡 이 필드가 꼭 Map일 필요가 있나요? key를 활용하고 있지 않아요.

 

처음 의도는 로또 당첨 개수만큼 Key에 1부터 차례대로 증가하는 값이 담긴다.

하지만 로직에서 Key를 전혀 활용하고 있지 않았다, 그래서 Map 대신 List로 변경하였다.

 

public class Lottos {

  private final Map<Integer, List<Lotto>> lottos;

  public LottoResult winCounts(LottoWinNumbers lottoWinNumbers, BonusBall bonusBall) {
    Map<LottoRank, Long> lottoRanks = new EnumMap<>(LottoRank.class);
    //...
    return new LottoResult(lottoRanks);
  }

 

 

로또 당첨 개수를 가지는 Lottos에서 로또 당첨번호, 보너스 볼을 인자로 받아서 로또 결과를 생성하고 있었다.

 

아래와 같은 피드백을 받았다.

💡 이 메소드는 LottoResult 을 생성하는 목적을 가지고 있어요.
메소드의 내용은 LottoResult를 생성하기 위한 Map을 만들고 있네요.
그렇다면 여기와 LottoResult 생성자 중 어느 위치에 두는게 객체의 목적에 더 부합할까요?

 

Lottos는 구매한 로또를 가지고 있는 객체이다. 해당 객체 내에서 LottoResult의 상태값인 Map을 생성하고 LottoResult 객체를 생성하는 것은 Lottos의 객체의 목적에 부합하지 않는다. LottoResult에서 직접 만드는게 객체의 역할에 부합한다.

 

구현하려고 하니 해당 피드백을 구현하는데 가장 애를 먹었다.

우선, winCounts의 모든 로직을 LottoResult의 생성자로 옮겨야 했다. 옮기다보니 생성자에 파라미터를 4개나 넘겨주고 있었다. 4개를 넘겨주는게 과연 최선인지 고민이 들었지만 별다른 해결책이 생각나지 않았다.

LottoFactory와 같이 LottoResultFactory를 구현해야 하나 고민도 했지만 불필요한 클래스 추가 같은 느낌이 들어 하지 않았다.

 

아래와 같이 구현하였다.

public LottoResult(int lottoCount, Lottos lottos, LottoWinNumbers lottoWinNumbers, BonusBall bonusBall) {
        lottoResult = new EnumMap<>(LottoRank.class);
        initLottoResult();
        createLottoResult(lottoCount, lottos, lottoWinNumbers, bonusBall);
    }

    private void initLottoResult() {
        for (LottoRank lottoRank : DISPLAY_LOTTO_RANKS) {
            lottoResult.put(lottoRank, 0L);
        }
    }

    private void createLottoResult(int lottoCount, Lottos lottos, LottoWinNumbers lottoWinNumbers, BonusBall bonusBall) {
        for (int i = 0; i < lottoCount; i++) {
            LottoRank rank = lottos.matchCount(i, lottoWinNumbers, bonusBall);
            lottoResult.merge(rank, 1L, Long::sum);
        }
    }

 

아래와 같은 피드백을 받았다.

💡 매개변수가 많다면 더 나은 설계를 할 수는 없는지, 몇 개를 묶을 수도 있는지를 고민해보세요.

제 생각엔 구매전용 로또와 당첨 로또번호는 약간 다릅니다.구매전용 로또는 중복없는 번호를 6개를 뽑지만 당첨 로또번호는 6개 + 1개 보너스 번호로 구성되어 있기 때문입니다.그럼 하나의 의미로 묶을 수도 있겠네요.

그리고 lottoCount도 꼭 필요해보이진 않습니다.lottos.matchCount() 수행에 사용되지만 lottoCount 없이도 충분히 수행할 수 있는 로직으로 보이거든요.lottos.matchCount() 가 List 를 반환한다면 어떤가요?

 

createLottoResult 메소드에서 lottoCount 개수 만큼 for문을 돌고 있는데 lottoCount는 리뷰어님 말씀하신대로 필요없다. 

왜냐하면 lottoCount는 Lottos에 있는 상태값의 사이즈 개수와 같기 때문이다. Lottos에 matchCount() 메시지를 보내면 Lottos에서 상태값 크기 만큼 for문을 돈 후에 List를 반환하면 되기 때문이다.

 

매개변수는 3개면 양호 하다고 생각하였고, 아래와 같이 구현하였다.

public class LottoResult {

    private final Map<LottoRank, Long> lottoResult;

    public LottoResult(Lottos lottos, LottoWinNumbers lottoWinNumbers, BonusBall bonusBall) {
        lottoResult = new EnumMap<>(LottoRank.class);
        initLottoResult();
        createLottoResult(lottos, lottoWinNumbers, bonusBall);
    }

    private void initLottoResult() {
        for (LottoRank lottoRank : DISPLAY_LOTTO_RANKS) {
            lottoResult.put(lottoRank, 0L);
        }
    }

    private void createLottoResult(Lottos lottos, LottoWinNumbers lottoWinNumbers, BonusBall bonusBal) {
        List<LottoRank> ranks = lottos.matchCount(lottoWinNumbers, bonusBal);
        for (LottoRank rank : ranks) {
            lottoResult.merge(rank, 1L, Long::sum);
        }
    }
    
    //..
    
}

 

public class Lottos {
    private final List<Lotto> lottos;

    public List<LottoRank> matchCount(LottoWinNumbers lottoWinNumbers, BonusBall bonusBall) {
        List<LottoRank> lottoRanks = new ArrayList<>();
        for (Lotto lotto : this.lottos) {
            LottoRank lottoRank = lotto.matchCount(lottoWinNumbers, bonusBall);
            lottoRanks.add(lottoRank);
        }
        return lottoRanks;
    }

 

 

아래 피드백은 잘 이해가 가지 않아서 4단계에서 다시 여쭤보았다.

제 생각엔 구매전용 로또와 당첨 로또번호는 약간 다릅니다.구매전용 로또는 중복없는 번호를 6개를 뽑지만 당첨 로또번호는 6개 + 1개 보너스 번호로 구성되어 있기 때문입니다.그럼 하나의 의미로 묶을 수도 있겠네요.

 

위와 같은 피드백을 로또 당첨 번호와 보너스 볼을 묶는 LottoWin 클래스를 만드는 것으로 해석이 되었다.

구현해보니 LottoNumber -> LottoWin -> LottoWinNumber 와 같은 메소드 호출 구조가 잡혀 이것도 맞나 의심이 들었다.

(4단계 피드백 확인)

 

 

public class LottoResult {

  public List<LottoWinResult> lottoStatistics() {

 

LottoResult클래스의 lottoStatistics() 메소드에서 List를 만들어 주고 있다.

Lottos에서 LottoResult를 생성하는 로직을 LottoResult의 생성자로 옮겼듯이,

List<LottoWinResult> 생성도 LottoWinResult 클래스쪽으로 옮겨야 하는건지 궁금했다. 

 

아래와 같은 피드백을 받았다.

💡 LottoWinResult에서 List<LottoWinResult> 를 반환하는게 부자연스러워 보이는데요,
LottoWinResult -> List<LottoWinResult> -> List<List<LottoWinResult>> -> ...
로직을 잘못 짜면 무한루프에 빠질 가능성이 있어서 그런듯 합니다.

 

 

 

 

public class LottoWinResult {
    private final long machCount;
    private final long prizeAmount;
    private final long winCount;
    private final boolean isBonus;

 

LottoWinResult는 당첨 통계를 출력하는데 필요한 필드들이다.

 

아래와 같은 피드백을 받았다.

💡 필드가 너무 많습니다. LottoRank 는 이 클래스 필드의 몇 개를 단 하나로 표현하고 있지 않던가요?

LottoWinResult 이 n개 맞은 로또의 결과를 저장하고 있는데 필드를 Map <key:LottoRank, value:총 상금> 처럼 변경하는 방법도 고려해볼 수 있습니다.

이 도메인은 총 당첨금에 대한 정보로도 볼 수 있거든요.
지금처럼 단 하나의 등수에 대해서만 취급한다면 LottoWinResult 을 여러 개 담는 일급 콜렉션에서 처리할 수도 있구요.

조금 더 고민해보시고 다음 단계 진행하면서 반영해주세요.

 

생각해보니 리뷰어님이 언급해주신 Map <key:LottoRank, value:총 상금>는 LottoResult에 존재하였다.

LottoWinResult 클래스를 삭제하고 해당 메소드의 역할을 LottoResult로 코드를 이전하였다.

 

 

Step4 로또(수동)

Step4 로또(수동)

 

LottoWinNumbers(당첨 번호)와 LottoNumbers(로또 번호)에서 숫자 범위를 확인하는 로직이 중복이였다.

당첨 번호든 로또 번호던 같은 번호 이기 때문에 클래스 도출이 필요하다는 생각이 들었다.

하지만 LottoNumber -> LottoWin -> LottoWinNumber -> LottoNumber 와 같은 순환 참조로 구현이 되어 고민이 들었고 설계가 꼬였다는 생각이 들었다.

 

중복 로직은 아래와 같다.

public class LottoNumbers {
    private static final int MIN_NUMBER = 1;
    private static final int MAX_NUMBER = 45;

    private final List<Integer> numbers;

    public LottoNumbers(LottoGenerator lottoGenerator) {
        List<Integer> numbers = lottoGenerator.lotto();
        validateNumberRange(numbers);

 

public class LottoWinNumbers {
    private static final int MIN_NUMBER = 1;
    private static final int MAX_NUMBER = 45;

    private List<Integer> winningLottoNumbers;

    public LottoWinNumbers(List<Integer> winningLottoNumbers) {
        validateNumberRange(winningLottoNumbers);

 

아래와 같은 피드백을 받았다.

💡  이 클래스를 포함, LottoNumbers에 List<Integer> 대신 List<로또번호>로 바꿔보시는건 어떤가요?
하나의 책임을 더 분리하게 되었을 때의 장점이 있을 것 같네요.

 

 

아래와 같이 구현 하였고, 설계가 꼬인 문제는 LottoGenerator를 제거하고 LottoFactory를 생성하니 해결 되었다.(아래 피드백 참고)

숫자 중복 책임이 LottoNumber 한곳에만 존재하여 중복코드가 줄었다.

public class LottoWinNumbers {
    private final List<LottoNumber> winningLottoNumbers;
    
   
public class LottoNumbers {
    private final List<LottoNumber> numbers;
    
public class LottoNumber {
    private static final int MIN_NUMBER = 1;
    private static final int MAX_NUMBER = 45;

    private final int number;

    public LottoNumber(int number) {
        validateNumberRange(number);
        this.number = number;
    }
    
    //..
    
}

 

 

나를 많이 헷갈리게 만든 코드는 아래 였다.

public class LottoGenerators {

    private List<LottoGenerator> lottoGenerators;

    public LottoGenerators(List<String> stringFormatManualLottos, int autoLottoCount) {
        List<LottoGenerator> lottoGenerators = new ArrayList<>();
        generateManualLotto(stringFormatManualLottos, lottoGenerators);
        generateAutoLotto(autoLottoCount, lottoGenerators);
        this.lottoGenerators = lottoGenerators;
    }

    private void generateManualLotto(List<String> stringFormatManualLottos, List<LottoGenerator> lottoGenerators) {
        for (String stringFormatManualLotto : stringFormatManualLottos) {
            List<Integer> manualLotto = StringParser.parseToInts(stringFormatManualLotto);
            InputValidator.validateNumberCount(manualLotto.size());
            lottoGenerators.add(new ManualLottoGenerator(manualLotto));
        }
    }

    private void generateAutoLotto(int autoLottoCount, List<LottoGenerator> lottoGenerators) {
        for (int i = 0; i < autoLottoCount; i++) {
            lottoGenerators.add(new RandomLottoGenerator());
        }
    }
    
    public Lottos generateLottos() {
        List<Lotto> lottos = lottoGenerators.stream()
                .map(lottoGenerator -> new Lotto(new LottoNumbers(lottoGenerator)))
                .collect(Collectors.toList());
        return new Lottos(lottos);
    }
    
}

 

위 코드에 대해서 두가지 피드백을 받았다.

 

피드백1

public class LottoGenerators {

 

💡  설계가 꼬였다고 언급하셨는데요,
제 생각엔 LottoGenerators가 반드시 필요한 클래스는 아닌 것으로 보입니다.
이 클래스의 목적이 결국 generateLottos() 로 향하고 있는데 삭제된 LottoFactory 에서 담당하는게 역할론적으로는 더 맞지 않나하는 생각이 드네요.
LottoGenerator 는 로또 생성보다는 로또 번호 생성하는 역할에 더 가깝거든요.

이쪽 의존 관계만 정리해도 이번 단계는 잘 학습했다고도 할 수 있을 것 같습니다.
조금 더 고민해보세요 :)

 

피드백2

   public Lottos generateLottos() {
        List<Lotto> lottos = lottoGenerators.stream()
                .map(lottoGenerator -> new Lotto(new LottoNumbers(lottoGenerator)))
                .collect(Collectors.toList());
        return new Lottos(lottos);
    }

💡  이 클래스가 설계상 오류로 느껴지는 또 다른 이유는 generateLottos()의 호출 구조 때문이 아닐까요?

LottoGenerators 를 선언하면 List<LottoGenerator> 의 데이터가 채워지고 generateLottos() 를 호출하는 방식입니다만, 클래스 상태를 기록하는 용도로 사용되진 않고 있네요.

무슨 이야기냐면 LottoGenerators 가 한 번 생성된 이상 무엇을 해도 generateLottos() 의 결과는 고정되어 있다는 것입니다. 클래스의 상태가 전혀 변하지 않아요.

게다가 이 클래스를 생성하고 메소드를 호출해야 한다는 순서도 반드시 지켜져야 한다는 부담을 안고 있습니다.

List<LottoGenerator> 도 조금 더 생각해볼만한게 수동은 로또 개수만큼 생성기도 만들어져야 하지만 무작위는 단 1개의 인스턴스로도 충분합니다.

 

클래스의 상태가 전혀 변하지 않고 메소드 호출 순서에 대한 부담

이것이 정복을 찌르는 문장이였다.

클래스의 상태는 변하지 않는다. 그 말은 상태가 필요 없다는 말이 아닐까?

new Lottos의 생성에 필요한 값만 만들면 된다.

Lottos의 생성 책임을 맡는 LottoFactory를 만들었다.

Lottos에는 수동로또와 자동 로또가 모두 들어가야 하니, 입력 받은 수동 로또 개수 만큼 ManualLottoGenerator를 생성한다. 자동 로또는 1개의 인스턴스 만으로 충분하다.

public class LottoFactory {

    private LottoFactory() {
    }

    public static Lottos generateLottos(List<String> stringFormatManualLottos, int autoLottoCount) {
        List<Lotto> lottos = new ArrayList<>();
        generateManualLotto(stringFormatManualLottos, lottos);
        generateAutoLotto(autoLottoCount, lottos);
        return new Lottos(lottos);
    }

    private static void generateManualLotto(List<String> stringFormatManualLottos, List<Lotto> lottos) {
        for (String stringFormatManualLotto : stringFormatManualLottos) {
            List<LottoNumber> manualLotto = StringParser.parseToInts(stringFormatManualLotto);
            InputValidator.validateNumberCount(manualLotto.size());
            lottos.add(new Lotto(new LottoNumbers(new ManualLottoGenerator(manualLotto))));
        }
    }

    private static void generateAutoLotto(int autoLottoCount, List<Lotto> lottos) {
        RandomLottoGenerator randomLottoGenerator = new RandomLottoGenerator();
        for (int i = 0; i < autoLottoCount; i++) {
            lottos.add(new Lotto(new LottoNumbers(randomLottoGenerator)));
        }
    }

}

 

 

 

제 생각엔 구매전용 로또와 당첨 로또번호는 약간 다릅니다.구매전용 로또는 중복없는 번호를 6개를 뽑지만 당첨 로또번호는 6개 + 1개 보너스 번호로 구성되어 있기 때문입니다.그럼 하나의 의미로 묶을 수도 있겠네요.

 

Step3에서 100% 이해 하지 못한 피드백에 대한 답변은 아래와 같다.

 

💡 이 위치에 너무 매개변수가 많다고 질문주셔서 드린 답변이었어요.

반드시 묶을 필요는 없지만 하나로 묶는다면 '당첨 번호' 라는 의미를 잘 전달하므로 그러한 의견을 드렸습니다.

저는 보통 피드백보다는 의견을 드렸다고 표현하려고 하는데
그 이유는 반드시 코드에 반영하길 원하는게 아니라 리뷰이님의 생각에 도움을 드리는 정도의 역할을 원하기 때문입니다.

이해가 안된다거나, 의견과 다른 생각을 가지고 있으시다면 언제든지 DM주셔서 서로 이야기하며 무엇이 더 나은지 맞춰가는 것도 즐거운 일이구요 :)

 

리뷰어님이 곧 정답이라 라는 마인드를 크게 가졌나 보다.

기존대로 사용하는 것이 좋다는 생각이 들어 LottoWin 클래스를 제거하였다.

 

 public List<LottoGenerator> getLottoGenerators() {
        return Collections.unmodifiableList(lottoGenerators);
    }

 

테스트 코드에서 값을 꺼내서 검증하기 위해 getter 메소드를 만들었다.

 

아래와 같은 피드백을 받았다.

💡  이 메소드는 테스트에서만 사용한다면 지워주세요.

 

설계를 변경하지 getter를 꺼내서 검증하는 테스트 코드 자체가 필요 없어져서 getter 메소드를 지웠다.

 

 

 

미션 마지막 리뷰는 더블 칭찬을 받아서 기분이 좋다 ^.^