Lotto step2 by Highjune · Pull Request #3115 · next-step/java-lotto

Highjune · 2023-05-06T08:49:30Z

질문할 부분

문자열 계산기 피드백
- StringCalculator 클래스에 isNumeric 메서드에서 try catch 문으로했는데 잘한 것이 맞을까요? 만약 맞다면 그냥 Double.ParseDouble 자체에서 NumberFormatException 발생되는 것이랑은 어떤 차이가 있을까요? 메시지를 담을 수 있는 부분일까요?
- 자료형 선언시, Wrapper class 로 사용하는 건 어떤 경우인가요? 제네릭 이외에, 웹 개발에서 RequestDto 처럼 null 이 들어올 수 있는 경우는 Wrapper 클래스로 감쌌거든요. 다른 경우는 어떤 것들이 있을까요?
로또
- Store 클래스에서 밖에서 사용하는 함수 하나(order()) 를 static 으로 만드니, 그와 연관된 함수들(private까지)을 다 static 으로 만들게 되던데 이렇게 하는 것이 맞을까요? 아니면 static 으로 늘어나는 것이 부담되면 밖에서 호출하는 함수도 인스턴스 메서드로 변경하는 것이 맞을까요?, OutputView 같은 함수도 마찬가지입니다.
- 가독성 문제. LottoApplicationMain 클래스에서 아래와 같이 작성했는데요.(한 라인 길이 무관)
```
WinningNumber winningNumber = new WinningNumber(Store.pickWinNumber(InputView.questionWinnerNumber()));
```
그런데 그렇게 말고 아래와 같이 나눠서 하는 것이 가독성 측면에서 더 나을까요?
```
String answer = InputView.questionWinnerNumber();
Lotto lotto = Store.pickWinNumber(answer);
WinningNumber winningNumber = new WinningNumber(lotto);
```
- Record enum에서 2개 이하로 맞힌 경우는 ETC 같은 타입으로 모는 것이 좋을까요? 특정 값들을 정의할 때는, 그 값들만 갖고 있는 것이 맞는지 그 예외에 해당하는 것도 갖고 있는 것이 더 맞을지 궁금합니다.

- 피연산자 각각에 대한 숫자 포맷인지 확인하는 로직 넣음 - 자동 생성되는 코드들도 if 문 한줄짜리일 경우 {} 블럭 생략하지 않기 - Wrapper 클래스 남발하지 않기

- 로또번호 - 로또번호 구매 - 출력 - 당첨결과

- 상위에 stringcalculator, lotto 패키지로 세분화

- 중복되지 않은 숫자 로또 테스트하기

- 로또 번호가 1 ~ 45 사이의 숫자가 아닐 경우 예외 발생

- 돈이 천원 단위가 아니면 에러가 발생한다

- List<Integer>을 List<LottoNumbers>로 변경 - 변경함으로써 발생되는 코드 리팩토링

- List<Lotto> -> LottoBundle 로 감싸기

- Main함수, Ui(Input, Output) 생성 - Output에서 로또번호 출력하기 함수 생성

- 로또 1장 중에서 당첨번호가 몇 개 있는지 계산할 수 있다

- 당첨번호 받아서 저장하기 - WinningNumber -> WinNumber 클래스명 변경 - Store 클래스 안에서 당첨번호 받아서 저장하는 로직 추가

- OutputView에서 정렬해서 내보내기

- Record(Enum) 정의 - 순위에 따른 테스트

- 2개 이하를 맞힐 경우 예외 발생

- Rank(Enum) 추가<-- Record에서 변경 - Record에서 Map으로 기록 갖고 있는 것 기록

catsbi

강준님 로또 2단계 미션 잘 구현해주셨어요. 객체를 분리하면서 어떤 고민을 했는지 잘 봤습니다 👍

코멘트에도 써놨지만, 전체적으로 구조가 트랜잭션 스크립트 패턴을 채용해서 쓰고 있기에 테스트하기 힘들어 보이고 객체지향 패러다임인 SOLID를 지키기 힘든 코드들이 많이 보였습니다.
이에 대해 고민을 해보면 좋을 것 같아요.

