Step3 - 로또(2등) #4217
Step3 - 로또(2등) #4217
Conversation
- 피드백 받은 부분개선 - BeforeNum 을 도입한 '확장성'은 당장 불필요하다고 판단 -> Lotto로 변환 - Lotto 생성자 유효성 검사 추가 - LottoFactory과 LottoMachine이 비슷한 역할을 한다고 판단해 팩토리 삭제 - 컨트롤러 역할인 LottoStore 의 협력구성의 가독성 개선
- 보너스볼 요구사항 추가 - 당첨번호 vs 로또번호 로 등수결과 추출 로직에 보너스볼 추가되게 변경 및 테스트 - WinStandard에 2등관련 작업 추가 - WinStandard findByValue 에 보너스볼 여부 파라미터 추가 - WinStandard 테스트코드 추가
- 추가 요구사항에 따른 관련 객체 캡슐화 => WinnerResult 객체의 필드를 List -> Map으로 변경 및 분산되어 있던 당첨결과 로직을 WinnerResult로 응집 - 관련해서 불필요한 클래스와 일부 협력 플로우 변경
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
3단계 구현하느라 수고했어요. 👍
요구사항 및 개선을 위해 WinnerResult 클래스의 내부를 갈아엎는정도의 리팩토링을 진행했는데요.
WinnerResult 내부에서 변경작업을 마친후 WinnerResult 과 의존성이 있는 객체들의 에러를 해결하려고 테스트코드를 돌렸는데 모든 테스트가 성공하더라구요
초반 객체의 캡슐화가 잘되어있다는 방증이었겠죠?
정말 소중한 경험을 했네요.
OOP의 맛이 이런 맛인 것 같아요.
캡슐화를 잘하고 응집도가 높은 코드를 구현해 놓으면 새로운 기능을 추가/변경, 리팩터링했을 때 외부의 변경을 최소할 수 있다는 장점이 있어요.
즉, 변경에 빠르게 대응할 수 있는 코드가 가능해 지는거죠.
이번 경험이 OOP의 맛을 느끼는데 소중한 경험이 되었기를 바랍니다.
| import static lottoGame.view.Casher.askBeforeWinNums; | ||
| import static lottoGame.view.Casher.askBonusNum; | ||
| import static lottoGame.view.Casher.askBuyPrice; | ||
| import static lottoGame.view.Casher.informBuyCount; | ||
| import static lottoGame.view.Casher.informPublishedLottos; | ||
| import static lottoGame.view.Casher.informWinningLottoNums; | ||
| import static lottoGame.view.Casher.informWinResult; |
There was a problem hiding this comment.
| import static lottoGame.view.Casher.askBeforeWinNums; | |
| import static lottoGame.view.Casher.askBonusNum; | |
| import static lottoGame.view.Casher.askBuyPrice; | |
| import static lottoGame.view.Casher.informBuyCount; | |
| import static lottoGame.view.Casher.informPublishedLottos; | |
| import static lottoGame.view.Casher.informWinningLottoNums; | |
| import static lottoGame.view.Casher.informWinResult; | |
| import static lottoGame.view.Casher.*; |
| import static lottoGame.view.Casher.informWinResult; | ||
|
|
||
| import java.util.List; | ||
| import lottoGame.model.lotto.Lotto; |
There was a problem hiding this comment.
| import lottoGame.model.lotto.Lotto; | |
| import lottogame.model.lotto.Lotto; |
패키지 이름은 전체가 소문자
| Lottos lottos = buyLottos( | ||
| lottoMachine, | ||
| buyPrice | ||
| ); |
There was a problem hiding this comment.
| Lottos lottos = buyLottos( | |
| lottoMachine, | |
| buyPrice | |
| ); | |
| Lottos lottos = buyLottos(lottoMachine, buyPrice); |
지금까지 계속해서 이런 스타일을 구현하고 있음.
그런데 대부분의 다른 개발자들이 구현하는 스타일은 위와 같음.
앞으로 다른 개발자와의 소통을 위해 위와 같은 스타일로 구현하도록 습관을 바꿔보면 어떨까?
지금 바꾸지 않으면 추후에 변경하기 힘들 것으로 예상되어 피드백 남겨봄
| } | ||
|
|
||
|
|
||
| private BuyPrice getBuyPrice() { |
There was a problem hiding this comment.
BuyPrice 이름에 대해 AI에 피드백을 받아볼 것을 추천
이름만으로 무슨 역할을 맡은 객체인지 막연한 느낌이 듦
| Lotto beforeWinLotto, | ||
| Lottos lottos, | ||
| int price, | ||
| LottoNum bonusNum |
There was a problem hiding this comment.
| Lotto beforeWinLotto, | |
| Lottos lottos, | |
| int price, | |
| LottoNum bonusNum | |
| WinnerResult winnerResult, | |
| int price |
메서드 인자가 많은데 위와 같이 결과를 전달하도록 접근해도 되지 않을까?
| .filter(beforeWinNums::isContain) | ||
|
|
||
| // 아래 카운트, 보너스 체크로직을 새로운 당첨객체값에 적용해야 할듯..? | ||
| public WinStandard checkIfWin(Lotto beforeWinLotto, LottoNum bonusNum) { |
There was a problem hiding this comment.
Lotto beforeWinLotto, LottoNum bonusNum 두개의 값은 서로 연관되어 있는데 WinningLotto와 같은 객체로 추상화 해보는 것은 어떨까?
| return WinStandard.findByValue( | ||
| Long.valueOf(result).intValue() | ||
| return WinStandard.findBy( | ||
| Long.valueOf(matchCount).intValue(), |
There was a problem hiding this comment.
처음부터 long이 아닌 int 값으로 값을 가져올 수는 없을까?
ai에게 방법을 물어보고 개선해 본다.
| /** | ||
| * TODO : 피드백 주신부분 확인했습니다. | ||
| * 아래 부분의 Set은 단순 중복제거 용도로 사용했습니다 | ||
| * List를 밖으로 응답하는 이유는 당장은 외부에서 해당 메서드를 호출시 응답 결과의 갯수보장 vs 정렬관련 편의성 중 선택하는 상황이었고 | ||
| * 두 방식의 차이는 Set보다는 순서개념이 존재하는(정렬할수 있는) List가 더 유용할거라는 판단이었습니다. | ||
| * 관련해서 고견 있으시면 다시 한번 피드백 주시면 더 고민해보겠습니다! 감사합니다! | ||
| * */ |
There was a problem hiding this comment.
이는 선택의 문제라 생각함
단, Lotto 가지는 속성은 중복값을 허용하지 않는 6개의 숫자 값이기 때문에 Set으로 6개의 필드 값을 가지는 것이 낫지 않을까?
정렬된 List 값은 UI에 출력할 때 필요한 것이지 내부 로직을 구현할 때는 정렬이 되어 있지 않아도 괜찮으니 외부에서 값을 구할 때 List로 정렬해 반환해도 되지 않을까 생각함
| SECOND("5개 일치 (1500000원)-", 5, 1500000), | ||
| THIRD("4개 일치 (50000원)-", 4, 50000), | ||
| FOURTH("3개 일치 (5000원)-", 3, 5000), | ||
| FIRST("6개 일치 (2000000000원)- ", 6, false, 2_000_000_000), |
There was a problem hiding this comment.
"6개 일치 (2000000000원)- " 값은 UI에 종속된 값인데 enum에서 제거해도 되지 않을까?
| @@ -38,4 +45,17 @@ public String desc() { | |||
| public int returnOfWin() { | |||
| return this.returnOfWin; | |||
| } | |||
|
|
|||
| public static Map<WinStandard, Integer> getInitWinStandardMap() { | |||
There was a problem hiding this comment.
WinStandard.values()를 활용하면 enum의 모든 값을 구할 수 있음.
Map을 초기화하는 이 부분은 WinnerResult가 책임지는 것이 어떨까?
- lottoGame 패키지명을 lottogame로 개선 - 당첨번호,보너스볼을 관리하는 WinningLottoNums 객체 생성 - 당첨결과 추출하는 행위를 Lotto에서 WinningLottoNums로 이관
- WinStandard 의 UI 메세지를 Casher 클래스로 위임 - UI 위임으로 인한 LottoStore 내 메서드 제거 - WinnerResult 초기화 로직 캡슐화
- Lotto의 필드값을 List에서 Set으로 변경 - UI 노출 부분도 정렬된 List 출력으로 개선
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
피드백 반영 잘 했네요. 👍
추가로 LottoNum과 같이 불변 객체로 구현할 때 발생할 수 있는 성능 이슈가 해결하기 위한 도전을 해봤으면 하는 바람으로 피드백 남겼어요.
45개의 LottoNum을 생성해 재사용할 수 있는 방법을 찾아보면 좋겠습니다.
| Set<LottoNum> lottoByNums = lottoMachine.createLottoByNums(askBeforeWinNums()); | ||
| int bonusNum = askBonusNum(); | ||
|
|
||
| return new WinningLottoNums(new Lotto(lottoByNums), new LottoNum(bonusNum)); |
There was a problem hiding this comment.
| return new WinningLottoNums(new Lotto(lottoByNums), new LottoNum(bonusNum)); | |
| return new WinningLottoNums(lottoByNums, bonusNum); |
객체에 대한 포장은 WinningLottoNums 내부 생성자에서 하도록 하면 위와 같이 구현할 수 있지 않을까?
다양한 타입의 부 생성자를 지원해 본다.
|
|
||
| public class LottoMachine { | ||
|
|
||
| public static final int LOTTO_NUM_COUNT = 6; |
There was a problem hiding this comment.
Lotto에도 같은 상수 값이 있는데 이 값을 사용하면 어떨까?
| private final Lotto winLotto; | ||
| private final LottoNum bonusNums; | ||
|
|
||
| public WinningLottoNums(Lotto winLotto, LottoNum bonusNums) { |
There was a problem hiding this comment.
보너스 번호가 Lotto의 값과 중복 되는지에 대한 유효성 체크를 해야하지 않을까?
| return this.winStandardToWinCount.get(winStandard); | ||
| } | ||
|
|
||
| public double calculateRateOfReturn(int buyPrice) { |
There was a problem hiding this comment.
규칙 3: 모든 원시값과 문자열을 포장한다. 원칙을 적용해 돈과 관련한 로직 구현을 이 객체가 담당하도록 구현해 보는 것은 어떨까?
|
|
||
| private final Set<LottoNum> lottoNums; | ||
|
|
||
| public Lotto(Set<LottoNum> lottoNums) { |
There was a problem hiding this comment.
| public Lotto(Set<LottoNum> lottoNums) { | |
| public Lotto(int... lottoNums) { | |
| public Lotto(String commaText) { | |
| public Lotto(LottoNum... lottoNums) { |
이와 같이 여러 부 생성자를 추가하는 것은 어떨까?
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class LottoNum implements Comparable<LottoNum>{ |
There was a problem hiding this comment.
상황
- 1만명의 사용자가 동시에 당첨 여부를 확인할 수 있어야 한다.
- 1명의 사용자는 평균 5장의 로또를 구매한 상태이다.
위 요구사항을 서버에서 생성되는 LottoNumber의 인스턴스의 갯수는?
1 * 5장 * 6개 숫자/1장 * 1만명 = 30만개이다.
동시에 생성되는 인스턴스 갯수가 너무 많다.
인스턴스 갯수를 줄일 수 있는 방법은?
로또 숫자 값을 LottoNumber 객체로 래핑하는 경우 매번 인스턴스가 생성되기 때문에 인스턴스의 갯수가 너무 많아져 성능이 떨어질 수 있다.
LottoNumber 인스턴스를 생성한 후 재사용할 수 있도록 구현한다.
힌트 : Map과 같은 곳에 인스턴스를 생성한 후 재사용하는 방법을 찾아본다.
- WinningLottoNums 부생성자 추가 - 피드백기반으로 WinnerResult.calculateRateOfReturn()는 단일책임원칙을 위반한다고 판단되어 로직 분리 - 중복 상수값 삭제
- 인스턴스 과부하 방지를 위한 static LottoNum 객체풀 생성 - 당첨번호와 보너스볼 중첩 예외처리
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
LottoNum에 대한 캐싱 적용 👍
단, 캐싱 적용에 추가로 고민해 봤으면 하는 부분이 있어 피드백 남겨봤어요.
이 단계에 남긴 피드백은 4단계 진행할 때 함께 반영해 보세요.
4단계 진행할 때 가능하면 컴파일 에러를 최소화하면서 점진적으로 리팩터링해보는 연습을 해볼 것을 추천합니다.
| ); | ||
| } | ||
|
|
||
| public Lottos publish(LottoPurchasePrice lottoPurchasePrice) { |
There was a problem hiding this comment.
이 메서드 이름이 의도를 파악하는데 어려움이 있는 느낌
AI 피드백을 통해 의도를 파악할 수 있는 이름에 대한 피드백 받아보면 어떨가?
| import java.util.stream.Collectors; | ||
| import lottogame.model.price.LottoPurchasePrice; | ||
|
|
||
| public class LottoMachine { |
There was a problem hiding this comment.
이 객체는 상태 값을 가지지 않고 있음.
그렇다면 굳이 인스턴스를 생성할 필요없이 모든 메서드를 클래스 메서드로 구현해도 되지 않을까?
|
|
||
| private final int num; | ||
|
|
||
| public LottoNum(int num) { |
There was a problem hiding this comment.
캐시 적용 👍
단 이와 같이 구현할 경우 캐시를 사용하지 않고 LottoNumber를 직접 생성 가능하다.
LottoNumber를 외부에서 직접 생성하지 못하도록 private으로 구현하고 캐시 효과를 내도록 리팩터링해본다.
힌트: 정적 팩터리 메서드
| } | ||
|
|
||
| private int calculateWinReturnBy(WinStandard winStandard) { | ||
| return winStandard.returnOfWin() * this.winStandardToWinCount.get(winStandard); |
There was a problem hiding this comment.
| return winStandard.returnOfWin() * this.winStandardToWinCount.get(winStandard); | |
| return winStandard.calculateWin(findWinCount(winStandard)); |
enum도 클래스와 똑같은 속성을 가지고 있음.
따라서 위와 같이 메시지를 보내는 방식으로도 구현 가능함
2 participants
로또 Step3 - 로또(2둥) 결과물 입니다.( master 브랜치에 실수로 잘못 PR 날렸네요.. 취소하고 다시 PR 드립니다.)
개인적으로 이번 작업중 좋은 경험을 했습니다.
요구사항 및 개선을 위해 WinnerResult 클래스의 내부를 갈아엎는정도의 리팩토링을 진행했는데요.
WinnerResult 내부에서 변경작업을 마친후 WinnerResult 과 의존성이 있는 객체들의 에러를 해결하려고 테스트코드를 돌렸는데 모든 테스트가 성공하더라구요
초반 객체의 캡슐화가 잘되어있다는 방증이었겠죠?
누군가에겐 별거 아니겠지만 저에겐 이 경험이 성장의 산물인것 같아 뿌듯하네요.
좋은 가르침 주셔서 감사하고 앞으로 더 잘 부탁드리겠습니다!
이전에 피드백 주신 부분은 거의 적용하긴 했지만 일부는 고민의 여지가 있는것 같아 코드상에 TODO 주석으로 생각몇자 끄적였습니다.
바쁘신 와중에 정성스런 피드백 주셔서 감사합니다!