Lotto step3 by Highjune · Pull Request #3173 · next-step/java-lotto

Highjune · 2023-05-11T04:03:39Z

지난 step2 PR 에 궁금한 점 댓글로 남겼는데 확인해주시면 감사하겠습니다.
step3 리뷰도 자유롭게 해주시면 감사하겠습니다!

- 디미터 법칙수렴. Record 수익률 계산하는 함수 인스턴스 메서드로 변경 - map.merge() 활용

- 보너스볼 단일 멤버변수 객체 생성 - 테스트 코드 작성

- Rank의 변수에 isBonusBall(boolean) 추가 - LottoCompany에서 WinNumber 만들 때 보너스 숫자도 받음 - Rank, Record 테스트 추가

catsbi

강준님 3단계는 2단계에서 고민을 많이하셔서 그런지 크게 문제될 부분도 안보이고 좋네요 👍
몇 가지 피드백 드린게 있으니 참고하시고 다시 리뷰요청 부탁드려요

catsbi · 2023-05-11T15:01:25Z

        int count = 0;
        for (LottoNumber lottoNumber : lottoTicket) {
-            count += target.increment(lottoNumber);
+            count += winNumber.countIfHave(lottoNumber);


Stream API를 써도 좋을 것 같네요 👍

변경했습니다.
그런데, 종종 향상된 for문에서 기존 변수에 값을 추가적으로 계속 더할때는 기본값을 못 쓰고 Atomic 값을 써야 하잖아요?
그래서 기본값을 사용하기 위해서 다시 for문으로 변경하는 경우도 있었는데요. 어느 것이 맞을까요?

질문을 잘 이해하지 못했습니다.
향상된 for문에서 지역변수 혹은 인스턴스 변수에 값을 연산할 때 기본값이 아닌 Atomic을 써야하는 경우가 언제죠?

@catsbi
아 아래와 같은 상황입니다. 1. 번과 같이 쓰면 컴파일 에러가 발생해서 2번과 같이 쓰게 되더라구요. 1번의 에러는 Variable used in lambda expression should be final or effectively final 입니다.

public int match(Lotto winNumber) { int count = 0; if (winNumber == null) { return count; } lottoTicket.stream() .forEach(lottoNumber -> count += winNumber.countIfHave(lottoNumber)); // 컴파일 에러. return count; }

public int match(Lotto winNumber) { AtomicInteger count = new AtomicInteger(0); if (winNumber == null) { return count.intValue(); } lottoTicket.stream() .forEach(lottoNumber -> count.addAndGet(winNumber.countIfHave(lottoNumber))); return count.intValue(); }

@Highjune
아 이런 컴파일 에러가 발생하는건 Stream API안에서는 지역변수(local value)에 영향을 주면 안됩니다.
local value는 jdk 과거버전에서는 무조건 final을 붙혀야 했고, 시간이 지나 지금은 final을 붙히지 않아도 되지만, 사실 final로 취급하기에 effective final value로 취급됩니다. 그래서 외부 변수에 값을 변경할 수 없는 것이죠.
AtomicInterger로 해서 에러가 발생안하는건 AtomicInterger 가 참조하고있는 주소값을 변경하는게 아니기 때문인데, 이 역시 외부 변수에 변조를 의도하기에 사용하지 않는게 좋습니다.

Stream API에서 사용되는(그렇지 않더라도) Lambda는 순수함수(pure function)이어야 해요. 즉 외부값에 영향을 주지 않아야 한다는것이죠. 그렇기때문에 forEach보다는 제가 따로 코멘트 드렸던 내용을 권장드렸던 것입니다.

catsbi · 2023-05-11T15:10:09Z

+    public boolean haveBonus(LottoNumber bonusNumber) {
+        return lottoTicket.contains(bonusNumber);
+    }
+    private boolean isHaveWinNumber(LottoNumber winLottoNumber) {
+        return lottoTicket.contains(winLottoNumber);


두 메서드의 내부는 동일한데 어떤 차이가 있는걸까요?
클린 아키텍처에서는 중복도 우연일 수 있다고 억지로 합치기보단 중복된 코드도 분리해도 괜찮다는 말이 있습니다.
그런 부분을 고려해서 이렇게 설계하신걸까요?

앗 아닙니다. haveNumber() 함수 하나로 통일했습니다.
그런데 위 함수 중 하나는 외부호출, 하나는 내부호출인데
public 으로 열어둔 haveNumber() 함수로 통일하는 것은 큰 문제가 없겠지요?

네 외부에 노출했지만 내부적으로 어떤 로직을 수행하는지까지 공개하는것이 아닌 how가 아니라what을 공유하는 것이니까요 ㅎㅎ

catsbi · 2023-05-11T15:11:23Z

+        isNumeric(number);
+        this.number = Integer.parseInt(number);


isNumeric과 Integer.parseInt에서 중복되는 Integer.ParseInt 메서드 호출이 이뤄지고 있네요. 어떻게 줄일까요?

@catsbi
위에서 17라인을 지우고 그냥 isNumeric 함수를 아래와 같이 변경했습니다. 어떤가요?

private int convertToNumber(String number) { int result = 0; try { result = Integer.parseInt(number); } catch (NumberFormatException e) { throw new NumberFormatException("로또 번호는 숫자형태만 가능합니다. " + number + "는 숫자 형태가 아닙니다."); } return result; }

아 그리고 추가적으로 궁금한 점이 있습니다. 기본값을 선언 & 초기화할경우에 int result; 가 낫나요 아니면 int result = 0; 가 나을까요? 원시값은 가장 기본적인 값들로 초기화되는데 그대로 두는 것이 나을까요 아니면 가독성을 위해서 0으로 딱 찍어서 선언해주는 것이 나을까요?

인스턴스 변수는 원시값의 경우 자동 초기화되는게 이미 정해져있기에 작성하지 않는다고 가독성이 낮아지진 않습니다.
필드 초기화를 해주는 경우는

Collection API를 사용해서 NPE 위험을 줄이기 위해서

기본값이 아닌 값을 모든 인스턴스 생성시 기본 초기값으로 지정해주기 위해서
입니다. ㅎㅎ

catsbi · 2023-05-11T15:12:36Z

+    public static Rank find(int matchingCount, boolean isBonusBall) {
        return Arrays.stream(Rank.values())
-                .filter(rank -> rank.matchingCount == matchingCount)
+                .filter(rank -> rank.matchingCount == matchingCount


어떤 타입의 경우에는 두 값이 equals가 아니라 다른 조건일 가능성도 있는데 어떻게 변경해야 할까요?

@catsbi
흠 이해를 못했습니다. 현재로서는 모든 타입에 대해서 다 찾을 수 있는 상황이라고 생각되는데,
혹시 다른 타입이 추가되었을 경우 확장편한 코드가 아니라는 말씀이실까요?

아 이해를 못했었지만, 잘못된 로직을 수정하면서 살짝 이해했어요. stream 안에만 모든 로직을 다 넣다보니 조건 변경(추가)에 대한 유연성이 떨어졌다는 말이 맞을까요?
그래서 아래와 같이 변경했습니다. 어떤가요?

public static Rank find(int matchingCount, boolean isBonusBall) { countValid(matchingCount); if (SECOND.isMatchCount(matchingCount)) { return rankSecondAndThird(isBonusBall); } return Arrays.stream(Rank.values()) .filter(rank -> rank.isMatchCount(matchingCount)) .findFirst() .orElse(Rank.MISS); } private static Rank rankSecondAndThird(boolean isBonusBall) { if (isBonusBall) { return SECOND; } return THIRD; } private boolean isMatchCount(int matchingCount) { return this.matchingCount == matchingCount; }

평가의 기준을 상위 객체(Rank)가 아닌 하위 객체(Rank.THIRD, SECOND, FIRST, ...)으로 잡으라는 의미였습니다. ㅎㅎ
예시로 주신 코드는 평가의 기준이 Rank죠.

열거타입은 Class랑 다를게 없습니다.
Rank는 inerface이고, FIRST, SECOND, THRID는 하위 인스턴스 구현체이죠.
그렇기에 평가식이 Rank에 있다는건 상위 인터페이스에서 하위 구현체에 대한 의존성을 갖는다는 문제가 생깁니다.
그에대한 리팩토링을 제시하고 싶었습니다 ㅎㅎ

catsbi · 2023-05-11T15:13:52Z

+            boolean haveBonus = winNumber.haveBonus(lotto);
+            putRankMap(matchingCount, rankMap, haveBonus);


이전 PR에서 제가 코멘트 답변으로 남겨드렸으니 참고해보셔도 좋을 것 같습니다.

@catsbi
이 부분은 변경중인데 아직 잘 안되네요^^; 계속 하는 중인데 너무 시간이 오래 걸릴 것 같아
이것만 제외하고 푸시했습니다.

이 부분만 추후 작업에 변경해서 해도 될까요?

catsbi · 2023-05-11T15:14:45Z

+        for (Rank rank : this.rankMap.keySet()) {
            int prize = rank.getPrize();
-            int matchingCount = record.getRecord().get(rank);
+            int matchingCount = this.rankMap.get(rank);


key, value가 다 필요하다면 entrySet을 써도 됐을 것 같네요

catsbi · 2023-05-11T15:15:08Z

    }

-    public static ProfitRate calculateProfit(int purchaseMoney, Record record) {
+    public ProfitRate calculateProfit(int purchaseMoney) {


이런 이율계산은 세법이나 여러 조건에 따라 변경되기 쉽죠. 그럼 이 역시 외부주입을 통해 전략패턴으로 구현해볼 수 있곘네요

- for 문 -> Stream 으로 변경 - 중복함수 통일화 - Integer.parseInt() 중복 제거 - map 의 key, value 둘 다 사용하려면 entrySet 사용하기 - 전략패턴 사용

- 이전 피드백 반영 - 가변변수는 1개라도 사용될시 배열을 생성하므로 항상 사용에 유의

Highjune · 2023-05-15T12:48:08Z

아 추가적으로 사소한 것이 궁금한데요.

자바 변수들 선언하는 순서 있잖아요(상수 -> 필드 -> 생성자 -> 정적메서드 -> 메서드). 그럼 같은 레벨 안에서 private, public 의 순서는 어떻게 되나요? 보통 private을 먼저 위에 선언하는 것이 맞을까요?
하나의 테스트 안에 선언문은 1개만 있어야 하는 것이 맞는거죠? 재성님 업로드하신 강의(예전 기수인듯) 보면 아래처럼 2개를 같이 쓰더라구요. 사실 분리하는 것이 맞을까요 아니면 아주 간단하면 굳이 분리하지 않고 아래처럼 사용하는 것(오히려 가독성 좋아 보이는듯도 하네요)이 맞을까요?

assertThat(Rank.rank(6, true)).isEqualTo(Rank.First);
assertThat(Rank.rank(6, false)).isEqualTo(Rank.First);

이 전에 한솔님과 나눴던 PR의 대화를 다시 보기 위해서는 링크 접근 말고 가장 빠른 접근이 뭘까요?
- 저는 nextstep 깃허브 저장소 -> Pull Request -> Closed -> Author 필터에 highjune 검색 이렇게 찾아 들어가는데 더 빠른 경로가 있을까요?

- RankTest 에서 번호를 하나씩 육안으로 맞춰봐야 하는 번거로움. 무슨 테스트인지 알기 어려워서 분기 - WinNumberTest에서 실제로 숫자를 하나씩 비교하는 메서드 테스트함 - RankTest 에서는 단순히 맞춘 갯수를 명시함으로써 육안으로 알기 쉽게 분기

- 3, 4개의 번호를 맞히고 && 보너스 볼을 맞추더라도 4, 5등이 되어야 하는데 그러지 못했다. - Rank Enum클래스에서 멤버변수의 값으로 고정한 isBonusBall을 삭제함으로 유연함 가짐

- int matchingCount -> MatchCount 클래스로 랩핑

catsbi

강준님 피드백 반영 잘 해주셨습니다 👍
제가 질문주신거에 대해 답변을 좀 더 빨리 드렸어야 했는데, 메일이 많이 쌓이다보니 놓치는 경우가 있네요 ㅠㅠ 그런 경우 별도로 DM주시면 좀 더 빠르게 확인 가능하니 DM 애용해주세요 ㅎㅎ

피드백 몇 가지 남겨드렸으니 확인해주시고 다음 미션에서 같이 반영해보시죠!

catsbi · 2023-05-15T15:10:07Z

+
+import java.util.Map;
+
+public interface CalculableStrategy {


하나의 메서드만 정의될 것 같으면 FunctionalInterface 애노테이션을 명시해서 추가적인 기능 정의를 컴파일 단계에서 막아줄 수 있습니다 👍

catsbi · 2023-05-15T15:14:51Z

        }
-        return count;
+        lottoTicket.stream()
+                .forEach(lottoNumber ->


forEach가 외부 변수에 영향을 주는건 지양해주세요.
forEach와 peek 중간 연산자는 디버깅 및 출력 목적을 제외하고 사용하는건 사용목적과는 어긋나거든요.
Stream API를 좀 더 활용해보면 어떨까요?

return lottoTicket.stream() .mapToInt(winNumber::countIfHave) .sum();

catsbi · 2023-05-15T15:16:39Z

+        return count.intValue();
    }

    private int countIfHave(LottoNumber lottoNumber) {


여기서 굳이 1~0을 할 필요도 없습니다 ㅎㅎ
제가 위에서 sum()을 제안했는데, 다음과 같이 구현한다면 해당 메서드가 필요 없어지겠죠.

return (int)lottoTicket.stream() .filter(winNumber::haveNumber) .count();

@catsbi 한솔님, 반영은 했습니다
그런데 1~0은 무엇을 말씀하시는 걸까요?

1이나 0을 반환해줄 필요가 없다는 것이였습니다. 0혹은 1씩 더할수도 1 혹은 2씩 더할수도 있는 것이고, 해당 메서드는 말 그대로 값이 있는지 여부만 판단해주면 충분할 것 같다는 의미였어요 ㅎㅎ

catsbi · 2023-05-15T15:18:38Z

+    private static Rank rankSecondAndThird(boolean isBonusBall) {
+        if (isBonusBall) {
+            return SECOND;
+        }
+        return THIRD;
+    }
+
+    private boolean isMatchCount(MatchCount matchCount) {
+        return this.matchingCount == matchCount.value();
+    }


제가 코멘트 답변드린거 참고해주시면 좋을 것 같아요!

catsbi · 2023-05-15T15:23:00Z

추가적으로 자바 변수의 등장순서는 다음과 같습니다.
a. public 멤버 변수
b. default 멤버 변수
c. protected 멤버 변수
d. private 멤버 변수
e. 생성자
f. public 메서드
g. default 메서드
h. protected 메서드
i. private 메서드

catsbi · 2023-05-15T15:24:31Z

그리고 테스트를 분리하는건 검증문 단위가 아니라 말 그대로 단위에요 ㅎㅎ
a라는 메서드를 테스트할 때 조건이 같다면(1이냐 2냐가 아니라 유효하고 동일한 결과가 유추) 하나로 묶어도 무관합니다.

그리고 이전 PR대화를 빠르게 보시려면 즐겨찾기하는게 제일 빠르겠죠 ㅎㅎ 그게 아니라면 저도 강준님과 동일하게 접근합니다 ㅎㅎ

Highjune added 5 commits May 10, 2023 12:56

feat(Lotto, LottoCompany 등): step2 피드백 반영 중

5354893

refactor(Record): lotto-step2 피드백

1bdf77e

- 디미터 법칙수렴. Record 수익률 계산하는 함수 인스턴스 메서드로 변경 - map.merge() 활용

docs(README.md): 기능 요구사항 작성

351fcaa

feat(BonusWinNumber): 보너스 볼의 범위

7589731

- 보너스볼 단일 멤버변수 객체 생성 - 테스트 코드 작성

feat(2등뽑기와 연괸된 것들): 2등 뽑기

02295b8

- Rank의 변수에 isBonusBall(boolean) 추가 - LottoCompany에서 WinNumber 만들 때 보너스 숫자도 받음 - Rank, Record 테스트 추가

catsbi requested changes May 11, 2023

View reviewed changes

Highjune added 3 commits May 14, 2023 20:47

refactor(전체): step3 피드백

852c0b7

- for 문 -> Stream 으로 변경 - 중복함수 통일화 - Integer.parseInt() 중복 제거 - map 의 key, value 둘 다 사용하려면 entrySet 사용하기 - 전략패턴 사용

refactor(Lotto 생성자): 가변변수 삭제

13dfed6

- 이전 피드백 반영 - 가변변수는 1개라도 사용될시 배열을 생성하므로 항상 사용에 유의

refactor(Lotto): 매직넘버 상수로 뺌

06b42f8

Highjune added 5 commits May 15, 2023 22:22

refactor(RankTest): 쓸데없는 코드 정리

a3aad06

refactor(Rank): 순위 결정 로직 오류 개선

5f2fec5

- 3, 4개의 번호를 맞히고 && 보너스 볼을 맞추더라도 4, 5등이 되어야 하는데 그러지 못했다. - Rank Enum클래스에서 멤버변수의 값으로 고정한 isBonusBall을 삭제함으로 유연함 가짐

refactor(Rank, RankTest): 순위 결정하는 로직 리팩토링

428abe9

refactor(MatchCount): 객체로 래핑

0367eb4

- int matchingCount -> MatchCount 클래스로 랩핑

catsbi approved these changes May 15, 2023

View reviewed changes

catsbi merged commit eec6570 into next-step:highjune May 15, 2023

Highjune mentioned this pull request May 17, 2023

Lotto step4 #3229

Merged

		boolean haveBonus = winNumber.haveBonus(lotto);
		putRankMap(matchingCount, rankMap, haveBonus);

Conversation

Highjune commented May 11, 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!

Highjune May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Highjune May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Highjune May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Highjune May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Highjune commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

catsbi commented May 15, 2023

Uh oh!

catsbi commented May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Highjune May 16, 2023 •

edited

Loading

Highjune May 13, 2023 •

edited

Loading

Highjune May 13, 2023 •

edited

Loading

Highjune May 15, 2023 •

edited

Loading

Highjune commented May 15, 2023 •

edited

Loading