-
Notifications
You must be signed in to change notification settings - Fork 5
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 코드리뷰 요청드립니다 #21
base: junwoochoi
Are you sure you want to change the base?
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.
수고하셨습니다!
크게 코멘트 달건 없었구요, 생각해 볼만한 것 적어봤습니다ㅎㅎ
|
||
public class WinningNumbers { | ||
public class WinningNumbers extends Lottery { |
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.
상속 적용하신것 👍
<tr> | ||
<th>3개 일치 (5000원)- 3개</th> | ||
<th>3개 일치 (5000원)- {{FIFTH_PRIZE}}개</th> | ||
</tr> | ||
<tr> | ||
<th>4개 일치 (50000원)- 1개</th> | ||
<th>4개 일치 (50000원)- {{SECOND_PRIZE}}개</th> | ||
</tr> | ||
<tr> | ||
<th>5개 일치 (1500000원)- 0개</th> | ||
<th>5개 일치 (1500000원)- {{THIRD_PRIZE}}개</th> | ||
</tr> | ||
<tr> | ||
<th>5개 일치, 보너스 볼 일치(30000000원)- 0개</th> | ||
<th>5개 일치, 보너스 볼 일치(30000000원)- {{SECOND_PRIZE}}개</th> | ||
</tr> | ||
<tr> | ||
<th>6개 일치 (2000000000원)- 0개</th> | ||
<th>6개 일치 (2000000000원)- {{FIRST_PRIZE}}개</th> |
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.
PRIZE가 4개 일치 까지만 계산하도록 변경됐을때, 결과 페이지도 수정하지 않는 방법이 있지 않을까요?
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.
흠... 사실 이부분은 찝찝한데 일단은 그냥 냈던거라... 더 좋은 방법을 강구해보겠습니다 ㅎㅎ
this.handlebars.registerHelper("length", new LengthHelper()); | ||
this.handlebars.registerHelper("plusOne", new PlusOneHelper()); |
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.
다들 커스텀 헬퍼를 잘 쓰셨군요 ㅠㅠ.. 저는 자바스크립트 단 코드만 주구장창 찾았는데.. 참고하도록 하겠습니다 !
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.
전체적으로 깔끔한 코드네요.
결과페이지만 조금 바꾸면 더 보기 좋아지겠네요!
고생하셨습니다.
responseParams.put(prize.name(), results.getCountOfPrize().getOrDefault(prize, 0)); | ||
} | ||
|
||
req.session().invalidate(); |
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.
저도 session 을 활용해봐야 겠네요! 배워갑니다
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.
코드 잘 보았습니다.
특별히 코멘트 달 부분은 없고 제가 참고할 부분이 있어서 참고하도록 하겠습니다 .!
this.handlebars.registerHelper("length", new LengthHelper()); | ||
this.handlebars.registerHelper("plusOne", new PlusOneHelper()); |
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.
수고하셨습니당!
dto
로 parsing
을 분리하신게 인상깊었습니당 👍
몇가지 코멘트를 남겨봤어욤 :)
|
||
import static view.InputView.COMMA; | ||
|
||
public class LotteryRequestDto { |
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.
오, 요렇게 dto를 분리해서, 파싱 로직을 분리한 부분이 인상깊네요! 배우고갑니당 :)
public class WinningNumbers extends Lottery { | ||
|
||
private List<LottoNo> winningNumbers; | ||
private LottoNo bonusNumber; | ||
|
||
private WinningNumbers(List<Integer> numbers, Integer bonusNumber) { | ||
validateParams(numbers, bonusNumber); | ||
protected WinningNumbers(List<Integer> numbers, Integer bonusNumber) { | ||
super(new TreeSet<>(numbers)); |
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.
한번 참고해보겠습니다 ㅎㅎ 감사합니다
커스텀 헬퍼를 적용해봤습니다...ㅎㅎ
생각보다 고칠부분이 많았네요....
많은 조언 충고 부탁드립니다...