[로또] 강민혁 미션 제출합니다. #114
[로또] 강민혁 미션 제출합니다. #114
Conversation
|
|
||
| private final LottoStore lottoStore = new LottoStore(); | ||
|
|
||
| public void run() { |
There was a problem hiding this comment.
"함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다."
추가적으로 메서드 내에서 개행은 논리적인 단위를 나누어 가독성을 높이는데 도움을 준답니다.
지금은 모든 줄에 개행이 들어가있는데요. 개행도 잘 사용해보면 좋을 것 같아요!
| final int payment = InputView.inputPayment(); | ||
|
|
||
| final int manualLottoCount = InputView.inputManualLottoCount(); |
There was a problem hiding this comment.
현재 manualLottoCount 가 payment 를 넘어가더라도 프로그램이 실행되네요. 어떻게 해야할까요?
There was a problem hiding this comment.
count가 payment를 넘게되면 exception을 만들어줄 것 같습니다.
|
|
||
| final int manualLottoCount = InputView.inputManualLottoCount(); | ||
|
|
||
| final Lottos manualLottos = InputView.inputManualLottos(manualLottoCount); |
There was a problem hiding this comment.
InputView에서 파싱, 검증, 객체생성을 모두 진행하고 있는데요.
이렇게 구조를 잡으신 이유가 궁금해요!
There was a problem hiding this comment.
원시값은 굳이 분리해야될까? 라는 생각을 했었는데 정확하고 명확한 책임분리를 위해서는 반드시 나눠주는게 맞는 것 같습니다.
| lottoStore.mergeLottos(manualLottos); | ||
|
|
||
| final Lottos userLottos = lottoStore.generateLottosByPayment(payment); |
There was a problem hiding this comment.
맞습니다. 그래서 원래 수동 로또를 Store에 저장하고 그다음에 남은 자동 갯수만큼 만들어서 추가하려했는데 올바르지 않은 설계같습니다. 상태를 가지고 있다가 중간에 무슨일이 일어나서 변경된다면 잘못된 설계같습니다.
| final LottoResult lottoResult = InputView.inputLottoResult(); | ||
|
|
||
| final LottoNumber bonusBall = InputView.inputBonusBall(); |
| @ParameterizedTest | ||
| @ValueSource(ints = {-1, 7, 8, 10}) | ||
| void 유효하지_않은_일치_개수는_MISS를_반환해야_함(int invalidMatchCount) { | ||
| Rank actualRank = Rank.getRankByMatchCount(invalidMatchCount); | ||
|
|
||
| assertThat(actualRank).isEqualTo(Rank.NONE); | ||
| } |
There was a problem hiding this comment.
-1 과 같은 경우는 matchCount가 될 수 없는 값일 것 같은데요.
에러를 반환하는게 더 명확하지 않을까요?
There was a problem hiding this comment.
네 맞습니다. 잘못된 테스트 작성인 것 같습니다 분리하였습니다.
| @Test | ||
| void 상수_순서_검증_테스트() { | ||
| assertThat(Rank.NONE.ordinal()).isEqualTo(0); | ||
| assertThat(Rank.FIFTH.ordinal()).isEqualTo(1); | ||
| assertThat(Rank.FOURTH.ordinal()).isEqualTo(2); | ||
| assertThat(Rank.THIRD.ordinal()).isEqualTo(3); | ||
| assertThat(Rank.SECOND.ordinal()).isEqualTo(4); | ||
| assertThat(Rank.FIRST.ordinal()).isEqualTo(5); | ||
| } |
There was a problem hiding this comment.
제가 처음에 코드를 잘못 작성했을때 자꾸 NONE다음 FIRST가 나왔어서 그걸 테스트 해보고자 작성했던 것 같습니다 ㅋㅋ. 굳이 필요없는 부분인 것 같습니다.
| @ParameterizedTest | ||
| @MethodSource("provideLottosAndWinningResult") | ||
| void 여러_로또의_당첨_통계를_확인한다(Lottos userLottos, Map<Rank, Integer> expectedResult) { |
There was a problem hiding this comment.
@MethodSource를 사용해주셨군요👍
테스트 케이스를 단순한 형태로 작성하기 힘들때 사용하기 좋아요! 저도 자주 사용한답니다!
팁을 드리자면 @MethodSource를 사용할때는 input을 생성하는 메서드의 가독성도 신경써주는게 좋아요! 어떤 케이스를 테스트하는지 바로 파악할 수 있어야해서요!
아래에서의 경우 case1, case2 등 변수로 따로 빼지 않고, 로또와 결과를 같은곳에서 볼 수 있게 하는게 어떨까 했어요
| class LottoResultTest { | ||
|
|
||
| private List<Integer> inputNumbers; | ||
| private List<LottoNumber> expectedLottoNumbers; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| inputNumbers = new ArrayList<>(); | ||
| expectedLottoNumbers = new ArrayList<>(); | ||
|
|
||
| for (int i = 1; i <= 6; i++) { | ||
| inputNumbers.add(i); | ||
| expectedLottoNumbers.add(new LottoNumber(i)); | ||
| } | ||
| } |
There was a problem hiding this comment.
@beforeeach를 써주셨군요👍
@nested를 사용해 테스트를 논리적인 개념에 따라 분리해보면 어떨까요?
그렇게되면 inputNumbers, expectedLottoNumbers 도 사용하는 내부 클래스의 필드로 제한할 수 있을 것 같아요.
| public boolean isContainBonusBall(final LottoNumber bonusBall) { | ||
| return numbers.contains(bonusBall); | ||
| } |
There was a problem hiding this comment.
"너 보너스볼을 가지고 있어?"를 Lotto에게 물어봐야할까요?
이렇게 되면 Lotto가 보너스볼이라는 규칙을 알게되는건데, 이러한 규칙에 변경이 생기면 또 Lotto 코드를 수정해야하지 않을까요?
There was a problem hiding this comment.
아닙니다. LottoResultChecker가 Lotto에게 hasBonusBall이라는 메서드를 통해 물어보는게 더 바람직한 설계 같습니다!
| private LottoNumber(final int number) { | ||
| this.number = number; | ||
| } | ||
|
|
||
| public static LottoNumber of(final int number) { | ||
| validateNumberRange(number); | ||
| return CACHE.get(number); | ||
| } |
There was a problem hiding this comment.
캐싱 잘 반영해주셨네요👍
검증 및 캐시에서 객체를 꺼내오는 작업은 생성자에서 진행해주는게 좋을 것 같아요!
왜 그래야할지 생각해보시면 좋을 것 같아요:)
| public static Rank valueOf(int matchCount, boolean bonusMatch) { | ||
| validateMatchCount(matchCount); | ||
|
|
||
| if (matchCount == 6) { | ||
| return FIRST; | ||
| } | ||
| if (matchCount == 5) { | ||
| return bonusMatch ? SECOND : THIRD; | ||
| } | ||
| if (matchCount == 4) { | ||
| return FOURTH; | ||
| } | ||
| if (matchCount == 3) { | ||
| return FIFTH; | ||
| } | ||
| return NONE; | ||
| } | ||
|
|
||
| public static Rank valueOf(int matchCount) { | ||
| validateMatchCount(matchCount); | ||
| return MATCH_COUNT_TO_RANK.getOrDefault(matchCount, NONE); | ||
| } |
There was a problem hiding this comment.
민혁님 valueOf()는 왜 두개의 메서드가 있는걸까요?
추가적으로 enum을 잘 사용해보면 분기문을 없앨 수 있습니다!
지금의 구조라면 6개, 5개 등 개수에 대한 관리는 두곳에서 하게되는걸로 보여요!
이부분은 필수적으로 수정해주시면 좋을 것 같아요!
|
민혁님 추가 코멘트 달아두었습니다! |
로또 - 클린 코드
진행 과정
저번 미션 진행했을 때, 다른 분들 PR보면서 제가 느꼈을 때, 많은 패키지와 많은 클래스로 작성하신 분들도 있었고, 진짜 간단하게 그냥 main 패키지에 패키지 분리 안하고 그냥 작성하신 분들도 계시더라고요, 제가 느낀점은 너무많아도 이해하기 어렵고, 너무 안나눠 놓으니 책임 분리가 안되버리는 것 같아서 저는 이번미션에서 그 중간점을 찾으려고 최대한 노력해봤습니다. 제 코드도 읽어보니 굳이 필요없이 패키지 나눴나? 이런 생각도 들었고요.
일단 이번 미션을 진행하면서 가장 신경써야 할 부분으로 코드 가독성이였습니다.
미션 제목 자체도 클린 코드이고 그래서 가독성을 신경쓰고 작성해야 되는 문제라고 생각했습니다.
이번에 진행하면서 조금 새롭게 알게된 부분은 두가지 정도 있었는데요,
첫번째는 Iterable 인터페이스 입니다.
로또 목록을 관리하는 Lottos 클래스에 Iterable 인터페이스를 구현함으로써 컬렉션을 훨씬 더 직관적으로 순회할 수 있게 되었습니다. 이렇게 함으로써 for-each 루프를 사용하여 로또 목록을 쉽게 순회할 수 있게 되어 클라이언트 코드의 가독성과 사용성을 향상시켰습니다. 일급 컬렉션 내부에 getLottos로 return을 해주면 가장 문제점이 lottos.getLottos() ~ 이런식으로 쓸데없는 코드? 가 생기는 상황이 있었고요, 이걸 해결할려면 Iterable 인터페이스를 받아서 구현시 이점이 있다고 찾아봤습니다. 내부 컬렉션을 외부에 노출하지 않으면서 사용가능하다는 장점이 있었습니다.
두번째는 EnumMap입니다. 로또 등수와 당첨 개수를 매핑할 때 일반 HashMap 대신 EnumMap을 사용함으로써 성능과 타입 안전성을 모두 확보할 수 있었습니다. EnumMap은 enum 키에 최적화된 맵 구현체로, 내부적으로 배열을 사용하기 때문에 일반 HashMap보다 더 효율적이라고 하더라구요, 또한 enum 값만 키로 사용할 수 있기 때문에 타입 안전성도 보장됩니다. 저 같은경우 Rank를 key로 가지는 EnumMap을 만들었고, 여기에 매개변수로 Rank.class를 넣어주면 Rank의 모든 enum 타입을 key로 넣어줘서 더 가독성 좋은 코드로 바뀌었던 것 같습니다.
가장 헷갈리고 어려웠던 부분은 변수명, 메서드명, 객체명정하기 였습니다.
영어를 잘하는게 아니라 일단 원하는 이름을 찾아보면서 구현했었는데요 막상 적어놓고 나서 보니 제가 써놓고도 이 객체, 변수, 메서드가 뭘 의미하는지 모호했습니다. 최대한 객체당 단일 책임을 가지도록 유도했지만 아닌부분도 있을 수 있을 것 같습니다.
그리고 확실히 처음부터 요구사항이 모두 요청된게 아니라, 단계를 거쳐가며 요구사항이 생기다 보니, 중간중간 코드가 꼬이고 바꿔야 할 부분이 많았습니다. Rank를 추가하니 많은 클래스에서 값이 바뀌어야 했고 많이 바뀌게 되면 결국에 객체지향 설계를 잘 지키지 못했나? 이런생각도 조금 들었습니다.
그리고 이번 미션에서는 굳이 Exception을 상속하고 관리하는 것 보다 도메인 로직에 집중하기 위해서 그냥 RuntimeException으로 통일 해서 진행했습니다. (물론 CustomException을 구현하여 사용하는 것이 더 좋다고 생각합니다!)
1단계 - 로또 자동 구매
1단계에서의 핵심은 저번 미션에서 숫자를 generate하는 함수형 인터페이스 대신에, 힌트에 나와있는 Collections.shuffle()을 사용하기 위한 방식으로 조금 변경이 있었습니다. 하나의 컬렉션에 1부터 45까지 담어놓고 shuffle() 메서드를 반복적으로 사용하면서 하나의 컬렉션의 재사용성을 높이고 자바 표준 프레임워크를 사용하게 되었을 때, 다른 개발자가 읽었을 때, 굳이 찾아보지 않아도 알기 쉬운 코드라고 생각이 들었습니다. (Collections.sort() 도 마찬가지)
2단계 - 로또 당첨
2단계에서는 로또 당첨 로직을 구현했습니다. Rank 열거형을 통해 로또 등수를 명확하게 표현했고, LottoResult 클래스에서 로또 당첨 번호와 사용자 로또를 비교하여 일치하는 개수를 계산했습니다. 또한 LottoResultChecker를 통해 여러 장의 로또에 대한 당첨 결과를 처리하고, WinningResult 클래스로 당첨 통계와 상금을 관리했던 것 같습니다. 가장 신경썼던 부분은 단일 책임 원칙을 위배하지 않도록 객체를 설계하려고 했고 이 부분이 모호하게 느껴지는 부분이 조금 있었습니다. 그리고, LottoResult도 결국 로또 번호 6개를 가지는 Lotto와 같은 객체라고 보았고 LottoResult를 Lotto를 상속하도록 만들어서 조금 더 객체지향적으로 개발하려 했습니다.
3단계 - 로또 2등 당첨
3단계에서는 보너스 볼을 추가하여 2등 당첨 조건을 구현했습니다. 이를 위해 기존 Rank 열거형을 리팩토링하여 보너스 볼 일치 여부를 포함하도록 변경했습니다. 특히, 5개 일치 시 보너스 볼 일치 여부에 따라 2등 또는 3등을 결정하는 로직을 추가했습니다.
Lotto 클래스에 isContainBonusBall 메서드를 추가하여 보너스 볼 포함 여부를 확인할 수 있게 했으며, 이를 통해 LottoResultChecker에서 명확한 등수 결정이 가능해졌습니다. 열거형 내부에 보너스볼 메서드를 추가하고 결과 확인 객체에 추가만 한번 해주면 간단히 해결되었던 것 같습니다.
4단계 - 로또 수동 입력
마지막 단계에서는 사용자가 직접 로또 번호를 입력할 수 있는 기능을 구현했습니다. Lottos 클래스를 도입하여 여러 장의 로또를 효율적으로 관리할 수 있게 했고, LottoStore에 수동 로또 추가 및 검증 기능을 추가했습니다.
Lottos 클래스는 Iterable 인터페이스를 구현하여 로또 목록을 순회하기 쉽게 만들었습니다.