[그리디] 이태규 로또 미션 1,2단계 제출합니다.#156
Conversation
There was a problem hiding this comment.
안녕하세요 태규님!
이번 미션도 수고 많으셨어요~ 코드에 고민한 흔적이 많이 보이네요👍👏
PR에 고민한 내용 또한 구체적으로 남겨주셔서 리뷰 남기기 수월했습니다!
이해 안되는 부분이나 질문이 있다면 편하게 말해주세용 잘 부탁드립니다🙌
✏️ 코드 스타일에 관한 피드백
현재는 메서드 순서가 조금 섞여있는 것 같아요!
https://dodop-blog.tistory.com/277
해당 블로그를 보고 메서드 순서를 통일해주시면 조금 더 읽기 쉬운 코드가 될 것 같습니다! 특히
메소드 : 접근자 기준으로 작성하지 않고, 기능 및 역할별로 분류하여 기능을 구현하는 그룹별로 작성이 이루어지도록 해야한다. (public 사이에 private 메소드가 존재할 수 있다)
요 부분을 신경써주시면 좋을 것 같아요. 현재는
B메서드(){
B메서드 호출
}
A메서드(){
B메서드 호출
}
이렇게 작성된 부분이 조금 있는데 코드의 흐름을 자연스럽게 하기위해서는
A메서드(){
B메서드 호출
}
B메서드(){
B메서드 호출
}
이렇게 수정해주시면 좋을 것 같아요!
🤔 QnA
1. 매직넘버 관리
Magic Number등 끼리 약하게나마 서로 연관성이 있으면 어떻게 관리하는 것이 좋을까요?
연관이 있다는건 무슨의미일까요?!
일단 매직넘버 관리에대해 현재는 LottoSettingsConstants라는 클래스에 변수들을 모아두셨더라구요! 이렇게 하나의 클래스 내부에 모아두게되면, 변수를 모든 클래스에서 재사용하기 쉬워, 상수 조건이 바뀌었을 때 이곳저곳 수정할 필요 없이 하나의 클래스만 확인 및 수정하면 된다는 편리함이 있습니다.
다만 저는 개인적으로 그냥 각 클래스 내부에서 한눈에 확인하는 것이 편하다 생각해, 각각의 클래스 상단에 해당 클래스에 필요한 상수들을 final static 선언해두는 방법을 더 선호하긴합니다!
이 부분은 취향차이라서 이대로 진행하셔도 좋을 것 같아요!
2. Lotto와 LottoFactory의 분리
저는 작성하신 구조가 아주 좋아보입니다! 로또 생성의 책임을 두 군데에서 담당한다기보다는, LottoFactory에서 생성 책임을 담당하고, Lotto는 생성된 값을 표현하는 역할에 집중하여 적당한 책임분리가 이루어진 것 같아요👍
3. 도메인 설계 과정
저는 먼저 Readme에 기능 목록을 쭉 작성한 뒤 플로우대로 차근차근 하나씩 기능을 구현하는 편이에요.
태규님은 Readme의 기능 요구사항에 간단한 요구사항만 적어주셨지만, 더 구체화 시켜보면 어떨까요?
- [ ] 로또 구입 금액을 입력한다.
- [ ] 로또 금액은 ~~ 이어야한다. (조건)
- [ ] 로또 ~~ 를 출력한다.
- [ ] 지난 주 당첨 번호를 입력한다.
- [ ] (조건)
- [ ] 당첨 통계를 출력한다.
- [ ] (조건1)
- [ ] (조건2) ...
위와 같이 태규님께서 이번 미션을 진행하며 고려할 조건 및 흐름을 작게 잘라 구체적으로 적어놓은 뒤 구현하면, 여러 기능을 한번에 구현할 일은 없을 것 같아요!
(new!) 4. controller 분리
현재 태규님은 controller를 LottoPurchaseController와 LottoResultCalculatorController로 나누어 작성해주셨어요. 로또를 구매하는 로직과 계산하는 단계를 분리함으로써 단계적으로 코드를 이해할 수 있다는 장점이 있는 것 같아요!
하지만 Main 클래스를 보면, 같은 lottoBatch를 인자로 받아 사용하고 있는데, 결국에는 같은 하나의 객체를 두고 절차만 두개의 클래스로 나누어 작성한 것 처럼 보일 수 있을 것 같아요. 따라서 하나의 컨트롤러에 구매 / 계산 로직을 2개의 메서드로 나누어 구현하는 방법도 있을 것 같네요!
해당 부분은 수정이 필요하다는 것은 아니고 컨트롤러를 나누었을 때의 이점에대해 한번 더 고민해보셨으면해서 남겨보았습니다!
이번 미션 파이팅입니다!🔥
|
|
||
| import static java.lang.Math.min; | ||
|
|
||
| public class LottoNumberGenerator implements NumberGenerator { |
There was a problem hiding this comment.
👍
인터페이스를 implement 한 이유가 무엇일까요? (인터페이스의 장점)
바로 랜덤 상수를 만드는 클래스를 만드는 방법도 있을텐데, 인터페이스의 구현체로 해당 클래스를 만드신 이유가 궁금합니다!
There was a problem hiding this comment.
랜덤성이 있는 부분은 최대한 코드에거 고립시켜 관리해야한다고 생각하기 때문입니다. 랜덤성이 있는 부분은 최대한 그 부분만 다른 방식으로 대체 될 수 있도록 구현하는 것이 보다 유지보수하기 수월한 코드가 될 것 같아 인터페이스로 구현하게 되었어요.
There was a problem hiding this comment.
태규님께서 정의하신 LottoBatch의 역할은 무엇인가요?
Readme에는 구매한 여러 개의 로또들을 관리하는 일급 컬렉션이라고 정의해주셨는데, 당첨 결과까지 계산하고있어 조금 많은 책임을 가지고있는 것 같아요 🥲
There was a problem hiding this comment.
일급 컬랙션의 역할은 로또들을 관리하면서, 로또가 여러개 있을 때 유의미한 계산 (평균, 최댓값 등)을 계산하는 것이라고 생각을 했습니다.
하지만 코드를 읽어보니 당첨 결과를 계산하는 것은 외부의 입력값(당첨번호)의 개입이 필요하니, 이 계산은 컨트롤러가 하는 것이 더 좋겠다는 생각이 들어 수정하였습니다!.
src/main/java/model/LottoBatch.java
Outdated
| LottoSettingsConstants.FIVE_MATCH_PRICE, | ||
| LottoSettingsConstants.SIX_MATCH_PRICE | ||
| ); | ||
| List<Lotto> lottos; |
There was a problem hiding this comment.
접근제어자가 빠진 것 같아요!
접근제어자를 설정해야하는 이유는 무엇일까요?
There was a problem hiding this comment.
헉, 이런걸 놓쳤네요.... 다음부턴 사소한 실수가 없도록 더 노력할게요...
접근제어자는 외부로 부터 원치 않은 접근을 통해 해당 상태가 오염되거나 원치 않은 메소드 호출을 막아주기에 꼭 필요하다고 봅니다. 또한 코드를 읽는 입장으로서, 이 코드가 어디에 사용될 수 있는지에 대한 의도도 대략적으로 주기에 가능하면 꼭 있으면 좋겠다라는 생각이 드네요.
| public void purchase() { | ||
| int userCashInput = inputView.getUserCashInput(); | ||
| checkPriceHigherThanSingleLottoPrice(userCashInput); | ||
|
|
||
| int lottoCount = userCashInput/ LottoSettingsConstants.LOTTO_PRICE; | ||
| for (int i = 0; i < lottoCount; i++) { | ||
| getLotto(); | ||
| } | ||
|
|
||
| List<LottoDto> lottoDtos = lottoBatch.getAllLotto().stream() | ||
| .map(this::wrapLottoIntoDto).toList(); | ||
|
|
||
| outputView.printPurchaseResult(lottoDtos); | ||
| } |
There was a problem hiding this comment.
태규님이 생각하시는 controller의 역할은 무엇인가요?
저는 해당 컨트롤러가 도메인의 역할도 하고있는 것 같아요!
checkPriceHigherThanSingleLottoPrice(입력 받은 금액에 대한 검증)int lottoCount = userCashInput/ LottoSettingsConstants.LOTTO_PRICE;(구매 개수 계산)for (int i = 0; i < lottoCount; i++)로또 반복 생성- dto 변환
너무 많은 책임을 가지고 있는 것 같은데, 도메인 각 기능에 대해 해당 기능은 어느 도메인 혹은 계층이 가져야할지 고민해보시면 좋을 것 같아요!
There was a problem hiding this comment.
저는 컨트롤러가 두 가지 역할을 해야 한다고 생각합니다
- 필요한 함수들을 호출하는 중재자 역할 및 일부 비즈니스 로직 실행
- 입력/출력에 대한 적절한 데이터 정제입니다.
그래서 입력값 검증, 받은 금액만큼 로또를 반복 생성하는 로직, 그리고 DTO로 변환하는 과정은 컨트롤러가 담당하는 것이 맞다고 보았습니다.
다만, 현재처럼 하나의 메소드가 그 모든 책임을 가지는 것은 문제가 있다고 생각했고, 이에 따라 로또를 반복 생성하는 부분은 별도의 메소드로 분리하여 책임을 나누었습니다.
There was a problem hiding this comment.
태규님이 작성해주신 컨트롤러의 역할에 대해 잘 읽어봤습니다!
- 필요한 함수들을 호출하는 중재자 역할 및 일부 비즈니스 로직 실행
컨트롤러는 어플리케이션의 흐름을 관리하는 역할이기에 요 의견에 동의합니다.
(다만 비즈니스 로직 실행이라는 말이 조금 모호한 것 같은데, 비즈니스 로직 실행 == model 의 메서드를 적절히 불러온다 라는 의미라면 맞는 것 같고, 만일 비즈니스 로직을 직접 작성하는 것이라면 model 계층의 역할과 섞인 것 같아요)
- 입력/출력에 대한 적절한 데이터 정제입니다.
하지만 입력/출력 로직의 호출이 아닌 데이터 정제 부분에 대해서는 저는 조금 다른 의견을 가지고있어요!
컨트롤러에서는 도메인과, view의 흐름만 통제해야지, 구체적인 비즈니스 로직에 대해서는 알고있으면 안된다고 생각해요.
그전에 model 계층의 역할에 대해 한번 정의하고 넘어가보겠습니다! 찾아보니 "model은 애플리케이션의 핵심 데이터와 비즈니스 로직을 나타낸다."라고 하네요.
🤔💭 만일 로또의 가격이 1000원에서 500원으로 바뀌었다거나, 로또 가격 검증 조건이 추가 및 수정 되었다고 가정해보겠습니다!
현재 코드에서는 controller 계층이 수정되겠네요! 하지만 mvc 패턴에서 각 계층의 정의에의하면 비즈니스 로직을 담고 있는 것은 model 계층이기때문에, 저는 model 부분 코드를 먼저 읽어볼 것 같아요.
저는 이러한 이유로 검증로직을 도메인 부분으로 넘겨야한다고 생각하는데, 태규님은 요 의견에대해 어떻게 생각하시나요?
만일 옮겨야하는 적절한 도메인 클래스가 없다면, 해당 값은 단순한 원시타입의 숫자나 값이 아닌 하나의 역할을 하는 객체로 포장이 필요했던것은 아닐까요?!
There was a problem hiding this comment.
컨트롤러에서는 도메인과, view의 흐름만 통제해야지, 구체적인 비즈니스 로직에 대해서는 알고있으면 안된다고 생각해요.
이 부분이 저에게 컨트롤러가 어떤 역할을 해야하는지 잘 정리해준 것 같습니다... 확실히 특정 비즈니스 로직이 컨트롤러에 있는 설계는 것 자체가 객체지향적이지 않은 것 같네요. 해당 부분을 참고하여 LottoResultCalculatorController를 수정해 보았습니다.
| protected void getLotto() { | ||
| Lotto lotto = this.lottoFactory.generateLotto(); | ||
| this.lottoBatch.add(lotto); | ||
| } |
There was a problem hiding this comment.
getLotto는 일반적으로 Lotto 정보를 반환할 때 사용하는데, 해당 메서드에서는 lotto를 추가하고있네요!
메서드 명 수정이 필요해보입니다~
src/main/java/model/LottoBatch.java
Outdated
| } | ||
|
|
||
| public List<Integer> getMatchCountPerLotto(List<Integer> winningNumbers) { | ||
| this.checkIfNumbersAreValid(winningNumbers); |
There was a problem hiding this comment.
getReturnRatio에서 checkIfNumbersAreValid 검증 된 채로 checkIfNumbersAreValid에 들어오게 되는데, 위 검증 로직은 중복이된 것 같아요!
There was a problem hiding this comment.
검증에 대한 역할을 고민하면서 수정했습니다! 이제 검증의 책임은 Lotto 객체에서 처리하는 것으로 바꿨습니다.
src/main/java/model/LottoBatch.java
Outdated
| private int countMatches(List<Integer> lottoNumber, List<Integer> winningNumbers) { | ||
| this.checkIfNumbersAreValid(winningNumbers); | ||
| Set<Integer> lottoNumberSet = new HashSet<>(lottoNumber); | ||
| Set<Integer> winningNumberSet = new HashSet<>(winningNumbers); | ||
| lottoNumberSet.retainAll(winningNumberSet); | ||
|
|
||
| return lottoNumberSet.size(); | ||
| } |
There was a problem hiding this comment.
getMatchCountPerLotto에서 lotto.getNumbers()로 Lotto 필드값을 직접 꺼내 정답 로또와 몇개 일치하는지를 비교하고있네요!
- 일단 객체 외부에서 필드값을 꺼내 로직을 수행하는 것은 위험해요! 비슷한 이유로 getter를 지양하라고하는데, 그 이유가 무엇일까요?
- 해당 로직은 LottoBatch가 아닌 다른 클래스로 옮길 수 있을 것 같아요. 어디로 옮기면 객체가 스스로의 책임을 다할 수 있을까요?
There was a problem hiding this comment.
-
제가 이해한 바로는 getter를 사용하여 필드값을 꺼내면, 특히 해당필드가 원시값이 아닌경우, 필드가 외부의 영향에 노출되기 때문이라고 알고 있습니다. 하지만 이 문제는 해당 필드를 deep copy 하면 될 것 같아 큰 문제는 아닌것 같아요. 그리도 어떤 자료들은 "getter에 의존하는 코드가 많아져 독립성을 해친다"라고 설명읗 하는데, 이는 getter를 지양하는 것 보단 getter를 왠만하면 쓰지 말자라고 하는 것이 더 맞는 방향인 것 같아요.
-
조금 고민해 봤는데, 이를 ENUM에서 해결할지, Lotto 에서 해결할지 고민했습니다. 일단 도메인의 조건을 확인한다는 측면에서 Lotto에 책임을 전가하자는 쪽으로 마음이 기울었는데, 이는 해윤님은 어떻게 생각하는지 궁금해요.
- ENUM을 계산하는 것은 ENUM에서 처리하는 것이 더 자연스러울까요?
- 혹시나 해서 제가 ENUM을 ENUM내부에서 계산하는 방식을 주석처리 했습니다. 시간이 되면 그 부분도 한 번 살펴주시고, 해당 구현 방법은 어떤지 의견을 공유해주시면 감사하겠습니다.
- 아니면 Lotto에서 필요한 ENUM을 계산하는 것이 더 적절한 방식일까요?
- ENUM을 계산하는 것은 ENUM에서 처리하는 것이 더 자연스러울까요?
There was a problem hiding this comment.
- 제가 이해한 바로는 getter를 사용하여 필드값을 꺼내면, 특히 해당필드가 원시값이 아닌경우, 필드가 외부의 영향에 노출되기 때문이라고 알고 있습니다.
👍👍👍
- 조금 고민해 봤는데, 이를 ENUM에서 해결할지, Lotto 에서 해결할지 고민했습니다. 일단 도메인의 조건을 확인한다는 측면에서 Lotto에 책임을 전가하자는 쪽으로 마음이 기울었는데, 이는 해윤님은 어떻게 생각하는지 궁금해요.
저도 단순히 두 로또를 비교해 몇 개 일치하는지를 반환하는 메서드라면 Lotto에서 진행하는 것이 맞다고 생각합니다!
하지만 말씀하시대로 맞춘 개수를 기반으로 Enum을 반환하는 것은 또 Enum의 책임이라는 생각이 들어요.
그렇다면 해당 메서드가 너무 많은 역할을 하기때문에 이런 고민이 드셨을 수 도 있어요!
로직을 조금 나누어서
- 로또 개수 비교
- 일치 개수에따른 Enum 반환
두 단계로 나누고 두 메서드를 호출해 하나의 흐름으로 묶어도 좋을 것 같네요~
There was a problem hiding this comment.
이를 통해 역할 분배가 애매하면 차라리 역할을 조금 더 쪼개 보는 것도 나쁘지 않은 방법이라는 것을 알게 되었습니다. 참고하고 수정했습니다!
src/main/java/model/LottoBatch.java
Outdated
| double earnResult = 0.0; | ||
| for (int i = 0; i < WINNING_COUNT.size(); i++) { | ||
| int currentCount = WINNING_COUNT.get(i); | ||
| int totalCount = result.stream().filter(matchCount -> currentCount == matchCount).toList().size(); |
There was a problem hiding this comment.
long totalCount = result.stream()
.filter(matchCount -> currentCount == matchCount)
.count();
리스트의 요소 개수 만을 세는 것이라면, count()를 사용해 의도를 명확하게 할 수 있을 것 같아요!
There was a problem hiding this comment.
앗 몰랐던 stream 함수네요.... 참고하겠습니다.
src/main/java/model/LottoBatch.java
Outdated
| private final List<Integer> WINNING_COUNT = List.of(3,4,5,6); | ||
| private final List<Integer> WINNING_PRICE = List.of( | ||
| LottoSettingsConstants.THREE_MATCH_PRICE, | ||
| LottoSettingsConstants.FOUR_MATCH_PRICE, | ||
| LottoSettingsConstants.FIVE_MATCH_PRICE, | ||
| LottoSettingsConstants.SIX_MATCH_PRICE | ||
| ); |
There was a problem hiding this comment.
현재 WINNING_COUNT와 WINNING_PRICE를 각각 변수로 생성한 뒤 아래에서 인덱스를 사용해 연결시키고 있는 것으로 보여요! 인덱스를 사용해 매칭을 하게되면 실수할 확률이 높아질 것 같아요!
연관있는 두 상수를 묶어 사용할 수 있는 방법이 있습니다.
- Map을 사용하여 묶기
- Enum 사용하기 ⭐️⭐️⭐️
근데 지금보니 아직 스터디에서 다루지 않은 내용이네요 ...ㅎ 해당 부분은 스터디 후 수정해보시면 좋을 것 같아요!
There was a problem hiding this comment.
코드 리뷰를 보고 ENUM에 대해 공부했습니다. 자바 ENUM 엄청 강력하네요
src/main/java/model/LottoBatch.java
Outdated
| this.checkIfDuplicateExist(numbers); | ||
| this.checkIfNotEnoughNumbers(numbers); | ||
| this.checkIfTooManyNumbers(numbers); | ||
| this.checkIfNumbersAreInRange(numbers); |
There was a problem hiding this comment.
this는 의도적으로 계속 쓰고 있습니다. 아무래도 이 변수는 이 스코프에서만 쓰이는 변수인가 아니면 클래스의 필드인가 가끔씩 햇갈리더라고요....
혹시 this를 너무 많이 남발하면 코드가 덜 깔끔해 보일까요...? 혜윤님은 어떻게 생각하시나요..?
There was a problem hiding this comment.
생성자에서는 필수적이긴한데, 저는 그냥 그 외의 부분에는 안쓰고있어요😅 깔끔한게 좋아서 ....
근데 헷갈리신다면 필드에 대해서는 일단 하던대로 사용하시는게 좋을 것 같지만, 메서드에서는 보통 사용하지않는 편입니다!
|
다음과 관련해서 몇 가지 궁금한 점이 있습니다.
|
|
리뷰를 모두 잘 반영해주네요!👍 몇 가지 새로운 리뷰와 답변 남겨두었으니 확인부탁드려요~ 추가로 주석 처리된 메서드나, 빈 테스트문 등은 삭제해주세요!
Zero, One, Two를 삭제하고 일치개수가 3개인 경우부터 저장하는 방법도 있지만, 요 부분은 원하시는대로 하셔도 좋을 것 같아요! 다만 이렇게 될 경우 현재는 outputview에 출력해야할 값들을
검증의 책임은 객체에 두는 것이 적절하다고 생각해요! 또한 해당 객체는 객체 내부에 정의된 규칙을 만족한 믿을 수 있는 객체구나~ 라고 믿고 사용하는 것은 당연한 사고방식인 것 같아요! 하지만 현재는 검증의 책임이 일부 컨트롤러에도 작성되어있던데 이부분은 수정이 필요해보입니다!
이런 의미에서의 연관성이라면 사실 어쩔수없는 문제인 것 같아요😅 일치 개수에 따라 당첨 금액이 달라지는 것은 당연한 부분이기도하고, 그것까지가 비즈니스 요구사항이기때문에 해당부분을 개선할 방법이 딱히 떠오르지는 않네요🤔 매직넘버에대한 검증이라는 것은 상수화한 상수에 대한 값을 믿고 사용하는 것은 괜찮은 것 같아요. 상수화를 하는 이유가 무엇일까요? 저는 이곳 저곳에서 반복적으로 숫자로 값을 하드코딩해 사용하면 어떤 곳에서는 실수로 다른 값을 입력할 수 있기때문도 있다고 생각해요.
해당 질문에 답변을 드리기전에 태규님은 어떤 경우에 Mock객체를 만드셨나요? Mock객체란 왜 사용하는 걸까요? |
There was a problem hiding this comment.
dto는 모델 외 다른 부분들이 모델의 구현과 별개로 필요한 정보를 전달 받을 수 있도록 해줄 수 있는 포장지 같은 클래스로 이해하고 있습니다. 예를 들어 OutputView' 의 printStats`은 모델이 어떤 방식으로 로또의 결과를 계산하는지 모르지만, 결과를 Map 형식으로 받아와 결과를 출력합니다.
지금은 모델이 단순해 전달해야하는 정보가 적지만, 더 복잡한 모델을 가진 시스템에서는 뷰가 필요한 정보만을 컨트롤러를 통해 전달 받을 수 있게 해주어 편리하기에 확장성을 고려하여 dto를 적용해봤어요.
| private final List<Integer> numbers; | ||
| private int index = 0; |
There was a problem hiding this comment.
LottoNumberGenerator는 단순히 랜덤값을 생성하는 util 클래스처럼 보입니다!
해당 클래스가 필드값을 가질 필요가 있을까요??
There was a problem hiding this comment.
이는 LottoNumberGenerator의 구현 방식에 의해 필요하다고 봅니다!
저는 어떻게 하면 중복되지 않는 숫자를 반환할까 고민해봤는데, 가장 확실한 방법은 1부터 45까지 있는 리스트를 무작위로 섞은 뒤, 처음 몇개를 뽑는 방식으로 구현하였습니다!
해당 구현체는 도메인(로또)를 생각하고 구현되었고, 오로지 도메인에서만 활용되므로 도메인 페키지로 옮겼습니다!
| LottoBatch lottoBatch = new LottoBatch(new ArrayList<Lotto>()); | ||
| NumberGenerator numberGenerator = new LottoNumberGenerator(); | ||
| LottoFactory lottoFactory = new LottoFactory(numberGenerator); | ||
| InputView inputView = new InputView(new Scanner(System.in)); |
There was a problem hiding this comment.
저는 이렇게 의존하는 객체는 최대한 외부로부터 받아오는게 저 좋을것 같아 위처럼 구현했어요. 아무래도 이 방식대로 구현하면 테스팅하기도 좋고, 의존하는 객체가 의존되는 객체의 생성 과정을 몰라도 되어 개발할 때 더 수월하다는 점들 때문에, 왠만하면 의존하는 객체는 외부로부터 받아오고 있도록 개발하는 방향이 더 바람직해 보입니다...!
src/main/java/Main.java
Outdated
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| LottoBatch lottoBatch = new LottoBatch(new ArrayList<Lotto>()); |
There was a problem hiding this comment.
생성 시 빈 리스트를 주입하는 것이 아닌, 생성자에서 초기화하는 방식으로 바꿔보는 것은 어떨까요?
There was a problem hiding this comment.
위 리뷰의 답변과 비슷한 맥락으로 구현하긴 했는데, 아무래도 일급 컬랙션은 비어있는 상태로 생성되는게 더 자연스러운것 같습니다! 수정했습니다!
src/main/java/view/OutputView.java
Outdated
| System.out.println(ScriptConstants.OUTPUT_STAT_HEADER_SCRIPT); | ||
|
|
||
| for (LottoResult currentResult : WINNING_RESULT) { | ||
| int resultCount = (int) matchCountPerLotto.stream().filter(result -> currentResult == result).count(); |
There was a problem hiding this comment.
계산을 하는 로직은 outputview에서 이루어지면 안될 것 같아요!
There was a problem hiding this comment.
이 부분이 계산하는 것은 인자로 받은 LottoResult 리스트에서 출력해야할 LottoResult를 카운팅하는 것이기에 outputview의 역할에 크게 벗어나지 않아보입니다.
다만 이렇게 될 경우 현재는 outputview에 출력해야할 값들을
List<LottoResult> WINNING_RESULT = List.of(LottoResult.THREE, LottoResult.FOUR, LottoResult.FIVE, LottoResult.SIX);이렇게 하드코딩 해 넣어주고있는데요, 도메인의 책임을 view가 조금 알게된다는 단점이 있을 것 같아요. 이 부분은 해당 로직을 다른 부분으로 옮김으로써 해결할 수 잇을 것 같습니다~
하지만 위 리뷰를 반영하며 코드를 수정하면서 위 로직이 자연스럽게 컨트롤러로 빠지네요...
src/main/java/model/LottoResult.java
Outdated
| public final int matchCount; | ||
| public final int reward; |
There was a problem hiding this comment.
원시값에 final이라 별 생각 없이 public으로 접근제어자로 두었습니다. 제가 알기론 이런 형태에서는 외부로 노출되어도 상태가 오염될 여지도 없는 것으로 알고 있어요. 혹시 이 상화에서 public으로 접근제어자를 사용하면 또다른 위혐이 있을까요...?
질문 별개로, 일단 private 및 getter를 사용하는 방식으로 수정하였어요!
src/main/java/view/OutputView.java
Outdated
| System.out.println(); | ||
| } | ||
|
|
||
| public void printStats(List<LottoResult> matchCountPerLotto) { |
There was a problem hiding this comment.
위의 printPurchaseResult메서드는 객체를 dto에 담아 출력을하도록 작성이되었는데, 현재 코드에서는 enum을 그대로 넘기고있네요!
위와 통일성을 주는것이 좋을 것 같아요(둘다 dto vs 둘다 도메인)
There was a problem hiding this comment.
참고하고 수정하였습니다! dto 역할 특성상 dto로 통일하는 방향이 더 일관성 있어보여 모두 dto를 받아오는 형식으로 수정했어요.
|
꼼꼼한 리뷰에 감사드립니다. 최대한 리뷰를 반영하며 코드를 수정해 봤어요.
저는 Mock 객체를 2가지 목적을 가지고 구현하였습니다.
일단 현제 제가 미션을 진행하면서 Mock객체들을 하나의 페키지로 묶어 관리했는데, 해윤님은 테스트용 Mock 객체를 어떻게 관리하시나요? 아니면 일반적으로 Mock 객체들을 관리할 때 쓰는 방법이 있을까요...? 또한 코드를 수정을 하며 메소드 오버로딩에 대해 질문이 생겼습니다. 입력받은 돈 만큼 |
🙋♂️ 인사
안녕하세요, 그리디 4기 백엔드 이태규입니다. 시간을 내주어 코드 리뷰 및 질의응답을 해주셔서 감사합니다.
🚗 단계별 설명
1단계 - <로또 자동 구매>
구입 금액을 입력받아 형식을 검증하고, 해당 금액에 비례하는 수량만큼 로또 번호를 자동 생성합니다.
2단계 - <로또 당첨>
지난주 당첨 번호를 입력받고, 발행된 로또 번호와 대조하여 당첨 통계(일치 개수 및 상금)와 총 수익률을 계산하여 출력합니다.
🤔 고민한 내용
❓ 질문사항
Magic Number 관련해서 자명하더라도 원시값은 변수를 활용해서 관리하는 것이 더 좋을 것 같다는 생각은 들지만, 이렇게 특정 항황에서 쓰이는 Magic Number가 많아질수록, 특히 Magic Number등 끼리 약하게나마 서로 연관성이 있으면 어떻게 관리하는 것이 좋을까요? 일단 저는 넓은 범위에서 쓰일 것 같은 것들은 하나의 클래스로 묶어 한번에 관리하는 방식으로 가긴 했는데, 이 마저도 시스템이 점점 복잡해지면 잘 안될 것 같다는 생각이 드네요...
LottoFactory를 작성하여 필요한 만큼Lotto를 생산할 수 있도록 구현을 해보았습니다. 편의상Lotto에서Lotto를 생성하는 역할을 하나의 클래스로 분리를 한 것이긴 하지만, 테스트 하기고 좋고, 제 역할을 잘 하는 것 같아 나름 괜찮아 보이는데, 혹시 이런 방식으로 구현을 하면 하나의 책임(객체 생성)이 두 곳에 존재하는 구조여서 좋은 코드인지 판단이 안서는 것 같아요. 리뷰어님이라면 이 상황에서 해당 코드는 유지보수하기 좋은 코드처럼 느껴지나요?이거는 조금 다른 느낌의 질문이긴 한데, 제가 미션을 수행하면서 하나의 기능을 구현하는 과정에서 여러 관련 기능을 동시에 개발하게 되는 경우가 많았어요. 해당 기능을 구현하려면 여러가지 필요한 기능들도 같이 덩달아 구현하게 되더라고요. 당연히 그렇게 생기는 큰 뭉텅이가 한 번에 저장되는 커밋들 때문에 버전 컨트롤이 잘 안되는 것 같네요... 혹시 리뷰어님은 어떤 기능을 구현하려할 때 어떤 방식으로 접근을 하시나요? 리뷰어님이 어떤 방식으로 기능 구현을 접근하는지 궁금합니다.