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

[포키 && 필] 로또 3단계 - 수동구매 기능 추가 #55

Open
wants to merge 7 commits into
base: PhilSoGooood
Choose a base branch
from

Conversation

PhilSoGooood
Copy link

@PhilSoGooood PhilSoGooood commented Feb 25, 2022

안녕하세요 리뷰어님!
포키와 필입니다.
로또 3단계 기능을 구현해서 PR 보냅니다.
그 과정에서 로또는 하지 않는게 좋다는 교훈을 얻었습니다.

감사합니다 주말 잘보내세요! :)

2단계 리뷰 반영사항

  • 당첨번호와 보너스 넘버를 합쳐서 WinningLotto라는 클래스를 만들어 숫자가 몇개가 맞았는지 확인하도록 하였습니다.
  • LottoResult클래스의 Map<Rank, Integer>result를 메서드를 통해 한번에 Rank를 넣도록 하였습니다.
  • 변수명을 상수가 아닌 것은 소문자로 변경하고 보다 명확하게 네이밍 하도록 노력하였습니다.

기능 요구사항

  • 현재 로또 생성기는 자동 생성 기능만 제공한다. 사용자가 수동으로 추첨 번호를 입력할 수 있도록 해야 한다.
  • 입력한 금액, 자동 생성 숫자, 수동 생성 번호를 입력하도록 해야 한다.

프로그래밍 요구사항

  • 예외가 발생하는 부분에 대해 자바 Exception을 적용해 예외처리한다.
  • 사용자가 입력한 값에 대한 예외 처리를 철저히 한다.
  • 상속과 인터페이스를 통해 구현을 간결히 할 수 없는지 고민해 본다. -> Composition을 사용했습니다.

gitignore.io를 활용해서 macOS, java, gradle, intellij를 추가하였습니다.
Lotto 각 티켓의 결과 비교는 WinningLotto가 담당하고
위 결과를 종합하고 당첨금 및 수익률 계산은 LottoGam이
담당합니다.

LottoResult는 각 등수가 몇번 있었는지 결과를 담는 역할을 담당합니다.
Rank의 체크 메서드를 분리하여 null값을 반환하지 않도록 수정하였습니다.
controller의 로또 티켓 생성을 통해 바로 Ticket을 반환하도록
변경하였습니다.
타입을 변경하였습니다.

scanner의 초기화 및 close를 담당하는 메서드를 구현하였습니다.
model의 각 클래스가 변경됨에 따라 이에 맞도록 OutputView의 각 메서드의
매개변수의 타입을 변경하였습니다.
gradle로 프로젝트를 변경함에 따라 빌드 파일을 추가합니다.
수동 구매 수량 및 해당 수량만큼 수동 번호를 입력 받는 기능을
구현하였습니다.
구매금액과 수동 구매 수량, 당첨번호, 보너스 번호 입력에
대해 예외처리를 하도록 했습니다.
@PhilSoGooood PhilSoGooood added the review-BE Improvements or additions to documentation label Feb 25, 2022
@wheejuni wheejuni self-assigned this Feb 26, 2022
@wheejuni wheejuni self-requested a review February 26, 2022 13:48
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 👍
코멘트 확인해주시고요, 테스트 코드가 너무 없군요.
테스트에도 조금 관심을 부탁드려요~

int ticketCount = calculateTicketAmounts(purchaseAmount);
LottoTickets lottoTickets;
if (manualCount > 0) {
List<List<Integer>> manualNumbers = inputView.inputManualNumbers(manualCount);

Choose a reason for hiding this comment

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

List<Integer> 는 여기에서 어떤 의미를 갖나요? 로또 번호에 대한 사용자 입력, 혹은 로또번호의 집합 그 자체가 될 수 있을 것 같은데요...
이걸 적절히 감싸서 리스트를 원소로 갖는 리스트, 즉 List<List<Integer>> 를 쓰지 않도록 리팩터링 부탁드려요.

Comment on lines +8 to +15
List<Lotto> ticketList = tickets.getTickets();
LottoResult result = new LottoResult();

for (Lotto lotto : ticketList) {
int ticketResult = winningNumbers.compareTicket(lotto.getTicket());
boolean bonusResult = winningNumbers.compareBonusNumber(lotto.getTicket());
result.check(ticketResult, bonusResult);
}

Choose a reason for hiding this comment

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

이 로직은 LottoTickets 에 있어야 할 겁니다.
공들여 일급 컬렉션 객체를 설계했는데 그걸 그냥 게터를 호출해서 값을 가져오고, 일급 객체 외부에서 로직을 수행한다면 다소 아쉬운 구현이라고 볼 수 있겠죠. 이걸 LottoTickets 로 옮기는 방향으로 리팩토링 부탁드려요.

return matchedCount;
}

private int containNumber(int number) {

Choose a reason for hiding this comment

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

여기서 boolean을 리턴했다면 compareTicket() 을 스트림 기반으로 리턴할 수 있지 않을까요?

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.

2 participants