Step2 - 로또(자동) #1790
Step2 - 로또(자동) #1790
Conversation
|
#tag @neojjc2 |
src/main/java/lotto/LottoGame.java
Outdated
| Lottos lottos = new Lottos(); | ||
|
|
||
| int price = inputView.buyLotto(); | ||
| lottos.addLotto(inputView.getCount()); |
There was a problem hiding this comment.
addLotto를 통해 Lotto를 계속 추가할 수 있는 구조입니다 😄
우리가 실제로 로또를 사고 난 다음에 임의로 추가할 수 없는 것 처럼
로또 생성과 동시에 count만큼 생성되고 그 뒤로는 변경 할 수 없어야 할 것 같습니다 😄
마찬가지로 도메인 (객체) 가 가진 데이터는 소중하고 함부로 외부에서 변경할 수 있으면 안됩니다 😄
이 부분은 개선 검토 해주시면 감사하겠습니다 🙇
src/main/java/lotto/LottoGame.java
Outdated
| int price = inputView.buyLotto(); | ||
| lottos.addLotto(inputView.getCount()); | ||
|
|
||
| for(int i = 0; i < lottos.getSize(); i++) { |
There was a problem hiding this comment.
lottos에서 List<Lotto>를 불변 객체로 반환한다면
size를 가져와서 index 연산을 하지 않아도 될 것 같습니다 😄
foreach로 한번 개선해보시는 건 어떨까요?? 😅
이 부분도 개선 검토 부탁 드립니다 🙇
src/main/java/lotto/LottoGame.java
Outdated
|
|
||
| Collections.sort(correctNumbers); | ||
|
|
||
| for(int i = 0; i < lottos.getSize(); i++) { |
There was a problem hiding this comment.
로또 번호를 검증하는 부분 로또 구매 금액을 검증하는 부분들이
InputView, ResultView에 나뉘어져 있습니다 😅
InputView, ResultView는 사용자로 부터 입력 받고 콘솔로 출력하는 역할만 담당하게 하시고
로또 게임에 필요한 도메인들에게 해당 역할들을 적절히 위임해보시는건 어떨까요?? 🤔
재사용성과 응집성 그리고 추가요구사항에 용이하게 대처하고 최소한의 수정을 하기 위한 구조 변경이라 생각하시고
한번 개선 검토 해주시면 감사하겠습니다 😄
MVC 패턴에 대해서 한번 정리하고 가시면 좋을 것 같습니다 🙇
There was a problem hiding this comment.
변경을 해보았는데 의존성이 너무 크게 제가 수정을 한걸까 궁금하네요
| private int third = 0; | ||
| private int forth = 0; | ||
|
|
||
| public void correctCheck(Lotto lotto, List<Integer> correctNumbers) { |
There was a problem hiding this comment.
출력에 대한 역할과 당첨번호를 찾는 역할이 섞여 있는 구조 입니다 😅
위에서도 의견 드렸지만 현 구조에서는
출력에 대한 변경사항이 발생할 경우 당첨 번호를 찾는 로직도 영향을 받고
그 반대의 경우에도 출력에 대한 부분이 영향을 받을 수 있습니다
또한 추가적으로 다른 유형의 로또번호가 생기고
동일한 당첨 로직과 당첨 순위를 가져간다면
재 사용 측면에서도 현 구조에서는 적용이 어렵습니다 😄
일단 ResultView에서는 로또번호를 출력하는 역할만 하시도록
개선해보시기를 의견 드리고 싶습니다 😄
| @Test | ||
| @DisplayName("로또 금액 예외(천원보다 작을때)") | ||
| void priceExceptionTest2() { | ||
| InputView inputView = new InputView(); |
There was a problem hiding this comment.
로또 금액에 대한 테스트 코드인데 InputView를 통해서 테스트하는 부분이 어색합니다
구조 개선 검토 부탁 드리겠습니다 🙇
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class LottoMakerTest { |
There was a problem hiding this comment.
로또 당첨부분에 대한 테스트 코드도 누락 되어있습니다 😄
확인 한번 해주시면 감사하겠습니다 🙇
| @Test | ||
| @DisplayName("로또 금액 예외(천원단위가 아닐때)") | ||
| void priceExceptionTest() { | ||
| InputView inputView = new InputView(); |
There was a problem hiding this comment.
이 부분은 테스트에서 필요 없는 부분이라 없애셔도 될 것 같습니다 😄
| LottoMachine lottoMachine = new LottoMachine(); | ||
|
|
||
| @Test | ||
| @DisplayName("당첨 테스트") |
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class LottoValidate { |
There was a problem hiding this comment.
역할과 책임에 대해서 많이 정리가 됐습니다 👍
요청사항 반영해주시느라 정말 수고 많으셨습니다 😄
다만 로또 게임에 대한 검증역할을 하는 부분과
로또 게임의 결과를 (랭킹) 확인 하는 부분은 서로 분리가 되면 더 좋을 것 같습니다
검증과 결과를 확인하는건 도메인의 성격이 다르니까요 😄
다음단계 진행하시면서 이 부분에 대해서
개선 검토 해주시면 감사하겠습니다 🙇
|
|
||
| public Map<String, Object> lotteryRewards() { | ||
| Map<String, Object> map = new HashMap<>(); | ||
| map.put("first", first); |
There was a problem hiding this comment.
"first"와 같은 key 값은 상수로 추출 되여 명확한 의미를 갖도록 하는게 좋을 것 같습니다 😄
아니면 공통 변수에 선언해도 좋을 것 같은데요 😄
왜냐하면 lotteryRewards()를 호출하는 입장에서 보면 Map안에 어떤 key, value를 가지고 있을지 알기 어렵습니다
당첨 결과를 담당하는 객체를 만드시고 그 객체의 외부 method를 통해 "값을 달라고 해서 받을 수 있도록" 개선되거나
최소한 key값 만이라도 오타를 내거나 잘못된 값으로 인해 value 값을 가져가지 못하는 일은 없도록 해야 할 것 같습니다 😄
이 부분도 다시한번 개선 검토 부탁 드립니다 🙇
ex) map.get("frist"); --> null
|
|
||
| private void addRank(int checkCount) { | ||
| switch (checkCount) { | ||
| case 6: |
There was a problem hiding this comment.
요구사항 전체를 알고 있는 승재님이나 저와 같은 경우 이 6, 5, 4, 3 이 당첨된 번호 개수라는 것을 알지만
나중에 이 부분에 대해서 잘 모르는 개발자가 수정하려고 이 코드를 열었을 경우
6, 5, 4, 3의 의미는 정말 알기 어렵습니다 😅
최소 매직넘버가 추출 되거나 아니면 Enum을 활용해보시는 건 어떠실까요??
명시적 비교가 가능할 것 같습니다 😄
이 부분도 추가 개선 검토 부탁 드립니다 🙇
도움되실 만한 내용 공유 드립니다 🙇
리뷰 부탁드립니다!