-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Step4 - 로또(수동) #3238
base: gisungpark
Are you sure you want to change the base?
Step4 - 로또(수동) #3238
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기성님, 미션 구현하시느라 수고하셨습니다!! 👍🏼
제가 앞서 남긴 피드백과 추가적인 비즈니스 사항을 적절하게 잘 반영해주신 것 같아요!! 제가 우선은 코드 레벨 수준에서의 피드백을 먼저 남겨봤는데 해당 부분 한번 확인해서 반영 후 리뷰 요청 주시면 될 것 같아요 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기성님, 피드백 반영하시느라 수고하셨습니다 👍🏼 추가로 제가 한동안 장염을 조금 심하게 걸려서 리뷰가 많이 늦어졌는데 이 점 정말 죄송합니다 ㅠㅠ
이번 리뷰에서는 뷰와 도메인 로직의 분리가 조금 아쉬운 부분들이 있는데 이 부분을 좀 더 신경써서 봐주시면 좋을 것 같아요!! 테스트 코드도 꼼꼼하게 잘 짜주셨고 로직도 충분히 잘 작성된 것 같아요. 그래서 비즈니스 로직을 도메인 객체에 적절하게 담아두고 분리하는 부분을 집중해준다면 좋은 개선이 될 수 있을 것 같습니다!! 💯
|
||
public class LottoGameController { | ||
|
||
private static final int LOTTO_TICKET_PRICE = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또 금액은 비즈니스 영역에 해당하는 값으로 보여지는데 현재 Controller에서 상수로 관리되고 있어요! 적절한 도메인 객체에서 해당 값을 관리하는 것이 어떨까요?
public int pay(int cost) { | ||
if (cost > this.money) { | ||
throw new IllegalArgumentException("잔액을 초과하셨습니다."); | ||
} | ||
this.money -= cost; | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0이라는 반환 값이 별다른 의미가 없어보이는데 void 메서드로 만드는 것이 어떨까요?
|
||
ResultView.printBlankLine(); | ||
ResultView.printMessage("당첨 통계"); | ||
LottoNo bonusNumber = LottoNo.of(InputView.readBonusNumber()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
축약어는 최대한 지양하면 좋을 것 같아요!! LottoNumber라는 이름이 좀 더 직관적이고 가독성이 더 좋을 것 같아요!!
private List<LottoTicket> buyManualTickets(Money money) { | ||
int manualTicketCount = getManualTicketCount(money); | ||
money.pay(manualTicketCount * LOTTO_TICKET_PRICE); | ||
|
||
ResultView.printMessage("수동으로 구매할 번호를 입력해 주세요"); | ||
List<LottoTicket> manualLottoTickets = new ArrayList<>(); | ||
for(int i=0; i<manualTicketCount; i++) { | ||
Optional<LottoTicket> lottoTicket = lottoGames.toLottoTicket(InputView.readManualTicketNumbers()); | ||
manualLottoTickets.add(lottoTicket.orElseThrow(IllegalArgumentException::new)); | ||
List<LottoTicket> manualLottoTickets = new ArrayList<>(manualTicketCount); | ||
for (int i = 0; i < manualTicketCount; i++) { | ||
LottoTicket lottoTicket = lottoGames.toLottoTicket(InputView.readManualTicketNumbers()); | ||
manualLottoTickets.add(lottoTicket); | ||
} | ||
return manualLottoTickets; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금은 입력 로직으로 인해 비즈니스 로직과 입력 로직이 많이 엮여있는 것 같아요!! 로직을 절차식으로 작성하려고 하면서 오히려 이 두 로직의 구분이 어려워 진 걸 수도 있어요!!
만약 입력 값을 먼저 다 받아두고 해당하는 입력값을 한번에 LottoGames에 넘겨줘서 로직을 구현할 수도 있고요!! 지금의 로직이 문제가 있는 것은 아니지만 조금 더 뷰의 로직과 도메인의 로직을 구분하고 싶다면 이런 식으로 개선을 해볼 수도 있을 것 같아요!!
} | ||
|
||
private List<LottoTicket> getBuyAutomaticTickets(Money money) { | ||
int automaticTicketCount = money.balance() / LOTTO_TICKET_PRICE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 로직은 getter를 사용해서 값을 가져와서 계산을 하고 있어요!! 이렇게 된다면 값을 객체로 감싼 의미가 조금은 퇴색될 수 있는데 어떻게 바꾸면 좋을까요?
import java.util.stream.IntStream; | ||
|
||
public class LottoNo { | ||
private static final int MIN_LOTTO_NUMBER = 1; | ||
private static final int MAX_LOTTO_NUMBER = 45; | ||
|
||
private static Map<Integer, LottoNo> lottoNumberCache = new HashMap<>(); | ||
private static final Map<Integer, LottoNo> lottoNumberCache = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍도 상수 컨벤션에 맞게 바꾸면 좋을 것 같아요!!
return lottoTicket.stream() | ||
.map(i -> String.valueOf(i.number())) | ||
.collect(Collectors.joining(",")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또 번호들을 리스트로 반환해주고 뷰에서 출력 형식에 맞게 joining을 해서 출력하는 것이 어떨까요?? 이렇게 되면 출력 형식이 바뀔 때 LottoTicket 도메인도 영향을 받을 것 같아요
src/main/java/step2/domain/Rank.java
Outdated
MISS(0, 0, "0개 일치 (0)"), | ||
FIFTH(3, 5_000, "3개 일치 (5000)"), | ||
FOURTH(4, 50_000, "4개 일치 (50000)"), | ||
THIRD(5, 1_500_000, "5개 일치 (1500000)"), | ||
SECOND(5, 30_000_000, "5개 일치, 보너스 볼 일치(30000000원)"), | ||
FIRST(6, 2_000_000_000, "6개 일치 (2000000000)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰와 도메인을 철저하게 분리하고자 한다면 여기의 message 부분도 뷰에 따로 빼두는 것도 좋을 것 같아요!!
만약 string 필드의 의미가 6등, 1등 등 도메인에 대한 설명이나 description에 해당하는 부분이라면 그대로 가질 수 있겠지만 현재 영역은 출력을 위한 message에 가깝기에 도메인 객체가 가지긴 적절하지 않은 필드인 것 같아요!!
예를 들어 View 패키지에 RankMessage라는 enum 객체를 두고 해당 객체가 Rank 객체외 메시지를 필드로 가지도록 하면 이러한 뷰와 도메인이 얽히는 문제를 해결하는 방법 중 하나가 될 수 있을 것 같아요!!
4단계 로또 수동 리뷰 요청드립니다.
많은 피드백 부탁드립니다.
감사합니다.