Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[쿠킴 / 땃쥐] 로또 2단계 - 보너스 번호 추가 #25

Merged
merged 5 commits into from Feb 24, 2022

Conversation

ttasjwi
Copy link

@ttasjwi ttasjwi commented Feb 23, 2022

인사

안녕하세요! 땃쥐, 쿠킴입니다.
이번 로또 미션 2단계 구현 결과물에 대해 풀리퀘스트 드립니다.


이전 리뷰(#13)에 대한 피드백

public class LottoController {
    public void run() {
        // ... (중략)
        LottoTickets lottoTickets = LottoTickets.createRandomTickets(money);
        OutputView.printLottoTickets(lottoTickets);

        WinningTicket winningTicket = InputView.inputWinnigTicket();
        // ... (중략)
    }
}
  • 리뷰(Honux) : 위닝 티켓도 로또의 일종으로 볼 수 있지 않을까요?
  • 피드백(땃쥐, 쿠킴)
    • 둘의 공통점이 있어 상위 개념으로 추상화하는 것도 고려해보긴 했습니다.
    • 그러나 WinningTicket은 내부적으로 로또티켓과 비교하여 등수를 반환하는 기능(match)을 가지고 있습니다.
    • 추가적으로 WinningTicket은 2단계 요구사항에 따라, 보너스 번호라는 상태도 가지게 됩니다.
    • 위의 사항을 고려했을 때, WinningTicket을 그대로 사용하기로 했습니다.

요구사항 및 반영 내역

  • 보너스 번호를 입력 및 보너스 추가 추첨 결과 출력
  • enum을 이용해 추첨 결과 정의
  • 그 외 세부적인 변경 내역들은 별도로 README에 정리해두었습니다.

고민 및 질문사항

LottoGameResults

public class LottoGameResults {
    // 변경 전
    private long getTotalReward() {
        return rankCounts.entrySet().stream()
                .mapToLong(entry -> entry.getKey().getReward() * entry.getValue())
                .sum();
    }

    // 변경 후
    private long getTotalRewards() {
        long totalRewards = 0;
        for (Map.Entry<Rank, Integer> entry : rankCounts.entrySet()) {
            long reward = entry.getKey().getReward();
            int count = entry.getValue();
            long totalReward = reward * count;
            totalRewards += totalReward;
        }
        return totalRewards;
    }
}
  • 기존 getTotalReward() 메서드는 스트림으로 구현했다.
  • 깔끔해 보이지만 entry.getKey().getReward()entry.getValue()에 대한 의미를 파악하기 어려웠다.
  • 따라서 for문으로 변경하여 해당 변수명을 할당하여 의미 전달을 높였다.
  • 호눅스의 견해를 듣고 싶습니다. 위 코드를 놓고 봤을 때, 어느 것이 더 가독성이 좋아보이나요?

감사합니다!

- InputView.inputWinnigTicket에서 기존 당첨 번호와 보너스 번호 입력 받아 WinningTicket 객체 생성
- 당첨 번호와 보너스 번호 중복 시 예외 발생
- WinningTicket.match 메서드에 보너스 번호 유무 로직 추가
- Rank에 2등 enum과 toString 추가
- InputView.inputWinnigTicket -> InputView.inputWinningTicket
- getTotalReward -> getTotalRewards 메서드명 변경
- 기존 람다식에는 변수에 대한 설명이 없어 for문으로 변경하여 각 값들을 변수로 두어 의미 전달을 명확히 함
- READEME.md 파일에 step1과 step2 결과와 코드와 설명, 고민 점 작성
@ku-kim ku-kim added the review-BE Improvements or additions to documentation label Feb 24, 2022
@ku-kim ku-kim assigned ku-kim and ttasjwi and unassigned ku-kim Feb 24, 2022
@honux77
Copy link
Contributor

honux77 commented Feb 24, 2022

일단 스트림이 좋습니다 ㅎㅎㅎ

Copy link
Contributor

@honux77 honux77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

참 로또 생성을 셋으로 반복적으로 겹치지 않게 생성하는 것보다는
1-46까지 리스트를 섞어서 앞에서 6개를 가져오는 편이 더 안정적입니다.

THIRD(5, 1_500_000L),
FOURTH(4, 50_000L),
FIFTH(3, 5_000L),
FAILED(-1, 0L);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매치 카운트라면 Failed는 0이 적합하지 않나요? -1인 이유가 있을까요?

Copy link
Member

@ku-kim ku-kim Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1로 했던 이유는 5등 이외의 맞은 개수 0,1,2 개가 있기 때문에 통칭하여-1로 했습니다.
생각해보니 -1로 했을때 만약 FAILED의 rank 값을 외부에서 사용할 수 있기 때문에 -1이 문제가 될 수 있어보이네요!
감사합니다.

Copy link
Contributor

@honux77 honux77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

참 로또 생성을 셋으로 반복적으로 겹치지 않게 생성하는 것보다는
1-46까지 리스트를 섞어서 앞에서 6개를 가져오는 편이 더 안정적입니다.

전반적으로 분리도 잘 되어 있고 코드고 깔끔해 보입니다.
수고하셨습니다!

@honux77 honux77 merged commit ecd0534 into codesquad-members-2022:ttasjwi Feb 24, 2022
@ttasjwi
Copy link
Author

ttasjwi commented Feb 24, 2022

코드 리뷰 감사합니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants