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

[Lee, Nathan] 로또 2단계 - 보너스 번호 추가 #41

Open
wants to merge 19 commits into
base: nathan29849
Choose a base branch
from

Conversation

nathan29849
Copy link
Member

안녕하세요! Lee, Nathan 입니다.
2단계 요구사항과 리팩토링을 진행하고 PR 보냅니다.
항상 감사드립니다.


로또 2단계 - 보너스 번호 추가

기능요구사항

  • 2등을 위해 추가 번호를 하나 더 추첨한다. 당첨 통계에 2등도 추가해야 한다.

프로그래밍 요구사항

  • enum을 적용해 프로그래밍을 구현한다.

힌트

  • Lotto 클래스를 사용자가 구매한 Lotto와 기계에 의해 추첨한 Lotto로 구분해서 생성한다.

image

nathan29849 and others added 15 commits February 23, 2022 14:16
Enum 클래스의 이름을 Rank로 변경
designateRank 메서드(switch문)를 Enum 클래스 내부로 이동
TicketOffice.issueTicket() 메서드 내에서 자동 추첨/수동 추첨 여부를 유저에게 입력받음.
이후 makeAutoTicket(), makeManualTicket()으로 분기하도록 구현.
수동 추첨의 경우 InputView.getManualNumber()를 통해 번호를 입력받은 후 로또 생성.
Rank enum Class에 FIRST ~ FIFTH 까지 총 5단계로 구성
TicketOffice에서 보너스를 처리하는 메서드 : checkBonusNumber(), setBonusNumber() 추가
이에 따른 InputView, OutputView 개선
…icket으로 옮김

TicketOffice -> LottoTicket으로 당첨 티켓 비교 기능 옮김
comparisonWinningTicket, checkNumber 메서드 생성
InputView.askIsAuto()에서 sc.next() -> sc.nextLine.charAt(0) 으로 바꾸어 char끼리 비교해 커맨드 수행하도록 수정.
자동, 수동으로 나뉘어 있던 메서드를 TicketOffice.makeLottoTicket()으로 합침.
해당 메서드의 매개변수로 boolean isAuto를 넘겨서 자동/수동에 따른 로직 분기하도록 구현.
…er()로 옮김

티켓 내부 정보에 대한 검사(포함하는지 안하는지)에 대한 책임은 LottoTicket이 지도록 지정
기존 TicketOffice의 역할이 과도하게 크다고 판단하여 당첨 번호 설정 및 당첨금 확인(통계) 기능을 담당하는 LottoCompany 클래스를 만들어 분리.

getStatistic()과 연결되는 calculateProfit에서 사용하는 TOTAL_PRICE 값은 TicketOffice에서 로또를 구매할 때 설정되기 때문에 구조 수정 필요.
클래스 분리로 인해 Main 로직도 수정 필요.
User 클래스를 추가하여 purchaseTicketsFrom() 메서드를 통해 TicketOffice에서 티켓을 구매하도록 구현.
이후 LottoCompany와의 상호작용을 통해 결과(통계) 출력 기능 수정 필요.
User.checkMyTicketsFrom(lottoCompany) 추가
Enum Rank 필요없는 매개변수 isBonus 제거
LottoCompany.check(tickets)를 통해 User의 당첨 정보를 조회
Enum 클래스인 Rank를 수정해 boolean 값인 isBonus를 가지고 있게 하고, LottoCompany의 멤버변수 statisctic의 key를 Rank로 변경.
LottoCompany의 statistics 자료형 구조 변경(<Integer, Integer> -> <Rank, Integer>)로 인한 showWinningReulst 메서드 변경 개선
FAIL 추가를 통해 꽝인 경우를 제대로 분기하게 하였습니다.
@nathan29849 nathan29849 added the review-BE Improvements or additions to documentation label Feb 25, 2022
nathan29849 and others added 4 commits February 25, 2022 11:30
InputView.getNumberOfManualTicket() : 수동으로 구매할 로또수만 입력받도록 수정
TicketOffice.makeTicket() : makeManualTickets, makeAutoTicket으로 분리하여 따로따로 티켓을 생성하도록 수정
기존 TicketOffice 클래스를 상속하는 AutoTicketOffice, ManualTicketOffice 클래스 추가.
각 클래스가 issueTickets()를 오버라이딩하는 형태로 구현.
User.goTicketOffice : 수동 자동 각각 티켓을 구입한 뒤 OutputView에 출력할 정보를 넘긴다.
(메서드 상속으로 인한 리팩터링의 결과)
@wheejuni wheejuni self-requested a review February 26, 2022 00:40
@wheejuni wheejuni self-assigned this Feb 26, 2022
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.

수고하셨습니다~ 👍
코멘트 참고 부탁드려요

private LottoTicket winningTicket;
private int bonusNumber;
private int totalPrice;
private final int PRICE = 1000;

Choose a reason for hiding this comment

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

인스턴스 변수인데 대문자로 선언되어 있고 값도 부여되어 있군요.
제 생각에 이 변수는 인스턴스 변수로 관리할 것이 아니라 static 키워드를 추가로 부여해서 런타임 상수로 보존하는 게 좋을 것 같군요.

Comment on lines +39 to +41
int matchedNumber;
boolean isBonus;
Rank currentRank;

Choose a reason for hiding this comment

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

이 세가지 정보를 LottoTicket 이 직접 줄 수 있어야 한다고 생각되네요.

private int bonusNumber;
private int totalPrice;
private final int PRICE = 1000;
private final Map<Rank, Integer> statistics = new HashMap<>();

Choose a reason for hiding this comment

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

statistics 라는 것이 제가 이해하기론 몇 등짜리가 몇 장 당첨되었느냐를 알려주면 되는 부분인 것 같네요.
그렇다면 굳이 Map 형태로 보관할 필요 있을까요?
필요할 때마다 List<LottoTicket> 을 순회하며 계산하면 될 것 같기도 한데요? 자신이 몇 등 당첨되었는지는 LottoTicket 이 직접 말하게 하면 되고요.

Comment on lines +28 to +40
switch (matchedNumber) {
case 3:
return FIFTH;
case 4:
return FORTH;
case 5:
return checkBonus(isBonus);
case 6:
return FIRST;
default:
return FAIL;
}
}

Choose a reason for hiding this comment

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

스위치문 없앨 수 있습니다. 없애주세요.

힌트: .values(), .stream(), .filter()


public void goTicketOffice(){
this.amount = InputView.getAmount();
int pricePerTicket = TicketOffice.getPrice();

Choose a reason for hiding this comment

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

로또 한장이 얼마인지는 동적으로 바뀔 필요가 없는 것 같은데 굳이 TicketOffice 에서 게터를 호출해야 하는 구현이 좋은지는 잘 모르겠습니다. 퍼블릭 상수로 관리되면 어떨까 싶네요.

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.

3 participants