[로또 미션] 김의천 미션 제출합니다. #71
Conversation
- 발행된 로또 값들을 그대로 로또 클래스에 생성자로 넣기 때문에, 모킹이 필요하지 않으며, 상속 또한 필요하지 않음
There was a problem hiding this comment.
안녕하세요 요토오~~!!! 명절이라고 다음주까지 리뷰를 해도 된다고 하셨지만! 그 김에 이번엔 오래 핑퐁하는 것도 좋아보여서 리뷰를 남깁니다. 아 진짜 토끼는 코딩 천재인가요? 왤케 잘하세요 자바 처음 한다며요;;ㅠ 컬렉션 개념이 확실하게 잡혀있는 것 같아서 감탄했답니다.
저는 아직 제가 쓰고 있는게 컬렉션이 맞나 의문이 들거든요. 그냥 한번 감싼걸 또 감싼다 -> 일급컬렉션
이렇게 이해하고 있는데 맞나요?ㅋㅋㅋㅠ 그래서 저는 로또 숫자도 클래스 만들고 그거로 로또티켓 만드는 형식으로 일급컬렉션 구현하려고 했거든요..
이번 리뷰는 워낙 너무 잘해주셨기도 하고 코드에 의문이 드는 부분이 많이 없어서 enum쪽을 위주로 리뷰해보았습니다.
이외에도 토론하고 싶은 주제가 있다면 언제든 말해주세요!
풀스텍 요토가 앞으로 단 한걸음 남았네요><ㅋㅋ
| import java.util.*; | ||
|
|
||
| public class LottoList { | ||
| List<Lotto> lottoList; |
There was a problem hiding this comment.
클래스에서 lottoList 필드를 final로 선언하지 않은 이유가 있나요?
다른 클래스들에서는 보통 final로 선언되어있는 것 같더라구요
| package model; | ||
|
|
||
| public enum LottoPrizeConstants { | ||
| FIRST_PRIZE_MONEY(2_000_000_000), |
There was a problem hiding this comment.
지금은 단순히 상수 값을 저장하는 용도로만 사용되고 있는데, 이런 식으로 enum을 사용한다면 연관된 여러개의 데이터를 함께 처리할 수 있을 것으로 보여요
| FIRST_PRIZE_MONEY(2_000_000_000), | |
| FIRST(6, 2_000_000_000, "6개 일치"), | |
| SECOND(5, 30_000_000, "5개 일치, 보너스볼 일치"), |
보너스볼 당첨 정보도 함께 다룬다면 로직들이 더 간단해지지 않을까요?
| FIRST_PRIZE_MONEY(2_000_000_000), | |
| FIRST(6, 2_000_000_000, false,"6개 일치"), | |
| SECOND(5, 30_000_000, true, "5개 일치, 보너스볼 일치"), |
| import java.util.stream.IntStream; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class LottoGameController { |
There was a problem hiding this comment.
컨트롤러가 너무 많은 상태를 가지고 있는 것 같은데 꼭 필수적인 상태 주입을 제외하고 줄여보는 건 어떨까요?
| import java.util.Random; | ||
|
|
||
| public class LottoGenerator { | ||
| Random random = new Random(); |
There was a problem hiding this comment.
구입금액만을 가지고 있는 클래스가 필요한 이유가 무엇일까요?
입력 받은 숫자의 불변성을 보장하고 싶은 것이 의도라고 생각되는데 그렇다면 컨트롤러에 선언되어있는 lottoCount 필드�가 없어야 할 것 같아요. 어떻게 생각하시나요?
|
|
||
| private void validateBonusNumber(int bonusNumber) { | ||
| if (bonusNumber < LottoConstants.MIN_NUMBER.getValue() || bonusNumber > LottoConstants.MAX_NUMBER.getValue()) { | ||
| throw new Error("보너스 넘버는 " + LottoConstants.MIN_NUMBER.getValue() |
There was a problem hiding this comment.
일반적인 Error보다는 IllegalArgumentException 와 같은 에러를 사용하는게 좋을 것 같아요
| } | ||
|
|
||
| public List<Integer> getValue() { | ||
| return lotto; |
There was a problem hiding this comment.
수정하지 못하게 불변 리스트로 반환하는 건 어떤가용?
| return lotto; | |
| return Collections.unmodifiableList(lotto) | |
| @@ -0,0 +1,18 @@ | |||
| package model; | |||
|
|
|||
| public enum LottoConstants { | |||
There was a problem hiding this comment.
제 생각엔 단순 상수를 enum으로 관리하는 건 약간 비효율적인게 아닐까? 라는 생각이 들어요. 그냥 정적 변수로 선언해도 괜찮을 것 같아요!
| private LottoList manualLottoList; | ||
| private LottoList lottoList; | ||
| private WinningLottoInfo winningLottoInfo; | ||
|
|
There was a problem hiding this comment.
컨트롤러에 DI 와 관련된 부분이 없으니 뭔가 어색해보이네요ㅋㅋ 따로 주입하지 않은 이유가 있나요?
| } | ||
|
|
||
| public static void printLottoPrizeResult(LottoPrizeResult lottoPrizeResult) { | ||
| System.out.println("당첨 통계\n---------"); |
There was a problem hiding this comment.
이넘을 수정한다면 아래 부분도 훨씬 간단하게 출력 할 수있을 것 같아보여요
안녕하세요 리뷰어님 👋🏻 로또 미션을 제출합니다. 잘 부탁드립니다!
기능 자체는 많이 들어있지 않은 것 같은데 엄밀하게 코드를 짜니 어마무시하게 거대한 코드가 되어버리는 것 같군요
이번 미션에서 신경써본 점 ✅
for문의 사용을 줄이고,stream,forEach등을 사용해 코드의 의도를 나타내었습니다.stream이 장난아니게 복잡하더군요.일급 컬렉션 사용 소감
상당히 좋았어요!
미션 진행 전 구글링을 하면서 클래스들을 개발자분들이 어떻게 다루나 보았는데 신기하게도 도메인 로직을 담당하는 클래스 내부 자체에 검증 로직을 많이들 두었더라고요. 그 때는 그런갑다 하고 넘어갔는데 이렇게 직접 검증 로직들을 그 클래스에 넣고 관리해주니 코드의 신뢰도가 많이 올라갔다는 것을 느꼈습니다.
결론적으로 데이터를 관리하기에 안전한 방식처럼 느껴졌어요. 특히 여러 클래스들을 조합하여 새로운 클래스를 만들거나 복잡하게 데이터가 오갈 때 이 장점이 부각될 것 같아요.
enum 사용 소감
음... 솔직히 아직까지는 크게 장점이 와닿지는 않았던 것 같아요. 상수들만을 관리하는 클래스를 만들어서 쓸 수도 있었을 것 같고, 심지어 그게 더 간단한 문법이었을 수도 있을 것 같아요. enum을 어떨 때 쓰면 좋을지, 상수들을 관리하는 클래스를 만들었을 때와는 무슨 차이가 있는지 좀 더 알아봐야겠네요.
~~~.getValue()남발하다 보니 너무 길어지더라고요. 물론 의미를 나타낸다면 충분히 이름은 길어질 수 있지만 이건 좀 다른 느낌인 것 같아요.getValue()를 뺀다고 의미가 퇴색되는 것도 아닐테고요.이번 미션에서는 크게 헤맨 점은 없지만, 번거로운 점은 좀 많았던 것 같아요. 자바스크립트에서는 쉽게 했던 배열 합치기, 배열 그대로 대입하기, 맵핑하기가 자바에서는 상당히 불편하게 느껴졌어요. 아마 코드 곳곳에 복잡한 부분이 있을 거에요. 자바스크립트의 문법이 그리워지네요. 😢
아래는 실행시켜 본 화면이에요.
그럼, 잘 부탁드리겠습니다!