-
Notifications
You must be signed in to change notification settings - Fork 1.1k
3단계 - 로또(2등) #4214
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단계 - 로또(2등) #4214
Conversation
javajigi
left a comment
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.
2단계 구현에서 객체 설계를 잘한 결과 큰 변경없이 3단계 기능 추가했네요. 👍
단, 기능 추가하면서 새로운 객체를 분리해 책임을 위임하는 것이 좋을지에 대해서고 고민해 보면 객체 설계 역량을 키우는데 더 도움이 될 것 같아요.
LottoNumber와 같은 불변 객체로 구현할 때 발생할 수 있는 문제 상황도 남겨봤으니 반영해 보면 좋겠습니다.
src/main/java/lotto/model/Lotto.java
Outdated
| if (lotto.contains(number)) { | ||
| return 1; | ||
| private void checkValidity(List<LottoNumber> numbers) { | ||
| if (numbers.size() != 6) { |
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.
6이 중복으로 사용되는데 상수로 빼는 것은?
| public class LottoNumber { | ||
| public class LottoNumber implements Comparable<LottoNumber> { | ||
| private final int number; | ||
|
|
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 LottoNumber(String number) |
문자열을 받는 생성자를 추가할 경우 의미있을까?
| import java.util.Objects; | ||
|
|
||
| public class LottoNumber { | ||
| public class LottoNumber implements Comparable<LottoNumber> { |
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.
상황
- 1만명의 사용자가 동시에 당첨 여부를 확인할 수 있어야 한다.
- 1명의 사용자는 평균 5장의 로또를 구매한 상태이다.
위 요구사항을 서버에서 생성되는 LottoNumber의 인스턴스의 갯수는?
1 * 5장 * 6개 숫자/1장 * 1만명 = 30만개이다.
동시에 생성되는 인스턴스 갯수가 너무 많다.
인스턴스 갯수를 줄일 수 있는 방법은?
로또 숫자 값을 LottoNumber 객체로 래핑하는 경우 매번 인스턴스가 생성되기 때문에 인스턴스의 갯수가 너무 많아져 성능이 떨어질 수 있다.
LottoNumber 인스턴스를 생성한 후 재사용할 수 있도록 구현한다.
힌트 : Map과 같은 곳에 인스턴스를 생성한 후 재사용하는 방법을 찾아본다.
| } | ||
|
|
||
| public LottoResults calculateResults(Lotto winningLotto) { | ||
| public LottoResults calculateResults(Lotto winningLotto, LottoNumber bonusNumber) { |
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.
Lotto winningLotto, LottoNumber bonusNumber 둘을 묶어 WinningLotto와 같은 객체로 분리해 보는 것은 어떨까?
이와 같이 구현할 경우 등수를 구하는 로직은 어디에 위치하는 것이 좋을까?
src/main/java/lotto/model/Lotto.java
Outdated
|
|
||
| private static List<Integer> generateRandomNumbers() { | ||
| private static Set<Integer> generateRandomNumbers() { | ||
| List<Integer> array = IntStream.rangeClosed(1, 45).boxed().collect(Collectors.toList()); |
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.
이와 같이 구현할 경우 45개의 값을 매번 반복 생성하게 된다.
45개의 값을 가지는 List 콜렉션을 생성한 후 재사용하도록 구현하면 어떨까?
- 당첨 번호와 보너스볼 번호 사이의 중복 불가 유효성 검증 추가 - 로또 몇등 당첨 확인 책임 WinningLotto 에 부여
- Set 타입으로 중복 제거에 대한 유효성 검증
- LottoNumber 객체는 범위가 지정되어 있으므로 매번 새로 생성하지 않고 인스턴스를 재사용하도록 변경
- Lottos 에 있던 계산 로직을 LottoResultCalculator로 이동
javajigi
left a comment
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.
피드백을 반영한 새로운 commit이 추가되지 않은 상태에서 리뷰 요청이 됐네요.
push가 정상적으로 됐는지 확인한 후에 다시 한번 리뷰 요청 주세요.
|
앗 커밋을 누락했네요.. 지금 추가했습니다..! |
javajigi
left a comment
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.
피드백 반영하느라 수고했어요. 👍
바로 머지하는 것보다 좀 더 객체 설계를 보완한 후에 4단계 진행하는 것이 좋을 것 같아 피드백 남겨봅니다.
LottoNumber의 경우 캐싱을 잘 적용했는데요. 약간은 아쉬움이 남는 부분이 있어 피드백 남겼어요.
| private final List<LottoNumber> numbers; | ||
| private final static int LOTTO_NUMBER_SIZE = 6; | ||
| private final static List<Integer> rangedInts = IntStream.rangeClosed(1, 45).boxed().collect(Collectors.toList()); | ||
| private final Set<LottoNumber> 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.
👍
| public class LottoNumber implements Comparable<LottoNumber> { | ||
| private final int number; | ||
|
|
||
| public LottoNumber(int number) { |
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으로 구현하고 있다.
이렇게 구현할 경우 LottoNumberFactory 사용법을 모르는 개발자의 경우 LottoNumber를 직접 생성할 수 있다.
다른 개발자가 LottoNumber를 직접하지 못하도록 막고, LottoNumber는 45개의 인스턴스만 생성해 재사용할 수 있도록 할 수 없을까?
힌트: 정적 팩터리 메서드
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| public class LottoNumberFactory { |
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 long getPrizeValue() { | ||
| return matchCounts.entrySet().stream() | ||
| .mapToLong(entry -> (long) entry.getValue() * Prize.fromMatchCount(entry.getKey()).value()) | ||
| public long getTotalPrizeValue() { |
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.
반환값도 원시 값을 포장해 보며 어떨까?
객체 이름이 PurchaseAmount이라 재사용함에 거부감이 있다.
그런데 Money와 같은 객체로 이름을 바꾼 후 재사용하는 것은 어떨까?
| public long getTotalPrizeValue() { | |
| public Money getTotalPrizeValue() { |
이와 같이 구현할 경우 수익률은 누가 계산하는 것이 맞을까?
| LottoResults lottoResults = new LottoResults(); | ||
| for (Lotto lotto : lottos.lottos()) { | ||
| lottoResults.update(winningLotto.calculatePrize(lotto)); | ||
| } | ||
| return lottoResults; |
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.
굳이 이 객체가 필요한가?
| LottoResults lottoResults = new LottoResults(); | |
| for (Lotto lotto : lottos.lottos()) { | |
| lottoResults.update(winningLotto.calculatePrize(lotto)); | |
| } | |
| return lottoResults; | |
| LottoResults lottoResults = lottos.match(winningLotto); |
이와 같이 Lottos에 메시지를 보내 위와 같이 구현해도 되지 않을까?
| @DisplayName("2등 당첨을 포함하지 않는 로또 당첨 결과를 계산한다.") | ||
| void getLottosResult() { | ||
| Lottos lottos = new Lottos( | ||
| Arrays.asList( |
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.
| Arrays.asList( | |
| List.of( |
위와 같이 구현도 가능함
Arrays.asList()와 List.of() 중 어느 api를 사용하는 것이 더 좋을까?
ai를 통해 확인해 본다.
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.
앗.. 이 부분 기본적인 건데 놓쳤네요 😂
| @Test | ||
| @DisplayName("로또 당첨 번호가 보너스볼의 번호를 포함하면 예외가 발생한다.") | ||
| void createWrongWinningLotto() { | ||
| assertThatIllegalArgumentException().isThrownBy(() -> new WinningLotto(new Lotto(1, 2, 3, 4, 5, 6), new LottoNumber(1))); |
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.
| assertThatIllegalArgumentException().isThrownBy(() -> new WinningLotto(new Lotto(1, 2, 3, 4, 5, 6), new LottoNumber(1))); | |
| assertThatIllegalArgumentException().isThrownBy(() -> new WinningLotto(new Lotto(1, 2, 3, 4, 5, 6), 1)); |
위와 같이 생성하도록 지원하면 어떨까?
|
이번 주에는 회사 워크샵 관계로 수업에 참여하지 못했는데요! Money 객체를 보니 다음부턴 객체를 어느 정도 수준까지 재사용할 수 있을지 고민을 좀 더 많이 하고 설계를 해야겠네요..! 감사합니다! |
javajigi
left a comment
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 캐싱을 위해 정적 팩터리 메서드 사용에 대해서는 앞으로도 성능 이슈가 있을 때 사용해 보면 좋겠어요.
Money로 이름을 바꾸고 적극적으로 활용한 모습이 인상적이네요.
4단계 구현할 때는 가능한 3단계 구조를 유지하면서 컴파일 에러를 최소화하면서 기능 추가해볼 것을 추천해 봅니다.
| static { | ||
| CACHE = new ConcurrentHashMap<>(); | ||
| IntStream.rangeClosed(1, 45).forEach(i -> CACHE.put(i, new LottoNumber(i))); | ||
| } |
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 PurchaseAmount { | ||
| private final int amount; | ||
| public class Money { |
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.
👍
안녕하세요!
아래 피드백 반영 및 로또 2등 당첨 기능 추가해보았습니다.
헷갈렸던 피드백
Lotto(Set<Integer> numbers))를 만들고 해당 생성자의 사용을 유도해서 중복 제거를 확인하도록 했습니다.감사합니다!