질문 주신 내용에 대해서는 코멘트에 남기도록 하겠습니다. 확인 후 리뷰요청 해주세요!

catsbi · 2023-05-07T09:23:29Z

+    - [x] 중복되지 않은 숫자 6개여야 한다.
+    - [x] 1 ~ 45 중의 숫자이어야 한다.


여기에 추가로 중복된 값을 전달하면 예외를 던진다, 1이하이거나 45를 벗어난 숫자는 예외를 던진다. 등의 요구사항도 추가되면 테스트 작성이 더 쉬워지겠죠 ㅎㅎ

@catsbi
넵, 수정 & 추가했습니다.
그런데 기능 요구사항은 테스트 단위로 1:1 대응되는 것이 맞을까요? 아니면 N개의 테스트를 포함하고 있는, 말 그대로 기능의 요구사항이 맞을까요?

테스트 1:1로 매칭될 수 있도록 세부기능사항으로 나누는 것이 좋을까요?

@Highjune 이상하게 코멘트에대한 메일링 알림이 제시간에 잘 안오고 랜덤으로 발송되는 느낌이네요;; 이제봤습니다 ㅠㅠ
README.md의 명세는 보통 개발자를 대상으로하는 명세로쓰이기에 테스트와 1:1으로 수렴할수록 좋습니다.
아니면 depth를 나눠서 기능과 내부적인 에지케이스를 나눠서 명세로 작성하는 방법도 있겠죠!

catsbi · 2023-05-07T09:25:35Z

+        int purchaseMoney = InputView.questionOrder();
+        LottoBundle lottoBundle = Store.order(new Money(purchaseMoney));
+        OutputView.showLottoBundle(lottoBundle);
+        WinNumber winNumber = new WinNumber(Store.pickWinNumber(InputView.questionWinnerNumber()));
+        Record record = Store.extractRecord(lottoBundle, winNumber);
+        OutputView.showRecord(record);
+        ProfitRate profitRate = Store.calculateProfit(purchaseMoney, record);
+        OutputView.showProfitRate(profitRate);


개념적 유사성에 따라 개행을 통해 구분을 지으면 어떨까요? 코드가 다 붙어있다보면 가독성이 낮아질 수 있습니다.

catsbi · 2023-05-07T09:26:22Z

