[로또] 박호건 미션 제출합니다. #3
[로또] 박호건 미션 제출합니다. #3moongua404 wants to merge 7 commits intoJava-JavaScript-Language-Stuty:mainfrom
Conversation
- 과제 내용, 코드 흐름, 구현 기능 목록, 처리할 예외 착성 - Mermail를 통해 flow chart 작성
- MVC 패턴 도입 - 파일을 우선적으로 생성, 추후 변동 가능
- 상수 파일의 경우 문자열을, 예외의 경우 예외를, 당첨 관리는 관련 정보를 반환할 수 있음 - 당첨 관리 enum에서는 이에 대한 판별을 포함함
- InputProvider 인터페이스로 의존성 분리 - 테스트 코드 작성
- Lotto 모델은 로또를 관리하는 책임을 가짐 - 당첨에 관한 책임은 LottoPrize로 분리됨
| private void validate(List<Integer> numbers) { | ||
| if (numbers.size() != 6) { | ||
| throw new IllegalArgumentException("[ERROR] 로또 번호는 6개여야 합니다."); | ||
| } |
There was a problem hiding this comment.
다른건 다 utility 사용하고 exceptionConstants throw하게 만드셨던데 얘는 그냥 내비둔 이유가 있을까용? 없으면 저 사이즈 검사도 utility내의 validating 함수로 만드시는게 더 나을듯용
There was a problem hiding this comment.
원본 파일을 수정하지 않으려고 그냥 뒀는데, 코드의 전체적 흐름에서 맞지 않는 것은 사실인 것 같네요...ㅎㅎ
| .toList(); | ||
| } | ||
|
|
||
| public static class LottoRepositoryBuilder { |
| List.of(1, 2, 3, 4, 5, 6), | ||
| List.of(1, 2, 3, 4, 5, 6) | ||
| ); | ||
| } |
seulnan
left a comment
There was a problem hiding this comment.
전체적으로 객체지향적인 설계랑 인터페이스 활용이 굉장히 인상적인 코드였습니다!
다만, 코드 전반에서 분리에 대한 접근이 다소 불필요하게 적극적인 부분이 보입니다
YAGNI 원칙도 한번 고려해보시면 좋을 것 같습니다:)
| lottoRepositoryBuilder = new LottoRepositoryBuilder(); | ||
| } | ||
|
|
||
| public void buyLotto(int amount, Supplier<List<Integer>> pickFunction) { |
There was a problem hiding this comment.
여기에서 randomService의 getLottoNumbers를 사용하지않고 supplier인터페이스를 사용하신 이유가 있을까요?
There was a problem hiding this comment.
테스트를 위해서 였습니다. getLottoNumbers를 내부에서 사용하면 의존성이 생겨 무작위성을 배제한 테스트가 불가능해지지만, 인터페이스로 분리하는건 좀 과한 것 같아서 supplier을 통해 의존성을 분리하여 테스트했습니다.
| @@ -0,0 +1,23 @@ | |||
| package lotto.utils; | |||
|
|
|||
| public enum ExceptionConstants { | |||
There was a problem hiding this comment.
단순하게 final class를 쓰지않고 enum을 쓰신 이유가 있을까요?
There was a problem hiding this comment.
상수 관리를 하려니까 습관적으로 Enum을 썼는데 말씀해 주시니까 다시 정리해보게 되네요ㅎㅎ
일반적인 Enum의 장점은 메모리 관리, 타입 안정성 측면이 있지만 static final로 둔다면 이러한 측면에서 유의미한 차이가 나지 않습니다. 하지만 Error을 붙이는 기능, 가독성 등을 고려했을 때 Enum을 활용하는게 더 적절하다고 생각합니다.
| return value >= min && value <= max; | ||
| } | ||
|
|
||
| public static boolean isInRange(List<Integer> values, int min, int max) { |
There was a problem hiding this comment.
public static boolean isInRange(List values, int min, int max) {
return values.stream().allMatch(value -> value >= min && value <= max);
}
There was a problem hiding this comment.
이런식으로 stream보다는 allMatch()를 사용해서 더 간결하게 코드를 변경하면 어떨까요?
There was a problem hiding this comment.
그런 API가 있었네요! 확실히 코드가 더 간결해지고 명확해지는 것 같아요 감사합니다ㅎㅎ
| package lotto.utils; | ||
|
|
||
| public class LottoConstants { | ||
| public static int LOTTO_PRICE = 1000; |
There was a problem hiding this comment.
final을 놓쳤네요..ㅋㅋㅋㅋ
| return List.of(FIFTH_PRICE, FOURTH_PRICE, THIRD_PRICE, SECOND_PRICE, FIRST_PRICE); | ||
| } | ||
|
|
||
| public static LottoPrize getLottoPrize(int matchCount, boolean matchBonus) { |
There was a problem hiding this comment.
여기에 불필요하게 if문이 많은것같아요 matchCount랑 matchBonus자체를 저장하는 방법은 어떨까요?? 데이터(당첨조건)와 로직(판별메서드)이 분리되어있어서 새로운 당첨규칙이 추가돈다면 두군데를 모두 수정해야해서 유지보수면에서 비효율적일 것 같아요
| public static LottoPrize getLottoPrize(int matchCount, boolean matchBonus) { | |
| public enum LottoPrize { | |
| NOTHING(0, false, "꽝", 0), | |
| FIFTH(3, false, "3개 일치", 5_000), | |
| FOURTH(4, false, "4개 일치", 50_000), | |
| THIRD(5, false, "5개 일치", 1_500_000), | |
| SECOND(5, true, "5개 일치, 보너스 볼 일치", 30_000_000), | |
| FIRST(6, false, "6개 일치", 2_000_000_000); | |
| private final int matchCount; | |
| private final boolean requiresBonus; | |
| private final String condition; | |
| private final int prize; | |
| LottoPrize(int matchCount, boolean requiresBonus, String condition, int prize) { | |
| this.matchCount = matchCount; | |
| this.requiresBonus = requiresBonus; | |
| this.condition = condition; | |
| this.prize = prize; | |
| } | |
| public static LottoPrize findPrize(int matchCount, boolean hasBonus) { | |
| return Arrays.stream(values()) | |
| .filter(prize -> prize.matchCount == matchCount && prize.requiresBonus == hasBonus) | |
| .findFirst() | |
| .orElse(NOTHING); | |
| } | |
| public String getCondition() { | |
| return condition; | |
| } | |
| public int getPrizeMoney() { | |
| return prize; | |
| } | |
| public static List<LottoPrize> getPrizeTypes() { | |
| return List.of(FIFTH, FOURTH, THIRD, SECOND, FIRST); | |
| } | |
| } |
이런식으로 matchCount와 requiresBonus값을 직접 저장하고 findPrize메서드를 저장하면 전체적으로 코드가 깔끔해지고 유지보수도 쉬워질것같습니다
There was a problem hiding this comment.
제가 matchCount와 requiresBonus를 떼어낸 이유는 조건의 다형성 때문입니다. 가령 3,4,5는 갯수로, 1, 2는 보너스 여부로, Nothing은 그 외로 정의하기에, 이를 데이터에 묶인 값으로 관리하기보다는 별도로 구분하는 메서드를 두어 활용하고자 했습니다. 가령 바뀐 코드에서 FIFTH를 5와 false로 봤지만, 4와 true로 볼 공산도 있고, Nothing을 0, false로 정의해뒀지만 실제로는 {(0, false), (1, false), (0, true), (1, true), (2, false)} 범위를 가집니다.
💸 로또 💸
1. 신경써서 구현한 부분
Enum의 활용
인터페이스 활용
빌더 패턴 도입
2. 피드백이 필요한 부분
이번 주차 역시 제가 부족한 부분은 무엇이든 피드백받고싶습니다.
3. 내가 고려한 예외