-
Notifications
You must be signed in to change notification settings - Fork 91
[그리디] 김지우 로또 미션 1,2 단계 제출합니다. #96
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
Conversation
kokodak
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.
안녕하세요 지우님~ 👋
로또 미션 구현 고생 많으셨습니다. 스터디에서 어려운 내용들을 진행하고 있는데, 배웠던 것들을 적용해보려는 노력이 매우 좋았어요. 어렵고 답답하더라도 지우님이 투자하신 이 시간과 노력들은 분명 긍정적인 결과로 돌아올테니, 끝까지 파이팅 해봐요.
밑에는 질문에 대한 답변을 남겨둘게요.
프로젝트 전체적으로 두 클래스의 책임분리가 명확히 되었는지를 중점적으로 봐주시면 좋을 것 같습니다. 😀
코드리뷰 코멘트로 제 의견을 남겨둘게요.
이 요구사항으로 원시값으로 사용하려고 했던 부분(구매금액, 구매개수, 수익률)들을 모두 wrapper클래스로 선언해두었습니다. 스터디때도 이 질문에 대해 상황에 따라 달라진다라는 결론이 나왔던 것 같은데요..(저도 이 결론에 동의합니다!)
그런데 이번 미션에서 이 요구사항이 나온 의도가 뭔지 궁금합니다..!
저 또한 이 요구사항의 명확한 의도는 잘 모르겠어요. 추측해보자면, 연습하기 위해서가 아닐까 합니다.
‘원시값 포장을 하는 것은 상황에 따라 달라진다’ 라는 결론은 사실 말로만 떠들어서 나온 결론이기도 한데요. 사실 실제로 왜 그런지 체감해보는 것이 가장 중요한 공부라고 생각해요. 모든 원시값을 한 번 포장해보고 거기서 어떤 불편함이 실제로 있었는지, 또 어떤 점은 좋았는지 느껴보는 거죠.
다만 하나 말씀드리고 싶은 점은, int를 Integer로 wrapping 하는 것은 이 미션에서 요구하는 원시값 포장이 아니에요. 커스텀 클래스를 만들어서, 비즈니스적인 의미를 담는 것이 이 미션에서 요구하는 원시값 포장입니다. 그런 관점에서 봤을 때, 아직은 원시값 포장이 잘 이루어지지 않았다고 생각해요. 요건 미션하면서 차차 개선해 나가 봅시다!
간단한 애플리케이션에서 클래스가 과도하게 많은건 좋지 않다고 생각합니다. 이런 관점에서 비슷한 로직 혹은 의존하는 클래스가 동일한 로직을 한 군데(ProfitCalculator)에 두다보니 또 너무 많은 책임을 담당하게 된 것 같은데요.. ProfitCalculator이 너무 많은 책임을 지고 있는 것인지, 이 정도까진 괜찮다고 받아들여 질 수 있는지에 대한 의견을 여쭤보고 싶습니다!
마찬가지로 코드리뷰 코멘트로 제 의견을 남겨둘게요.
단위 테스트 구현 (테스트 코드 작성해야 하는 것을 다른 분들 제출한 거 보고 떠올렸습니다ㅜㅜ)
테스트는 자바 미션에서 중요한 영역이에요. 다음 리뷰 반영 때 테스트 구현도 꼭 포함해 주세요~!
|
|
||
| public class LottoController { | ||
|
|
||
| private final static int PRICE_PER_LOTTO = 1000; |
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.
이 상수는 로또 비즈니스적으로 큰 의미가 있어 보여요. 만약 로또 가격에 변화가 생긴다면, 현재는 Controller의 코드가 변경되어야겠네요. 이 지점이 저는 조금 어색하다고 느껴지네요.
이 상수가 어떤 클래스, 더 나아가서 어떤 패키지 내부에 위치하는 것이 적절할까요? 한번 고민해보면 좋을 지점인 듯하여 리뷰 남깁니다~!
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.
로또 한 장당 가격이 변동 가능성 있는 값이지만, 처음에는 단순히 '사용자 입력을 받아 도메인을 호출하는 역할'을 하는 Controller에서 관리해도 괜찮다고 생각했어요. (아래 부연설명 참고 부탁드려요!)
하지만 리뷰를 보고 생각해보니, 로또 가격은 로또의 비즈니스 로직을 결정하는 중요한 값이기 때문에 Controller가 아닌 도메인 영역에서 관리하는 것이 더 적절해 보이네요 ㅎㅎ
MVC 패턴에서 controller에 대한 개념, 역할이 제일 모호하게 느껴졌는데요.
비지니스 로직에 영향을 받지 않도록 하는 것이 중요하다는 것을 모른 채
단순히 중간 다리 역할을 한다는 것만 생각하고 구현하다 보니 현재 프로그램에서 controller가 비지니스 로직(변동)에 대한 영향을 많이 받고 있는 것 같습니다.
좀 더 설명드리자면 제가 알고 있는 controller는 정의하는 개념보다 더 포괄적인 것 같아요,
중간 다리 역할이라 해서 도메인을 호출하는 것 이상으로 도메인에 진입하기 전 필요한 여러 가지 일들을 처리하게 되는데요.
이 일들 또한 domain 딴에서 관리되어야 할 비지니스 로직이 아닌가 하는 의심을 계속해서 해봐야 할 것 같은 생각이 듭니다.
관련된 내용에 대해 좀 더 공부해 보도록 하겠습니다! 😄
ps. 혹시 이 부분에 대해 공부하기 좋은 자료나 kokodak님 만의 판단 팁이 있을까요?!
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.
좋은 고민을 적어주셨네요. 저도 사실 MVC 패턴을 공부하면서 Controller의 의미/역할이 제일 모호했던 것 같아요.
일단 제 생각에는, Controller가 비즈니스 로직에 영향을 받는 것은 어쩔 수 없는 영역인 것 같아요. 도메인 영역의 코드가 레고 블럭이라면, 결국 비즈니스에 맞게 레고를 잘 조립하는 일은 Controller에서 할 수 밖에 없기 때문이죠. 저희가 MVC 패턴을 통해서 컴포넌트를 나누긴 했지만, 그렇다고 나눈 영역이 서로 완벽히 분리되고 격리되는 건 또 아니라고 생각해요.
중간 다리 역할이라 해서 도메인을 호출하는 것 이상으로 도메인에 진입하기 전 필요한 여러 가지 일들을 처리하게 되는데요.
이 일들 또한 domain 딴에서 관리되어야 할 비지니스 로직이 아닌가 하는 의심을 계속해서 해봐야 할 것 같은 생각이 듭니다.
좋은 접근이라고 생각합니다. '이 로직도 비즈니스 로직이 아닐까?' 하는 의심을 꾸준히 하면서 도메인 영역을 발전시켜나가면, 점차 Controller의 영역이 뚜렷해질 거라고 생각해요.
ps. 혹시 이 부분에 대해 공부하기 좋은 자료나 kokodak님 만의 판단 팁이 있을까요?!
로또 미션 코드는 레퍼런스가 굉장히 많아요. 다른 사람들은 어떻게 작성했는지, 최대한 많은 코드를 봐보는 것이 제일 좋은 공부 자료인 것 같습니다.
src/main/java/domain/LottoList.java
Outdated
| public class LottoList { | ||
|
|
||
| private final List<Lotto> lottoList; | ||
| RandomLottoNumberGenerator generator = new RandomLottoNumberGenerator(); |
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.
이 필드는 불필요한 것 같아요. 현재 LottoList 생성자로 RandomLottoNumberGenerator를 받아오고 있고, 이 Generator를 Lotto에 주입해주고 있어서요!
src/main/java/domain/Lotto.java
Outdated
|
|
||
| public class Lotto { | ||
|
|
||
| private final List<Integer> 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.
로또 번호를 특정 객체로 포장해보면 어떨까요? 예를 들어, List<LottoNumber> numbers; 처럼요.
로또 번호에 대한 검증 로직도 추가되면 좋을 것 같아요. ex)로또 번호는 1~45 사이의 정수이다.
src/main/java/domain/LottoList.java
Outdated
| public LottoList(Integer lottoCount, RandomLottoNumberGenerator generator) { | ||
| this.lottoList = new ArrayList<>(); | ||
| this.generator = generator; // 이미 생성된 RandomLottoNumberGenerator를 사용 | ||
| for (int i = 0; i < lottoCount; i++) { | ||
| this.lottoList.add(new Lotto(generator)); | ||
| } | ||
| } |
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.
현재 LottoList에서 생성자로 받아오는 Generator는 추상화된 LottoNumberGenerator가 아닌, RandomLottoNumberGenerator 인데요! 이렇게 되면 로또 번호를 생성한다 라는 행위를 추상화한 이유가 조금 모호해지는 거 같아요.
이런 질문을 드리고 싶어요.
Lotto는 추상화된 LottoNumberGenerator를 주입받는데, LottoList는 어떤 이유에서 RandomLottoNumberGenerator를 주입받는 걸까요?
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.
지난 미션에서 리뷰 받았던 의존성 역전 원칙(DIP)에 대해 아직 제대로 학습되지 않아 생긴 실수인 것 같습니다 😅
(아래 코멘트에서 이어서 답변하도록 하겠습니다!)
|
|
||
| private final static int PRICE_PER_LOTTO = 1000; | ||
|
|
||
| private final static RandomLottoNumberGenerator generator = new RandomLottoNumberGenerator(); |
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.
LottoController에서 RandomLottoNumberGenerator를 가지고 있군요!
추측하기에는,
1 ) 지우님은 Controller가 가장 외부 레이어라고 판단하셨고,
2 ) 어디에선가는 LottoNumberGenerator를 구현한 구체 클래스를 주입해 주어야 할 테니,
Controller에서 RandomLottoNumberGenerator를 선언해주신 것 같아요. 맞나요?
이건 어떨까요? LottoController도 추상화된 것에 의존하도록 하고, 구체 클래스는 LottoApplication에서 주입받도록 하는 거죠.
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 ) 지우님은 Controller가 가장 외부 레이어라고 판단하셨고,
2 ) 어디에선가는 LottoNumberGenerator를 구현한 구체 클래스를 주입해 주어야 할 테니,
Controller에서 RandomLottoNumberGenerator를 선언해주신 것 같아요. 맞나요?
LottoList를 제외하고, 제 의도는 정리해주신 흐름이 맞습니다..!
(LottoList에서도 구체 클래스가 아닌 인터페이스에 의존하도록 수정하겠습니다.)
LottoController도 추상화된 것에 의존하도록 하고, 구체 클래스는 LottoApplication에서 주입받도록 하는 거죠.
이 부분을 보고 controller와 application의 차이에 대해 서치한 후,
DI(의존성 주입)을 고려하여 Application과 Controller의 역할을 다음과 같이 정리해 봤습니다 🤓
(혹시 잘못 이해한 부분이 있거나 동의하지 않는 부분이 있으면 말씀해주세요!)
Application- 애플리케이션의 시작과 설정
- 객체를 생성하고 조립하는 역할
ControllerApplication에서 주입받은 객체를 사용- 핵심 로직을 실행하는 역할
이런 관점에서 남겨주신 리뷰를 application에서 RandomNumberGenerator를 생성해서 controller에 주입시켜라 라는 의미의 말씀으로 받아드렸습니다!
그런데 한 controller에서 여러 생성기를 받아야 하는 경우에도 이런 방식으로 코드를 짜는게 더 효율적인지에 대한 의문이 듭니다.
좀 더 설명을 드리자면, 현재 LottoNumberGenerator를 상속받는 구현체가 RandomLottoNumberGenerator 하나지만,
만약 여러 개의 구현체가 생기고 사용자의 요청에 따라 다른 생성기를 선택해야 하는 상황이라면 Application에서 모든 생성기를 만들어 Controller에 넘겨주는 방식이 더 적절한지 궁금합니다.
(아니면 혹시 리뷰 남겨주신 방식과 제가 이해한 방식이 다른가요?)
추상화된 것에 의존하는 이유는 내부 로직을 외부에서 모르게 하기 위해서라고 이해하고 있는데, 위와 같은 방식으로 설계하면 결국 Application이 특정 구현체에 대한 세부 내용을 알고 있어야 하지 않나 하는 의문이 들어서요..!
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.
이런 관점에서 남겨주신 리뷰를 application에서 RandomNumberGenerator를 생성해서 controller에 주입시켜라 라는 의미의 말씀으로 받아드렸습니다!
정확합니다 👍
만약 여러 개의 구현체가 생기고 사용자의 요청에 따라 다른 생성기를 선택해야 하는 상황이라면 Application에서 모든 생성기를 만들어 Controller에 넘겨주는 방식이 더 적절한지 궁금합니다.
만약 그런 요구사항이 존재한다면, 저는 말씀주신대로 구현할 것 같아요.
추상화된 것에 의존하는 이유는 내부 로직을 외부에서 모르게 하기 위해서라고 이해하고 있는데, 위와 같은 방식으로 설계하면 결국 Application이 특정 구현체에 대한 세부 내용을 알고 있어야 하지 않나 하는 의문이 들어서요..!
좋은 질문인 것 같아요. 일단 한 가지 말씀드리자면, 아무리 추상화를 하더라도 어디에선가는 구현체를 주입해야 합니다. 구현체를 주입해주지 않고 추상화된 코드로만 애플리케이션이 구성되는 것은 마법 같은 일이에요. 불가능하다는 뜻입니다. 따라서, Application은 특정 구현체 내용을 알고 있어야 해요.
저희가 추상화를 하는 이유는, 변경으로부터 안전한 영역을 최대한 확보하기 위해서라고 생각하는데요. Application 영역을 희생하는 대신, Controller, Domain 등의 영역을 안전 구역으로 만드는 데 의미가 있는 것 같아요.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class LottoList { |
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와 LottoList의 책임 분리를 질문 주셔서 전체적인 제 생각을 남겨보겠습니다.
Lotto는 본인의 로또 번호를 관리하고, 당첨 번호가 들어왔을 때 본인의 번호와 몇개가 일치하는지 판단하는 책임을 가지고 있는데요. 저는 적절히 객체가 구성됐다고 생각합니다. 어떤 숫자들의 집합(List)이 로또 라는 중요한 비즈니스적 의미를 만들기도 하구요.
다만 LottoList는 조금 불분명하다고 느껴져요. 현재는 여러 로또들을 관리하고 있는 책임 이외에 아무런 행위를 하지 않기도 하구요. 또한, 여러 로또들의 집합이 비즈니스적으로 중요한 의미를 만들어내는가? 에 대해서도 크게 와닿지 않는 것 같습니다.
일급 컬렉션을 위한 일급 컬렉션이 되어버린 건 아닐까 하는 생각입니다.
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.
이 부분에 대해서 다음 코멘트에 의견 남겨두도록 하겠습니다!
리팩토링한 코드 보시면서 LottoList의 책임에 대해 추가 답변 부탁드려요 🙌
|
|
||
| public RandomLottoNumberGenerator() { | ||
| this.lottoNumbersRange = new ArrayList<>(); | ||
| for (int i = 1; i < 46; 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.
for문에 적혀있는 숫자는 비즈니스적으로 중요한 의미가 있어보여요.
상수로 추출하면 어떨까요?
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.
전 사실 변동 가능성을 염두에 두고 상수로 빼내는 것이 좋겠다는 의견은 아니었어요.
상수로 빼내는 것은 관리 포인트를 한 곳으로 줄인다는 장점도 있지만, 리터럴이 가지는 비즈니스적 의미를 담을 수 있다는 장점 또한 있습니다. 저는 후자의 장점이 중요하다고 생각해요.
로또 라는 비즈니스를 잘 모르는 신규 개발자가 지우님의 팀에 합류해서 개발을 시작했다고 가정해봐요. 그 개발자는 1, 45 숫자의 의미를 모르는 상태이겠지만, 상수명을 보고 어느정도의 비즈니스 의미를 파악할 수 있겠죠. 만약 숫자만 쓰여져 있었다면, 그 숫자의 의미를 파악하기 위해 추가적인 시간이 소모됐을 거예요.
코드에 최대한 많은 맥락을 주는 친절함이 있다면, 불필요한 리소스 낭비를 막을 수 있다는 장점이 있는 거 같아요.
추가로, 로또 코드를 구현하다보면 1, 45라는 숫자는 반복적으로 사용될 텐데요, 한번 상수로 빼두면 재사용할 수 있다는 장점도 있겠네요.
| Collections.shuffle(lottoNumbersRange); | ||
| List<Integer> lottoNumber = new ArrayList<>(lottoNumbersRange.subList(0, 6)); | ||
| Collections.sort(lottoNumber); | ||
| return 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.
직관적으로 잘 보이는 로직이네요 👍
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class ProfitCalculator { |
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.
ProfitCalculator이 너무 많은 책임을 지고 있는 것인지, 이 정도까진 괜찮다고 받아들여 질 수 있는지에 대한 의견을 여쭤보고 싶습니다!
클래스를 분리하는 기준은 사람마다 다르고, 정답이 없는 영역이지만, 그럼에도 꼭 분리해야 하는 경우가 있다면 비즈니스적으로 경계지어야 하는 영역 일 때 분리하는 것 같아요.
그런 관점에서, 현재 ProfitCalculator는 많은 책임을 가지고 있다고 판단했습니다.
수익률과 로또 등수는 명확히 다른 영역으로 보여요. 어떤 로또가 몇등인지 판단을 수익률 도메인에서 하는 것은 적절한 책임 분리가 아니라고 생각해요.
또한, ProfitCalculator 라는 클래스명은 좋지 않은 이름이라고 생각해요. 만능의 느낌이 든달까요? 수익률을 계산하기 위해서 필요한 다른 도메인 영역들을 마음대로 끌어다 써도 어색하지 않은 네이밍이라, 개발자가 구분해야 할 도메인 영역을 구분하지 못하는 상황을 발생하게 만드는 것 같아요.
이 클래스명이 Profit 이었다면, Prize와는 조금 다른 단위이지 않을까 하는 생각이 들었을 것 같아요.
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.
수익률과 로또 등수는 명확히 다른 영역으로 보여요. 어떤 로또가 몇등인지 판단을 수익률 도메인에서 하는 것은 적절한 책임 분리가 아니라고 생각해요.
이 클래스명이 Profit 이었다면, Prize와는 조금 다른 단위이지 않을까 하는 생각이 들었을 것 같아요.
수익률 계산과 등수 산출이 프로그램에서 순차적으로 연산되는 과정이라는 점에 집중하다 보니, 두 개를 분리하는 게 맞을지 고민이 많았어요. 그런데 리뷰를 보니 나누는 게 더 적절하다는 점이 확실히 와닿았습니다! 🤓
책임 분리에 대한 중요성은 인지하고 있는데 아직 연습이 부족하다 보니 실제로 구현해 내기가 생각보다 어려운 것 같습니다 😅
주신 리뷰 바탕으로 로직 분리해서 구현해 보도록 하겠습니다!
|
아직 테스트코드가 완벽하게 작성되진 않았지만, 리뷰 반영 내용 정리 🗒️
|
kokodak
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.
지우님! 리뷰 반영 고생 많으셨습니다 👏 그리고 재리뷰가 늦어져서 죄송해요 ㅠㅠ
아직 더 보완하면 좋을 점들이 있어서 코멘트를 좀 남겨봤는데, 보면서 이해가 잘 안되는 것이 있다면 제게 연락해 주세요.
다만 이제 3, 4, 5단계를 하고 계실 거라서 1, 2단계 PR은 머지하도록 하고, 제가 이번에 남겨놓은 코멘트는 다음 미션을 제출할 때 반영해주시면 될 것 같아요. 아래는 질문에 대한 답변을 남겨놓겠습니다. (지우님이 정리해주신 리뷰 반영 내용들은 모두 합리적이고 많은 고민을 해보신 게 느껴져서 별 다른 코멘트는 남기지 않을게요. 잘해주셨습니다~)
WinningLottoNumbers 클래스 구현
리뷰 코멘트로 제 의견을 남겨두었습니다.
LottoList의 책임이 이번에 너무 과하지 않은지 봐주시길 부탁드립니다.
전 어색하지 않은 것 같아요. 책임 부여를 적절히 해주신 것 같습니다.
Controller의 무게를 줄이기 위해 LottoList와 WinningLottoNumbers 클래스에 여러 로직을 담도록 했는데 여전히 controller가 비지니스 로직의 영향을 많이 받는 것 같은데 이 부분에 대해서 조언 부탁드립니다.
Controller가 비즈니스 로직에 영향을 받는 것은 어쩔 수 없는 것 같아요. 다만 그 영향을 최소화시키는 것에 집중하면 될 것 같습니다.
인텔리제이의 테스트 커버리지 라는 기능을 알게 되었습니다! 현재까지 짠 3가지 클래스에 대한 테스트 코드가 나름대로 잘 짜여졌다고 생각했는데 예상과 달리 커버리지가 0%인데요...ㅜ 혹시 문제점이 뭔지 보이시나요..?!
저는 도메인 영역의 커버리지가 제일 중요하다고 생각하는데, 현재는 대략 60% 정도 검증되고 있어요. 최소 80% 정도까지는 커버리지를 올리면 좋을 것 같습니다~!
| private static final int MIN_NUMBER = 1; | ||
| private static final int MAX_NUMBER = 45; |
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에 같은 상수가 선언되어 있는데, 이걸 사용하면 어떨까요?
| // 입력 값 가공: 공백 제거, 쉼표로 분리 | ||
| private String[] parseInput(String input) { | ||
| return input.trim().split("\\s*,\\s*"); | ||
| } |
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.
도메인 영역인데 view에 대한 의존성이 간접적으로 존재하네요. 제거해야 할 의존성으로 보여요!
| @Override | ||
| public int compareTo(LottoNumber other) { | ||
| return Integer.compare(this.number, other.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.
compareTo 로 비교 가능하도록 만들어 주셨군요! 좋은 것 같아요 👍
| assertThrows(IllegalArgumentException.class, () -> new LottoNumber(0)); | ||
| assertThrows(IllegalArgumentException.class, () -> new LottoNumber(46)); | ||
| assertThrows(IllegalArgumentException.class, () -> new LottoNumber(-5)); |
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.
값만 다르고 테스트의 형식이 같다면, @ParameterizedTest 를 활용해볼 수 있어요.
https://velog.io/@ohzzi/junit5-parameterizedtest
한번 학습해보시고, 테스트 코드에 적용해보면 좋을 것 같아요~!
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class WinningLottoNumbers { |
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.
당첨 번호는 사용자로부터 입력을 받는 값이기에 여러 검증이 필요하다고 생각이 들었습니다.
WinningLottoNumbers의 설계가 view 컴포넌트에 강하게 의존하고 있는 것 같아요. 도메인의 입장에서 WinningLottoNumbers는, 그 값이 사용자의 입력으로부터 들어오는지 아닌지는 신경써야 할 관심사가 아니라고 생각해요. 사용자의 입력이든, 아니면 코드 레벨에서 개발자가 임의로 객체를 생성하든 일관되고 일반적인 검증을 수행하는 게 좋을 것 같아요.
저는 WinningLottoNumbers도 결국 본질은 Lotto 객체 아닐까? 하는 생각을 했었는데요! 왜 Lotto 객체를 재활용하지 않고, 비슷한 친구가 또 나오게 됐을까 생각하다가, 지우님이 이런 클래스를 만드시게 된 배경에는 Lotto 클래스의 생성자가 LottoNumberGenerator를 받고 있어서 그런 것 같다고 느꼈어요.
그럼 잠깐 얘기를 Lotto 클래스로 옮겨 가볼게요.
Lotto의 생성자로 받는 파라미터 타입이 LottoNumberGenerator가 아닌, List 였다면 어땠을까요?
제 생각엔 Lotto 객체 생성이 조금 더 자유로울 것 같아요. 외부에서 입력을 받든, 개발자가 직접 생성하든, 어쨌든 저희가 List를 적절히 구성하면 되니까요. 또한, 번호의 개수가 6개인지, 그리고 번호에 중복이 있는지에 대한 검증은 Lotto로 옮겨볼 수도 있겠네요.
제 리뷰를 보며 느끼셨을 지 모르겠지만, 사실 어떤 클래스의 생성자 파라미터 타입은 필드 타입과 같게끔 설계하는 것이 객체 생성에 더 많은 자유도를 줘요. 이렇게 되면 단위 테스트 작성이 더 쉬워지기도 하고, 객체 재활용이 수월해지기도 하지만, 자유도가 높아지기 때문에 객체의 무결성이 깨질 수 있다는 위험성이 뒤따르기도 해요. 예를 들어, 로또의 번호 개수가 6개를 초과한다던지, 혹은 로또 번호가 1~45 사이에 없다던지 하는 식으로요. 그래서 보통 객체 생성자 내부에서 검증 로직을 넣어두고, 객체 생성 시점에 무결성이 지켜지도록 설계하는 것 같아요. 이렇게 되면 성공적으로 생성된 객체 내부의 데이터는 무조건 저희가 의도한대로 만들어지게 되겠죠.
이제 다시 WinningLottoNumbers로 돌아와볼게요. WinningLottoNumbers는 결국 Lotto의 연장선이에요. Lotto에서 필요한 검증을 모두 마쳤다면, 그리고 WinningLottoNumbers가 Lotto 객체를 재활용해서 사용한다면, 현재 WinningLottoNumbers에서는 로또에 대한 비즈니스 검증을 수행할 필요가 없어요. 입력에 대한 검증은 도메인 영역이 아닌 view 영역으로 책임을 옮기는 게 좋아 보이구요.
제가 좀 길게 적어놨는데, 리뷰를 보시면서 객체를 다시 설계해보시면 어떤 느낌인지 이해가 가실 것 같아요. 한번 도전해보시죠!

kokodak님, 이번 미션 리뷰어로 만나게 되어 반갑습니다 🙌
잘 부탁드려요~! 🙇♀️
지난 미션 통해서 배운 내용들을 유념하며 최대한 많이 활용해보려고 노력했습니다.
그치만 아직 미숙한 부분이 있으니 더 공부하면 좋을 부분에 대해서 많은 의견 남겨주시길 부탁드립니다!
고민 포인트와 질문 사항 💭
일급컬렉션과 구성하는 객체의 책임분리
Lotto 클래스는 로또 번호를 관리하는 책임을 맡고,LottoList 클래스는 여러 Lotto 객체들을 관리하는 역할을 합니다. 코드를 짜다보니 두 클래스의 책임과 관리 포인트들이 섞인 느낌인데요. (아니면 제 스스로가 아직 일급컬렉션 사용이 익숙하지 않아서 느끼는 혼란일 수도 있을 것 같습니다 ㅎㅎ.)프로젝트 전체적으로 두 클래스의 책임분리가 명확히 되었는지를 중점적으로 봐주시면 좋을 것 같습니다. 😀
원시값을 무조건 사용하지 말아야 하나?!
이 요구사항으로 원시값으로 사용하려고 했던 부분(구매금액, 구매개수, 수익률)들을 모두 wrapper클래스로 선언해두었습니다. 스터디때도 이 질문에 대해 상황에 따라 달라진다라는 결론이 나왔던 것 같은데요..(저도 이 결론에 동의합니다!)
그런데 이번 미션에서 이 요구사항이 나온 의도가 뭔지 궁금합니다..!
클래스의 개수
간단한 애플리케이션에서 클래스가 과도하게 많은건 좋지 않다고 생각합니다. 이런 관점에서 비슷한 로직 혹은 의존하는 클래스가 동일한 로직을 한 군데(
ProfitCalculator)에 두다보니 또 너무 많은 책임을 담당하게 된 것 같은데요..ProfitCalculator이 너무 많은 책임을 지고 있는 것인지, 이 정도까진 괜찮다고 받아들여 질 수 있는지에 대한 의견을 여쭤보고 싶습니다!이 외에 제가 프로그래밍 속도가 느려서 생각만하고 아직 구현은 못 한 부분들이 있습니다.
이 부분들은 로컬에서 작업하다가 commit 전에 디코로 멘션 드리도록 하겠습니다 🥹리뷰 반영하면서 같이 commit 하도록 하겠습니다!
감사합니다 😄