2단계 - 로또 #4196
2단계 - 로또 #4196
Conversation
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
자동차 경주 미션에서 지켰던 원칙을 지키면서 구현해서 일까요?
전체적인 객체 설계, 포장, 테스트 코드 구현 등이 작은 단위로 깔끔하게 구현 잘 했네요. 💯
그럼에도 불구하고 몇 가지 개선했으면 하는 부분이 있어 피드백 남겼어요.
Q1.
TDD사이클을 연습하면서 현재 가장 작은 단위의 기능이 될거로 예상되는 LottoNumber, 그다음 Lotto 그리고 여러장이모인 LottoTickets 그리고 당첨갯수와 상금을 계산하는 Rank까지 구현하였는데요
Lotto에서는 번호가 중복인경우에대한 validate를 진행하였는데 "랜덤 로또를 생성한다"는 책임을 LottoTickets에 우선 넣어놨는데 Lotto로 들어가도 이상하지 않을것 같기도하고해서 이런 부분이 조금 애매하네요
A1.
이 부분 또한 정답이 있는 것은 아닌 영역인데요.
저는 LottoTickets와 Lotto 모두 랜덤 로또를 생성 책임을 부여하기에 적합한 곳은 아니지 않나라는 생각을 해봅니다.
객체의 역할을 어떻게 부여하느냐의 차이인 만큼 승인님이 결정하고 구현하고 리뷰 요청해보세요.
| public Lotto(List<Integer> numbers) { | ||
| validateSize(numbers); | ||
| validateDuplicate(numbers); | ||
| this.numbers = createLottoNumbers(numbers); | ||
| } |
There was a problem hiding this comment.
| public Lotto(List<Integer> numbers) { | |
| validateSize(numbers); | |
| validateDuplicate(numbers); | |
| this.numbers = createLottoNumbers(numbers); | |
| } | |
| public Lotto(List<Integer> numbers) { | |
| this(createLottoNumbers(numbers)); | |
| } | |
| public Lotto(List<LottoNumber> numbers) { | |
| validateSize(numbers); | |
| validateDuplicate(numbers); | |
| this.numbers = numbers; | |
| } |
위와 같이 주 생성자를 추가하고 부 생성자가 주 생성자를 호출하도록 구현하면 어떨까?
There was a problem hiding this comment.
지속적으로 리뷰해주시는 내용인데 이부분이 바로 잘안되는듯하네요 신경써서 진행해보겠습니다
| } | ||
|
|
||
| private void validateDuplicate(List<Integer> numbers) { | ||
| Set<Integer> uniqueNumbers = new HashSet<>(numbers); |
There was a problem hiding this comment.
중복을 허용하지 않는다면 인스턴스 변수를 List가 아니라 List으로 구현하는 것은 어떨까?
| public int countMatchNumber(Lotto other) { | ||
| int count = 0; | ||
| for (LottoNumber number : numbers) { | ||
| if (other.contains(number)) { |
| for (LottoNumber lottoNumber : numbers) { | ||
| if (lottoNumber.equals(number)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
| for (LottoNumber lottoNumber : numbers) { | |
| if (lottoNumber.equals(number)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| return this.numbers.contains(number); |
콜렉션의 contains()를 활용해 구현하면 어떨까?
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class LottoNumber { |
| public LottoNumber(int value) { | ||
| validate(value); | ||
| this.value = value; | ||
| } | ||
|
|
||
| public LottoNumber(String value) { | ||
| this(Integer.parseInt(value)); | ||
| } |
There was a problem hiding this comment.
| public LottoNumber(int value) { | |
| validate(value); | |
| this.value = value; | |
| } | |
| public LottoNumber(String value) { | |
| this(Integer.parseInt(value)); | |
| } | |
| public LottoNumber(String value) { | |
| this(Integer.parseInt(value)); | |
| } | |
| public LottoNumber(int value) { | |
| validate(value); | |
| this.value = value; | |
| } |
주 생성자를 마지막에 구현하는 것이 컨벤션
| this.lottos = lottos; | ||
| } | ||
|
|
||
| public static LottoTickets create(int purchaseAmount) { |
There was a problem hiding this comment.
purchaseAmount도 원시 값인데 포장해 보면 어떨까?
포장한 후에 이 객체의 역할에 대해 고민해 본다.
| } | ||
|
|
||
| private static Lotto createRandomLotto() { | ||
| //TODO 많은 일을 하고 있음 |
There was a problem hiding this comment.
공감함
메서드를 분리하거나 새로운 객체로 분리하는 것은 어떨까?
| void 로또_번호_6개로_생성() { | ||
| List<Integer> numbers = Arrays.asList(1, 2, 3, 4, 5, 6); | ||
|
|
||
| Lotto lotto = new Lotto(numbers); |
There was a problem hiding this comment.
| Lotto lotto = new Lotto(numbers); | |
| Lotto lotto = new Lotto(1, 2, 3, 4, 5, 6); |
가변 인자를 받는 생성자도 추가하면 Lotto 생성이 더 편리해 지지 않을까?
2 participants
안녕하세요! 아직 미션을 다 완성하지 못하였는데 궁금한점이 있어서 PR을 생성하게되었습니다.
자세한 요구사항 조건이 없어서 일반적으로 알고있는 요구사항으로 기능 요구사항을 뽑아봤습니다.
변수명을 뽑으면서 알고있는 영어단어가 짧음을 또 한번 느끼고있네요.
요구사항 정리를 할때 어떠한 기능들 단위로 클래스를 뽑을지에 시간 투자를 많이하였는데도 불구하고 구현하다 보니 계속 바뀌게 되네요
PR작성하고나서 보니 랜덤에 대한 테스트도 작성하기 힘들게 작성한것같아서 수정해보겠습니다
Q1.
TDD사이클을 연습하면서 현재 가장 작은 단위의 기능이 될거로 예상되는 LottoNumber, 그다음 Lotto 그리고 여러장이모인 LottoTickets 그리고 당첨갯수와 상금을 계산하는 Rank까지 구현하였는데요
Lotto에서는 번호가 중복인경우에대한 validate를 진행하였는데 "랜덤 로또를 생성한다"는 책임을 LottoTickets에 우선 넣어놨는데 Lotto로 들어가도 이상하지 않을것 같기도하고해서 이런 부분이 조금 애매하네요