[1단계 - 로또 구현] 케빈(박진홍) 미션 제출합니다. #228
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
필드변수명 변경에 따라 getter 또한 변경한다
등수에 따른 개수라는 의미를 직관적으로 살리기 위해 변경한다
자동으로 티켓을 발권한다는 역할을 메서드명에 드러나도록 변경
- 각 상수들의 접두로 PRIZE를 추가한다. - 상금 상수의 가독성을 위해 언더바를 추가한다.
counts가 아닌 ticketCounts라고 명시한다.
- 각 메서드가 어떤 내용을 출력하는지를 명시해준다. - 또한 반복문을 삭제하고 람다 스트림을 이용한다.
학습 로그[JDK] Stream API - 4내용
링크[JDK] Map API - 2내용
[Design Pattern] Singleton - 3내용
링크[OOP] 표준 vs Custom Exception - 3내용
링크[OOP] 상속보단 컴포지션을 - 5내용
링크[JDK] EnumMap - 2내용
링크[TDD] 테스트 주도 개발 - 3내용
[MVC] DTO - 3내용
링크 |
hyunssooo
requested changes
Feb 20, 2021
Comment on lines
+19
to
+21
| public static LottoNumber from(int number) { | ||
| return CACHE.computeIfAbsent(number, LottoNumber::new); | ||
| } |
Comment on lines
5
to
7
| public interface LottoNumberGenerator { | ||
| public List<Integer> generate(); | ||
| } |
Comment on lines
25
to
31
| public LottoStatistics getStatistics(WinningLottoTicket winningLottoTicket) { | ||
| Map<LottoRank, Long> statistics = lottoTickets.stream() | ||
| .map(winningLottoTicket::compareNumbers) | ||
| .collect(Collectors.groupingBy(lottoRank -> lottoRank, () -> new EnumMap<>(LottoRank.class), | ||
| Collectors.counting())); | ||
| return new LottoStatistics(statistics); | ||
| } |
Comment on lines
19
to
23
| public double calculateYield(PurchasingPrice purchasingPrice) { | ||
| long prizeMoneyTotal = calculatePrizeMoneyTotal(); | ||
| int denominatorCost = purchasingPrice.getPrice(); | ||
| return (((double) prizeMoneyTotal) / denominatorCost); | ||
| } |
There was a problem hiding this comment.
뷰에서 사용되는 로직이네요. 뷰에서 도메인 객체에게 메세지를 보내는 것에 대해서 고민해보면 좋을 거 같아요 :)
뷰가 웹으로 바뀐다면 어떻게 될까요?
Comment on lines
17
to
21
| private LottoRank(int matchCounts, boolean isBonusBall, int prizeMoney) { | ||
| this.matchCounts = matchCounts; | ||
| this.isBonusBall = isBonusBall; | ||
| this.prizeMoney = prizeMoney; | ||
| } |
There was a problem hiding this comment.
enum은 생성자를 디폴트로 사용합니다! private일 필요는 없어요!
Suggested change
| private LottoRank(int matchCounts, boolean isBonusBall, int prizeMoney) { | |
| this.matchCounts = matchCounts; | |
| this.isBonusBall = isBonusBall; | |
| this.prizeMoney = prizeMoney; | |
| } | |
| LottoRank(int matchCounts, boolean isBonusBall, int prizeMoney) { | |
| this.matchCounts = matchCounts; | |
| this.isBonusBall = isBonusBall; | |
| this.prizeMoney = prizeMoney; | |
| } |
Comment on lines
15
to
17
| public long getTicketCountsByRank(LottoRank lottoRank) { | ||
| return statisticsMap.computeIfAbsent(lottoRank, key -> ZERO); | ||
| } |
Comment on lines
+34
to
+41
| @DisplayName("로또 번호는 싱글톤 객체이다") | ||
| @Test | ||
| void isSingleton() { | ||
| LottoNumber lottoNumber = LottoNumber.from(1); | ||
| LottoNumber target = LottoNumber.from(1); | ||
|
|
||
| assertThat(lottoNumber).isEqualTo(target); | ||
| } |
There was a problem hiding this comment.
LottoNumber는 value object로 만들 수도 있겠네요!
https://woowacourse.github.io/javable/post/2020-06-11-value-object/
Comment on lines
12
to
13
| private final List<LottoNumber> lottoNumbers; | ||
|
|
Comment on lines
+40
to
+42
| public boolean contains(LottoNumber lottoNumber) { | ||
| return lottoNumbers.contains(lottoNumber); | ||
| } |
There was a problem hiding this comment.
LottoNumber 객체에게 같은 숫자인지 물어보는 방법은 없을까요??
There was a problem hiding this comment.
public boolean contains(LottoNumber lottoNumber) {
return lottoNumbers.stream()
.anyMatch(lottoNumberOfTicket -> lottoNumberOfTicket.hasSameNumber(lottoNumber);
}같은 형태의 코드를 말씀하시는 것일까요? 해당 부분의 피드백이 잘 이해가 되지 않네요!
There was a problem hiding this comment.
public class LottoNumber {
..
public boolean match(LottoNumber LottoNumber) {
return this.value == value;
}
}이런 형태로 LottoNumber에게 메세지를 보낼 수도 있을거 같아서 남긴 피드백이였습니다!
Comment on lines
34
to
38
| public int getMatchCounts(LottoTicket lottoTicket) { | ||
| return (int) lottoNumbers.stream() | ||
| .filter(lottoTicket::contains) | ||
| .count(); | ||
| } |
- 열거형 생성자는 기본 default를 사용한다. - 인터페이스 추상 메서드의 접근 제어자의 public은 생략한다.
- getStatistics 등의 메서드 명 또한 checkResult로 직관되게 변경
- 구입가격 객체는 양의 정수만을 허용하도록 변경한다.
도메인 객체로 메시지를 보내던 기존 방식을 외부에서 주입받은 수익률을 바로 출력하도록 변경한다.
LottoTicket은 Set이기 때문에 정렬을 View단으로 넘긴다
|
안녕하세요. 피드백을 반영하는 과정에서 코드의 구조가 다소 많이 변경되었네요.
중복을 허용하지 않는 자료구조 피드백에 대해서 Set을 적용하도록 코드를 변경했습니다. 그리고 지적해주신 메서드 네이밍에 대해 고민해보면서, 기타 직관적이지 않은 클래스 및 메서드 네임들에 대해 전부 리팩토링해보았습니다. (예 : LottoStatistics -> LottoResult 등) 또한 뷰에서 도메인 객체에게 메시지를 보내는 것에 고민해보았는데요. 말씀하신대로 요구사항이 웹으로 바뀌면... 기껏 도메인에서 뷰에 필요한 정보들(DTO, JSON 등)을 만들어 보내줬는데 뷰가 도메인쪽으로 추가 정보를 달라고 다시 요청하는(?) 이상한 형태가 되네요. View에서 도메인 객체에게 메시지를 보내 그 때 그때 필요한 정보를 받기보다는, 도메인 객체가 제공하는 DTO 성격의 데이터를 파싱하여 View를 보여주는 방식으로 변경해보았습니다! |
hyunssooo
approved these changes
Feb 21, 2021
Comment on lines
+44
to
+51
| public int getPurchasingPrice() { | ||
| return lottoTickets.size() * LOTTO_TICKET_PRICE; | ||
| } | ||
|
|
||
| public int getTicketCounts() { | ||
| return lottoTickets.size(); | ||
| } | ||
|
|
Comment on lines
+44
to
+46
| public List<LottoNumber> getLottoNumbers() { | ||
| return Collections.unmodifiableList(new ArrayList<>(lottoNumbers)); | ||
| } |
There was a problem hiding this comment.
getter 메서드가 내부와 다른 자료구조를 던져주네요!
Suggested change
| public List<LottoNumber> getLottoNumbers() { | |
| return Collections.unmodifiableList(new ArrayList<>(lottoNumbers)); | |
| } | |
| public Set<LottoNumber> getLottoNumbers() { | |
| return Collections.unmodifiableSet(lottoNumbers); | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요.
스트림, 람다 등을 연습하고자 반복문 사용을 최대한 지양했는데 다소 억지스럽거나 가독성이 떨어진다면 지적해주세요!
리뷰잘부탁드리겠습니다. :D