[2단계 - 로또 구현] 포츈(정윤성) 미션 제출합니다. #312
Conversation
[JDK] 생성자 타입 오버로딩 - 1내용
private final int value;
public LottoNumber(final String value) {
this(Integer.parseInt(value));
}
public LottoNumber(final int value){
validateRange(value);
this.value = value;
}[OOP] 팩토리 메소드 패턴과 캐싱 - 2내용
링크
[JDK] 내가 만든 객체를 정렬하기 - 2내용
링크 |
There was a problem hiding this comment.
업무가 바빠 리뷰가 늦었네요 미안해요 😭
LottoManualTicketCount 역할은 필요할지 의문이 들어요. LottoManualTicketCount 가 없어도 로또를 생성하고 로또 게임을 실행하는데는 아무런 무리가 없을 것같아요. 추가적은 내용은 커멘트로 남겨놓았습니다. 확인 후 리팩토링해보면 좋을 것 같아요.
자동으로 발급된 로또와 수동으로 발급된 로또는 발급 하는 방식외에는 모두 같은 성질을 공유하고 있어 둘을 다른 클래스로 나눴을때 장점이 없을 것 같아요. 포츈은 나눠서 어떤 이점을 얻으셨나요 ? 🤔
위 부분은 고민 하고 다음 미션을 수행하는 것이 좋을 것 같습니다. 고민 해본 후에 다시 PR 주세요.
디엠이나 코멘트도 환영입니다.
| @@ -0,0 +1,33 @@ | |||
| package lottogame.domain; | |||
|
|
|||
| public class LottoManualTicketCount { | |||
There was a problem hiding this comment.
카운트가 꼭 필요한 객체인지 의문이에요
View 단에서 넘겨받는 정보를 List<String> numbers 라고 한다면 리스트의 크기가 수동 로또의 갯수이니
로또를 생성하는 입장에서는 가격과 수동 로또의 번호만 알면 충분히 로또를 만들어 낼 수 있을 것 같아요.
There was a problem hiding this comment.
포장의 용도로써는 필요한 객체지만, 패키지 구조상 위치가 적절하지 않았어요!
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class LottoGameUtils { |
There was a problem hiding this comment.
객체지향에서 유틸리티 클래스는 최대한 사용하지 않기를 권장하고 있어요.
해당 객체의 도움 없이 로또를 생성하도록 리펙토링하면 좋을 것 같아요.
| public class LottoGameUtils { | ||
| public static final String DELIMITER = ","; | ||
|
|
||
| public static LottoNumbers getLottoNumbersByInputString(String lottoNumbers) { |
There was a problem hiding this comment.
LottoNumbers 가 충분히 혼자서도 할 수 있는일 인것 같아요.
There was a problem hiding this comment.
로또 Utils의 역할을 LottoNumbers로 옮겨 넣었어요!
| private static List<LottoNumber> getSplitLottoNumbersByString(String lottoNumbers) { | ||
| lottoNumbers = lottoNumbers.replaceAll(" ", ""); | ||
| final List<LottoNumber> lottoNumberGroup = new ArrayList<>(); | ||
|
|
||
| for (String number : lottoNumbers.split(DELIMITER)) { | ||
| lottoNumberGroup.add(LottoNumber.of(number)); | ||
| } | ||
| return lottoNumberGroup; | ||
| } |
There was a problem hiding this comment.
기존 util 에 있던 문자열을 잘라내면서 lottoNumber로 만드는 작업을 LottoNumbers로 이관시켜줬어요.
이전에는 중복되는 기능이니까 유틸로 빼야지 ~ 하고 넘겼었는데, 다음부터는 이 책임을 적절히 수행할 수 있는 다른 객체가 있는지 한번더 찾아볼것 같아요.
| public static LottoTicket of() { | ||
| return new LottoTicket(getAutoNumbers(), true); | ||
| } | ||
|
|
||
| public static LottoTicket of(final LottoNumbers lottoNumbers){ | ||
| return new LottoTicket(lottoNumbers, false); | ||
| } |
There was a problem hiding this comment.
두개로 나누었던 로또 티켓의 종류를 다시 한 클래스로 합쳤어요.
책임을 줄이겠다고 나눈거였는데, 오히려 나누어보니 기존 10이었던 책임이 5 + 5 가 아닌 6 + 6 이 되는 경험을 해봤고,
무조건 쪼갠다는것이 능사가 아님을 배웠네요.
그리고 팩토리 메서드 패턴을 이용해서 파라미터의 개수에 따라 오토로 만들지, 수동으로 결정하게 했어요
| private static List<String> getManualNumbersGroup(LottoTicketCount lottoTicketCount) { | ||
| List<String> manualLottoNumbers = new ArrayList<>(); | ||
| while (lottoTicketCount.isRemain()) { | ||
| lottoTicketCount.reduce(); | ||
| manualLottoNumbers.add(getManualNumbers()); | ||
| } | ||
| return manualLottoNumbers; | ||
| } |
There was a problem hiding this comment.
리팩토링을 진행하며 제일 많이 배운부분이 이 파트인것 같네요.
뷰에서 도메인으로 넘길때, 로또 넘버들의 리스트를 넘기게 리팩토링했어요.
과연 해당 정보를 도메인이 굳이 알아야할지? 에 대해서 고민하게 되는 좋은 경험을 해본것 같아요.
그리고 뷰에서도 어느정도 책임을 질수 있다는것을 알게 되었고, 틀에 박혀있던 생각을 좀 더 확장할 수 있었어요.
안녕하세요 미립
이번 리팩토링을 진행하면서, 이 코드가 당장 제가 몇일전에 만든 코드임에도 불구하고
레거시 코드를 수정한다는것이 얼마나 힘든일지를 깨닫는 경험을 할 수 있었어요.
이번 리팩토링에서
기존에 수동 티켓을 추가하면서 인터페이스를 의식적으로 사용해봤어요.
이 과정에서 미립이 마지막으로 준 피드백인
부분도 어느정도 같이 해소가 된것 같아요.
저는 로또 티켓이 로또 번호를 들고 있는것이지, 티켓이 로또 번호가 된다고 생각하지 않았고,
로또 번호는 로또 당첨여부를 확인할때도 쓰이는데 그것은 번호 로 확인하는것이지 티켓으로 확인 한다고 생각하지 않았어요.
반대로 로또 번호들의 묶음을 자의적으로 티켓 이라고 생각하는것도 이상하다고 생각했어요.
그리고 이번 단계에서 로또 티켓을 두가지로 나누며 로또 번호를 따로 관리하길 잘했다는 생각이 조금 들더 라구요.
이렇게 리팩토링을 하니
LottoNumbers의 역할과LottoAutoTicket,LottoManualTicket의 역할이 좀 더 명확하게 분리된 느낌을 받았어요.아래부터는 미션을 진행하며 궁금했던 것이 몇가지 있는데 이어서 질문 드릴게요
LottoManualTicketCount 의 책임이 어디까지면 좋을까요?
LottoManualTicketCount는 수동으로 구매할 로또의 수를 포장한 객체에요.여기서 고민했던점은
이 두가지 방법에 대한 고민이 있었어요.
get하고 for 문을 돌리게 한다면, 불변객체로 유지할 수 있다는 장점이 있어서 처음에는 그렇게 구현 했지만,
좀 더 다시 고민해보니 그냥 굳이 불변을 유지하는것보다 try되는 횟수를 스스로 책임지게 하는것이 맞는것 같아서 리팩토링 했어요.
과연 어떤 상황에서 어떤식으로 하는게 옳을지가 궁금하네요
제가 인터페이스로 두개를 나눈 방식이 괜찮은 방법일까요?
LottoTicket인터페이스를 두고LottoAutoTicket,LottoManualTicket두개로 쪼갰어요.쉽게 구현한다면 기존
LottoTicket에서 본인이 auto인지 여부를 확인하는 flag 만 하나두고, flag에 맞춰서 자신의 상태를 관리하며 로또 티켓을 발급해주는 방법이 있어 보였어요.하지만 이 방법은
LottoTicket의 책임이 너무 무거워 진다고 생각이 들어 두개의 객체로 쪼갰네요.그러다보니 테스트 코드의 변경이 굉장히 많이 일어나고, 리팩토링하는데 있어서 고생을 했네요.
제가 이렇게 두개로 나누어 새롭게 객체를 구성한 방식이 괜찮은 방법인지가 궁금하네요.
추가 내용
미립이 해준말 기억하고 있어요
열심히 리뷰해주시면 리뷰내용에 대해서 즐겁게 리팩토링 해볼게요