+    }
+
+    private void checkDuplicate(List<LottoNumber> lotto) {
+        Set<LottoNumber> setLottoNumbers = new HashSet<>(lotto);


lotto타입이 Set이면 유효성 검증을 위해 별도로 Set으로 생성해서 체크할 필요가 없지 않을까요?

catsbi · 2023-05-07T09:26:45Z

+
+public class Lotto {
+
+    private final List<LottoNumber> lotto;


클래스명과 변수명을 다르게 작성해주시는게 좋습니다.

getLotto()가 클래스를 말하는건지 자료구조를 말하는건지 구분짓기 힘들죠 ㅎ

catsbi · 2023-05-07T09:27:46Z

+    private static final int LOTTO_MAXIMUM_VALUE = 45;
+    private static final int LOTTO_MINIMUM_VALUE = 1;


일반적으로는 상수가 객체 최상단에 위치하는게 관례이긴 합니다 ㅎ

catsbi · 2023-05-07T09:40:44Z

+public class LottoNumberTest {
+
+    @Test
+    @DisplayName("로또번호는 1 ~ 45이어야 한다.")


1 ~ 45가 아닌 경우 어떤 동작을 할지에 대해서는 테스트가 필요 없을까요?

catsbi · 2023-05-07T09:41:02Z

+    @Test
+    public void dd() {
+        String a = "2 ";
+        System.out.println(a.trim());
+        System.out.println(a.trim().length());
+    }


불필요한 코드는 정리해주세요

catsbi · 2023-05-07T09:41:19Z

+
+public class LottoTest {
+
+    @DisplayName("로또 숫자가 중복이 되면 예외가 발생한다.")


예외는 발생한다기보단 던진다(throw)가 적절한 표현입니다.

catsbi · 2023-05-07T09:41:53Z

+
+    @BeforeEach
+    public void setUp() {
+        List<LottoNumber> lottoNumberList = Arrays.asList(new LottoNumber(1)        // todo) new LottoNumber() 로 테스트 데이터 만드는 것 수정하기


todo는 PR전 모두 제거해주시는게 좋습니다.

catsbi · 2023-05-07T09:42:14Z

+    @DisplayName("1등을 가릴 수 있다.")
+    @Test
+    public void rank_DependsOnCount_ChooseFirst() {
+        List<LottoNumber> lottoNumberList = Arrays.asList(new LottoNumber(1)


중복된 given절이 보이는데 어떻게 줄이면 좋을까요?

catsbi · 2023-05-07T09:56:32Z

질문주신 내용에 대해 답변드리자면,

잘했다 못했다 평가는 기획서의 내용에 따라 달라지겠죠. 자바에서 try-catch가 딱히 문제되는 코드는 아닙니다. 다만 NumberformatException자체가 어떤 값을 변환하려 시도할 때 예외가 던져져는지에 대해 메세지를 다 제공할꺼라서 그 외에 추가적으로 넣어야 하는 메세지가 있거나, 해당 예외가 발생했을 때 기본 값으로 세팅해서 예외를 안던진다는 기획이 있거나 할 때는 적절하겠죠. 그게 아니라면 굳이 try-catch를 안하고 그냥 예외가 던져지면 던져지는대로 사후 처리를 하는게 맞구요.
말씀하신대로 제네릭사용이 대표적이고 컬렉션 프레임워크에서도 타입지정을 위해 사용됩니다. 그외에는 크게 필요한 경우는 못 본것 같습니다. null값이 필요한 경우 사용될 수 있긴한데 거의 못봤고, Number등으로 상위타입으로 추상화를 통해 다른 객체들관의 호환성을 높힐수도 있긴한데 이 역시 거의 사용되지 않죠.
static이여도 상관없는 재사용성을 가지고 별도의 상태를 가지지 않는 메서드라면 상관 없습니다. 인스턴스 메서드로 만들되 싱글턴으로 관리할지, 아예 유틸리티 클래스로 갈지는 크게 차이가 없습니다.
한 라인에 작성하는게 더 가독성도 떨어질 뿐더러 디버깅도 어렵게 만들죠. 적절한 개행을 통해 구분짓는게 좋습니다. 매개변수 작성칸에 다른 메서드를 호출해 반환값을 그대로 넣어준다는 것의 장점은 코드라인이 줄어든다는 것을 제외하고는 없습니다. 메서드 시그니처가 너무 명확해서 간혹가다 쓰는건 그럴 수 있지만 그 외에는 추천드리지 않습니다.
ETC나 MISS타입등으로 분류해도 좋을 것 같네요 일종의 default 개념이죠 ㅎㅎ 위에서도 말했지만 기획의도에 따라 달라질 수 있는 부분인것 같네요. 중요한건 예외는 정말 예외 상황에서만 던져야 한다는 것입니다.

- 기능 요구사항 테스트 작성에 용이하도록 변경 - 개념적 유사성에 따라 개행을 통해 구분 - 클래스명과 변수명은 다르게 짓기. getLotto가 클래스를 말하는건지 자료구조를 말하는건지 구분짓기 힘들다 - List<LottoNumber> -> Set<LottoNumber> 로 변경함으로써 유효성 체크 로직 삭제 - 상수 제일 위에 위치시키기(계속 깜빡함) - 예외 메시지 넘길 때 클라이언트가 이해할 수 있도록 정보 넘기 - 매직 넘버 상수로 분리하기 - 곳곳에 산발된 상수(1000) 한 곳으로 통일 - , 대신에 _로 높은 숫자 가독성 높이기 - Rank 예외 타입(꽝 = MISS) 추가 - 트랜잭션 스크립트 모델 -> 도메인 모델로 변경하기 - getter로 private 한 내부값 꺼내서 조작하지 말고 객체한테 던지기 - 테스트 추가 - 불필요한 코드 삭제(todo, 비즈니스 로직 무관한 개인 확인 테스트) - 중복된 코드(given) 삭제

Highjune · 2023-05-08T16:27:21Z

한솔님 리뷰 너무너무 좋아요!! 감사합니다.
피드백 최대한 반영해서 적용해봤습니다. 한번 피드백 해주실때마다 엄청 느는 것 같아요.

확인해주시면 감사하겠습니다!

- of 함수로 중복코드 삭제

catsbi

강준님 피드백 반영 잘 해주셨습니다.
체크리스트도 한층 디테일해졌네요. 👍

몇 가지 피드백이 필요한 부분이 있어 코멘트 남겨드렸는데 확인 해보시고 다음 미션에서 같이 반영해보는걸로 하죠. 이만 merge 하겠습니다!

catsbi · 2023-05-09T05:49:41Z

+    public static Lotto of(Integer... numbers) {
+        Set<LottoNumber> lottoTicket = new HashSet<>();
+        for (Integer number : numbers) {
+            lottoTicket.add(new LottoNumber(number));
+        }
+        return new Lotto(lottoTicket);
+    }


Wrapper 타입을 사용하는것은 꼭 필요한 경우(제네릭, Colleciton API 등)가 아니라면 지양해주세요 장점보단 단점이 많습니다.

가변인수는 쓰기 전 한 번 더 고민해볼 필요가 있습니다. 변수가 하나만 전달되더라도 내부적으로 배열을 생성하는 작업이 필요하죠. 그래서 오버로딩을 통해 구현하거나 하는식으로 대체됩니다.

catsbi · 2023-05-09T05:50:45Z

+    public int match(Lotto target) {
+        int count = 0;
+        for (LottoNumber lottoNumber : lottoTicket) {
+            count += target.increment(lottoNumber);


메서드명을 보면 증가인데,, 반환값과 연관성이 떨어져보여 가독성이 낮아 보입니다. 좀 더 적절한 이름은 없을까요?
그리고, target은 NPE 위험이 있어보이네요 lottoNumber.increment(target)이 더 적절하지 않을까 싶네요

메서명은 수정했습니다. 그리고 위 코드에서는 보이지 않지만, 파라미터로 넘어오는 target은 WinNumber 클래스 안에서 본인의 인스턴스를 직접 넘기는 것이므로 NPE 의 위험성은 없어보입니다.

감사합니다!

앗 협업관점에서 해당 메서드는 public API이기 때문에 WinNumber가 아닌 다른 클라이언트에서 사용할 수 있지 않을까요?

@catsbi
�아하, public 그 자체만으로 모든 경우를 다 생각해야 하는군요. 반영하겠습니다.
감사합니다.

catsbi · 2023-05-09T05:55:33Z

+
+public class LottoCompany {
+
+    public static final int PURCHASE_UNIT = 1000;


Lotto의 가격은 Lotto가 가지고 있는게 어떨까 싶네요 ㅎㅎ 상위 단체인 회사에서 가격을 변경할 수 있지만, 그건
상위 객체에서 로또 객체에게 가격을 바꾸라고 요청하는거지 가격정보를 상위 단체에서 가지고 있는건 적절한 것 같지는 않습니다

catsbi · 2023-05-09T05:56:58Z

+    private static void isNumeric(String number) {
+        try {
+            Integer.parseInt(number);
+        } catch (NumberFormatException e) {
+            throw new NumberFormatException("로또 번호는 숫자형태만 가능합니다.");
+        }
+    }


LottoNumber라는 객체가 있는데 그 외의 객체에서 LottoNumber의 유효성 검증이 이뤄지고 있는데, 적절한 책임 분리일까요?

catsbi · 2023-05-09T05:58:01Z

    }
+
+    public static Record extractRecord(LottoBundle lottoBundle, WinNumber winNumber) {
+        Map<Rank, Integer> rankMap = new HashMap<>();


열거 타입을 사용하는 경우 EnumMap을 사용해보면 어떨까요?

catsbi · 2023-05-09T05:59:14Z

+        for (Lotto lotto : lottoList) {
+            int matchingCount = winNumber.distinguish(lotto);
+            putRankMap(matchingCount, rankMap);
+        }


Stream API의 collect부분에서groupingBy를 사용하면 좀 더 간결하게 Map으로 그룹핑이 가능할 것 같네요!

@catsbi
한솔님 코드를 조금 정리해보려고 했는데 잘 안되네요.
우선은 커밋 & 푸시하는대로 했고, 다른 측면에서 아래처럼 시도는 해봤는데요

public static Record extractRecord(LottoBundle lottoBundle, WinNumber winNumber) { List<Lotto> lottoList = lottoBundle.unfoldLottoBundle(); Map<Rank, Long> rankMap = lottoList.stream() .collect(Collectors.groupingBy(lotto -> makeRank(winNumber.distinguish(lotto)) , Collectors.counting())); return new Record(rankMap); // 컴파일 에러 } private static Rank makeRank(int matchingCount) { return Rank.find(matchingCount); } `` - 우선 생성자 타입에서 컴파일 에러가 발생하는데, 2가지 측면인 것 같습니다. 생성자 파라미터의 타입 자체가 맞지 않고, 두번째는 Map 안의 두번째 타입인 Long 타입을 Integer 로 변환해야 하는 영향을 받는 것 같습니다. - 그래서 생성자 파라미터 타입을 EnumMap -> Map 으로 변경하게 되면 EnumMap을 사용하지 못하고, 또한 Map<Rank, Integer> rankMap 멤버변수 정의자체를 Map<Rank, Long> 으로 변경하게 되면 그것도 나름대로 맞는지 의문이 드네요.

counting이 Long타입을 제공하기 때문인 것으로 보이는데 summingInt를 활용해보면 어떨까요?
추가적으로, 이를 EnumMap으로 만들고싶다면 여러 방법이 있는데 가장 간단한 방법은 groupingBy대신 toMap을 사용하되 mergeFunction을 정의하는 것이죠.

.collect(Collectors.toMap(rank -> rank, rank -> 1, Integer::sum, () -> new EnumMap<>(Rank.class)));

이런식으로 동작할수도 있을 것 같네요.

혹은 collectingAndThen과 groupingBy를 혼용해서 쓸 수도 있습니다 ㅎ

List<Lotto> lottoList = lottoBundle.unfoldLottoBundle(); Map<Rank, Integer> rankMap = lottoList.stream() .map(winNumber::distinguish) .collect(Collectors.collectingAndThen(Collectors.groupingBy(...), EnumMap::new));

catsbi · 2023-05-09T06:04:12Z

+
+    public static ProfitRate calculateProfit(int purchaseMoney, Record record) {
+        double allPrize = 0;
+        for (Rank rank : record.getRecord().keySet()) {


디미터 법칙에서는 네 이웃의 이웃을 알게 하지 마라라는 말이 있습니다.
레코드는 내부값의 내부값을 꺼내야 하는 상황인데 이럴 경우 이 로직이 여기에 있는게 적절한가에 대한 고민을 할 필요가 있죠.

해당 메서드가 정적이여야 할 이유가 있을까요?

catsbi · 2023-05-09T06:04:37Z

+    private static void putRankMap(int matchingCount, Map<Rank, Integer> rankMap) {
+        if (matchingCount >= 3) {
+            Rank rank = Rank.find(matchingCount);
+            rankMap.put(rank, rankMap.getOrDefault(rank, 0) + 1);


map에는 merge등의 여러 유틸 메서드가 있어서 이를 활용하면 더 간결하게 작성이 가능해집니다.

Highjune added 22 commits May 5, 2023 15:09

feat(Domain): step1 피드백 반영

37d08fd

- 피연산자 각각에 대한 숫자 포맷인지 확인하는 로직 넣음 - 자동 생성되는 코드들도 if 문 한줄짜리일 경우 {} 블럭 생략하지 않기 - Wrapper 클래스 남발하지 않기

docs(REAMD.md): step2(로또 자동) 기능 요구사항

4af1e7b

- 로또번호 - 로또번호 구매 - 출력 - 당첨결과

refactor(all): 패키지 구조 정리

0fdfabd

- 상위에 stringcalculator, lotto 패키지로 세분화

feat(Lotto): 중복되지 않는 로또 숫자 테스트

7164af6

- 중복되지 않은 숫자 로또 테스트하기

feat(Lotto): 로또 숫자 범위 안 테스트 구현 중

3fb1593

feat(Lotto): 로또 번호의 유효범위

0f75391

- 로또 번호가 1 ~ 45 사이의 숫자가 아닐 경우 예외 발생

refactor(Lotto): isBetweenValid 메서드 리팩토링

bad28bb

feat(Money): 돈의 제약

ec3d40b

- 돈이 천원 단위가 아니면 에러가 발생한다

feat(Store): 로또 구매하기

57505b3

refactor(Store): 로또 금액만큼 주문하기

445c3bd

refactor(LottoNumber): LottoNumber로 감싸기

145647e

- List<Integer>을 List<LottoNumbers>로 변경 - 변경함으로써 발생되는 코드 리팩토링

refactor(LottoBundle): LottoBundle 추가

3d4bdf5

- List<Lotto> -> LottoBundle 로 감싸기

feat(로또번호 출력): 구매한 로또 번호 출력

caf6311

- Main함수, Ui(Input, Output) 생성 - Output에서 로또번호 출력하기 함수 생성

feat(WinningNumber): 당첨번호 맞추기

e4b70d0

- 로또 1장 중에서 당첨번호가 몇 개 있는지 계산할 수 있다

feat(WinNumber, Main): 당첨번호 받아서 저장하기

bdb7573

- 당첨번호 받아서 저장하기 - WinningNumber -> WinNumber 클래스명 변경 - Store 클래스 안에서 당첨번호 받아서 저장하는 로직 추가

feat(OutputView): 로또번호 출력할 때 정렬하기

3e229c6

- OutputView에서 정렬해서 내보내기

feat(Record): enum으로 맞힌 개수, 금액 설정

c9898d1

- Record(Enum) 정의 - 순위에 따른 테스트

test(Record): 상금 범위에 없는 갯수를 맞힌 경우

18efdb0

- 2개 이하를 맞힐 경우 예외 발생

feat(순위, 기록): 순위를 기록한다

dcf0d43

- Rank(Enum) 추가<-- Record에서 변경 - Record에서 Map으로 기록 갖고 있는 것 기록

feat(Record 출력): 로또 당첨결과 출력

c8fa46b

feat(수익률 계산): 수익률 계산하기

7f7b918

docs(READM.md): 체크리스트 업데이트

19bf6dd

catsbi requested changes May 7, 2023

View reviewed changes

refactor(RankTest): 중복코드 삭제

21ee0e5

- of 함수로 중복코드 삭제

catsbi approved these changes May 9, 2023

View reviewed changes

catsbi merged commit 8d92c11 into next-step:highjune May 9, 2023

		- [x] 중복되지 않은 숫자 6개여야 한다.
		- [x] 1 ~ 45 중의 숫자이어야 한다.

		private static final int LOTTO_MAXIMUM_VALUE = 45;
		private static final int LOTTO_MINIMUM_VALUE = 1;


		public class LottoTest {

		@DisplayName("로또 숫자가 중복이 되면 예외가 발생한다.")


		public class LottoCompany {

		public static final int PURCHASE_UNIT = 1000;

Conversation

Highjune commented May 6, 2023

질문할 부분

Uh oh!

catsbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catsbi commented May 7, 2023

Uh oh!

Highjune commented May 8, 2023

Uh oh!

catsbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants