[또링] 로또 2차 미션 코드리뷰 요청합니다. #198
Conversation
1. 정수만 입력 가능 2. 음수와 0은 입력 불가능 3. 1,000원 단위로만 입력 가능 4. 최대 구입 가능 금액 100,000원 제한
1. 생성자 대신 valueOf로 변경 2. 테스트코드의 생성자를 valueOf로 수정
- 기존의 Empty를 검증하는 기능 제거
| int numberOfManualLotto; | ||
| try { | ||
| numberOfManualLotto = Integer.parseInt(inputCountOfManualLotto()); | ||
| } catch (IllegalArgumentException e) { | ||
| printExceptionMessage(COUNT_OF_MANUAL_IS_WRONG_TYPE_MESSAGE); | ||
| return receiveCountOfManualLotto(numberOfLotto); | ||
| } | ||
| if (numberOfManualLotto < 0 || numberOfManualLotto > numberOfLotto) { | ||
| throw new IllegalArgumentException(COUNT_OF_MANUAL_IS_WRONG_RANGE_MESSAGE); | ||
| } | ||
| return numberOfManualLotto; |
| @@ -3,6 +3,7 @@ | |||
| import java.util.ArrayList; | |||
| import java.util.Iterator; | |||
| import java.util.List; | |||
| import java.util.Objects; | |||
|
|
|||
| public class Lottos implements Iterable<Lotto> { | |||
There was a problem hiding this comment.
일급컬렉션으로서 하는 역할이 없네요 :)
그렇다면 List 를 사용하는게 더 편하지 않을까요?
의미없는 객체 포장은 불필요해보여요.
There was a problem hiding this comment.
넵 해당 부분 수정하였습니다! 확실히 List로 사용하는 것이 더 편하게 느껴지네요 😀
| LottoMoney inputLottoMoney = receiveInputMoney(); | ||
| int countOfLotto = inputLottoMoney.calculateCountOfLotto(); | ||
| int countOfManualLotto = receiveCountOfManualLotto(countOfLotto); | ||
| int countOfAutoLotto = countOfLotto - countOfManualLotto; | ||
| Lottos manualLottos = receiveManualLotto(countOfManualLotto); | ||
| printPurchaseCompleteMessage(countOfManualLotto, countOfAutoLotto); | ||
| Lottos autoLottos = LottosGenerator.generate(countOfAutoLotto); | ||
| Lottos totalLottos = manualLottos.add(autoLottos); | ||
| printPurchasedLotto(totalLottos); | ||
|
|
||
| WinningLotto winningLotto = receiveWinningLotto(); | ||
| LottoWinningResult winningResult = new LottoWinningResult(totalLottos, winningLotto); | ||
| printWinningResult(winningResult.getLottoRankCount()); | ||
| printWinningRatio(winningResult.calculateWinningRatio(inputLottoMoney)); |
There was a problem hiding this comment.
LottoMoney, Lottos, Lotto, WinningLotto 등 여러 객체를 추출했지만,
객체 간의 관계는 설계되지 못하고 LottoController 에서 나열로만 처리되고 있네요 :)
LottoController 가 너무 많은 책임과 역할을 가지고 있다고 생각이되요.
로또의 구입을 담당하는 LottoMachine 을 추출하여, LottoMachine 을 통해 로또를 생성해낼 수 있을거에요.
LottoMoney 는 현재 금액을 내장하고 있지만, 로또의 가격까지도 알고 있기 때문에 책임이 모호한 객체가 되어있어요.
Money 는 돈을 관리하는 역할에 집중하고, 로또의 가격을 관리하는 어딘가에서 금액을 차감해가며, 구입할 수 있는 개수를 책임질 수 있을 것 같아요.
위에는 예시로 역할과 책임을 더 작은 단위로 나누고 객체 간의 관계를 만들 수 있지 않을까하여 적어보았어요.
LottoController 가 가지고 있는 너무 많은 역할을 덜어줄 수 있도록 리펙토링해보는 것은 어떨까요?
There was a problem hiding this comment.
좋은 리뷰 감사합니다 :) 사실 어떻게 리팩토링을 해야할까 고민하다가 마감시간이 되어 우선 올려야겠다는 마음에 제출했는데 이렇게 빨리 리뷰를 해주실 줄 몰랐습니다... (정말 돌아가는 쓰레기 코드..같아서 매우 부끄럽습니다...🤣) 우선 리뷰어님이 말씀해주신 내용들 바탕으로 열심히 리팩토링 해보고 다시 리뷰요청 보내겠습니다!
LottoMachine, AutoLottoGenerator, ManualLottoGenerator, CountOfManualLotto, LottoPrice, NumberLinesOfManualLotto 객체 생성
- 기존 수동 로또 개수와 수동 로또 번호들을 전달하는 방법에서 수동 로또 번호들만 전달하고 사이즈를 얻어오는 방법으로 변경
기존 LottoParser로 생성하던 방법에서 ManualLottoGenerator로 생성하는 방법으로 변경
| import lotto.domain.lotto.Lotto; | ||
| import lotto.domain.number.NumberLinesOfManualLotto; | ||
|
|
||
| public class LottoMachine { |
There was a problem hiding this comment.
정적 클래스로 제공하는 이유는 무엇일까요?
더 유연하게 사용할 수 있도록 객체로서 제공되는 것이 더 효율적이지 않을까요?
There was a problem hiding this comment.
LottoMachine 클래스는 단순히 금액을 넣으면 로또를 반환하는 역할을 한다고 생각하여 정적 클래스로 만들었습니다😥 하지만 컨트롤러의 책임을 분리하면서 LottoMachine의 생성자를 통해 금액과 수동 로또 갯수를 입력받도록 하여 LottoMachine 객체를 더 유연하게 사용할 수 있도록 해당 부분 수정하였습니다😀
| countOfAutoLottoTicket = DEFAULT_COUNT; | ||
| } | ||
|
|
||
| public static List<Lotto> buyLottoTicket(int countOfAllLotto, NumberLinesOfManualLotto manualLottoNumbers) { |
There was a problem hiding this comment.
LottoMachine 의 역할은 무엇인가요?
정보를 소유하고 가지고 있다면 개방되지 않아도 될 데이터들이 많이 있을 것 같아요 :)
단순하게 비지니스 로직만을 책임 지기 때문에 LottoController 에서 더 많은 정보를 요구하고 책임져야할 기능들이 많아진 것 같아요.
적절한 응집으로 LottoController 가 가지고 있는 책임과 역할을 분리해보는 것은 어떨까요?
| @@ -0,0 +1,41 @@ | |||
| package lotto.domain.lotto; | |||
|
|
|||
| public class CountOfManualLottoTicket { | |||
There was a problem hiding this comment.
이 객체의 역할과 책임은 무엇인가요?
행위를 포장해놓은 객체는 의미가 없지 않을까요?
대신 이 행위를 가지고 있어야할 객체를 찾아보는 것도 좋을 것 같습니다 :)
| @@ -0,0 +1,9 @@ | |||
| package lotto.domain.money; | |||
|
|
|||
| public class LottoPrice { | |||
There was a problem hiding this comment.
정적 클래스라면 private 생성자 로 생성을 막아주는 것이 안전한 방법입니다 :)
There was a problem hiding this comment.
이 클래스도 행위를 책임질 객체를 찾지 못하였거나 추출하지 못했기 때문에 애매하게 만들어진 역할을 가지고 있네요 :)
| import lotto.domain.money.LottoPrice; | ||
| import lotto.domain.money.Money; | ||
| import lotto.domain.number.LottoNumber; | ||
| import lotto.domain.number.NumberLinesOfManualLotto; | ||
| import lotto.domain.result.LottoWinningResult; | ||
| import lotto.domain.result.WinningLotto; | ||
|
|
||
| public class LottoController { |
There was a problem hiding this comment.
각 클래스의 역할과 책임을 분리해보면 좋을 것 같습니다.
책임과 역할을 나누었다면 그것을 클래스로 작성하여 LottoController 에서는 큰 행위만 수행하도록 해보시고,
두번째로 리펙토링을 하며, 나누어진 클래스 안에서 책임과 역할을 또 다시 분리할 수 있는지를 보시길 바랍니다.
|
저번 리팩토링을 하며 새로 만든 나름 controller의 역할을 덜어준다고 코드를 다시 짰는데 여기서 더 어떻게 할 수 있을지에 대해서 계속해서 생각을 해봤더니, 정말 말 그대로 controller는 입력받은 결과를 이렇게 생각을 하고 나니 지적해주셨던 아직도 갈 길이 먼 것 같지만..!! 부족한 부분 많이 지적해주시면 더 열심히 수정해보겠습니다~😊 |
| @@ -45,15 +47,11 @@ private void validatePositive(long parsedMoney) { | |||
| } | |||
|
|
|||
| private void validateUnit(long parsedMoney) { | |||
There was a problem hiding this comment.
Money 객체는 얼마를 몇개 구매할 수 있는지를 제공해주어 로또와의 의존을 분리했다면 더 투명한 객체가 되었을 것 같네요 :)
| int countOfManualLottoTicket = manualLottoNumbers.size(); | ||
| countOfAutoLottoTicket = calculateCountOfAutoLottoTicket(countOfAllLotto, countOfManualLottoTicket); | ||
| countOfAutoLottoTicket = countOfAllLotto - countOfManualLottoTicket; | ||
|
|
||
| ManualLottoTicketGenerator manualLottoTicketGenerator = new ManualLottoTicketGenerator(manualLottoNumbers); | ||
| AutoLottoTicketGenerator autoLottoTicketGenerator = new AutoLottoTicketGenerator(countOfAutoLottoTicket); |
There was a problem hiding this comment.
ManualLottoTicketGenerator 와 AutoLottoTicketGenerator 가 위 처럼 되어있다면 매번 생성하는 비효율을 갖고,
인터페이스를 갖는 이점을 모두 갖을 수 없겠네요 :)
인터페이스를 추출한다는 것은 사용하는 쪽에서 구현을 모르게 하여 결합을 분리하겠다는 설계 의도를 갖습니다.
2차 미션 리뷰도 잘 부탁드립니다! 😊