[2단계 - 로또(수동)] 연로그(권시연) 미션 제출합니다. #454

yeon-06 · 2022-03-02T08:23:47Z

안녕하세요 닉~!😊
어제 휴일이었는데 잘 보내셨나요?
1단계에 2단계도 피드백 잘 부탁드립니다!

2단계 추가 요구 사항

수동 로또 구매 기능
- 구매할 수동 로또 개수 입력
- 수동 로또 목록 입력
- 수동 로또 목록 출력

1단계 피드백

String.format 또는 System.out.printf 활용
테스트 코드의 given, when, then 적절하게 분리
그 외 개선 사항은 코멘트로 추가

고민

코멘트로 추가

- array -> map 활용 - 미리 전부 생성해두지 않고 필요할 때마다 생성해서 저장

- 하드 코딩 상수화 - String.format()을 사용해 형식 통일

yeon-06 · 2022-03-02T08:26:13Z

src/main/java/lotto/controller/LottoController.java

@@ -13,7 +16,8 @@
    public void start() {
        LottoAmount amount = inputAmount();

-        LottoTicket lottoTicket = buyTickets(amount);
+        ManualLottoCount manualLottoCount = inputManualLottoCount(amount);


수동 로또 개수를 검증하는 로직을 분리하고 싶어서 ManualLottoCount라는 vo를 생성했습니다
다만 ManualLottoCount에 저장한 값이 계속 필요해서 사용할때마다 getValue()를 호출하게 되네요
이게 적절한 VO의 생성이 맞나?라는 의문이 계속 들어요😂

값을 사용해야 하는 시점에서는 어쩔 수 없죠 ㅎㅎ
그게 아니라면 public 메서드를 열어서 내부에서 처리하게끔 해야하는데, 아래 count 계산 로직의 경우 책임 분리의 관점에서 애매한 부분이 있을수도 있겠네요 :)

yeon-06 · 2022-03-02T08:32:28Z

src/main/java/lotto/domain/LottoNumber.java


-    private static final LottoNumber[] cacheLottoNumber = new LottoNumber[MAX + 1];
+    private static final Map<Integer, LottoNumber> cacheLottoNumber = new HashMap<>();


캐싱된 인스턴스 Map에 저장
👉 현재는 저장되는 값이 숫자지만 특정 문자열 등이 캐싱되어야 하는 경우 Array나 List 등을 통해 값을 꺼내오기에는 의미를 분명히 하는데 한계가 있을 수 있겠다고 생각

yeon-06 · 2022-03-02T08:37:18Z

src/main/java/lotto/domain/LottoNumber.java


 public class LottoNumber {
-    private static final int MIN = 1;
-    private static final int MAX = 45;
+    protected static final int MIN = 1;


LottoNumbers에서도 해당 상수가 필요해 접근 제어자 변경

➕ default와 protected 중 고민했으나 나중에 LottoNumber을 상속하는 클래스가 생기면 해당 클래스의 패키지에서도 사용할 일이 생기지 않을까? 라고 예상했습니다.
하지만 LottoNumber을 상속하는 경우가 생기지 않을 것 같기도 하고...
뭘 사용해야 더 적절한지에 대한 고민이 생겼습니다.
닉은 default와 protected를 사용하는 기준 같은게 따로 있으실까요?

일단 저는 상수 같은 경우는 public으로 열긴 하는데요. (불변 상수기 때문에 접근제어를 fit하게 해봤자 큰 의미 없다고 생각해서)
저도 해당 시점에 판단해서 상속이 필요할 것 같으면 protected, 패키지 내에서만 사용할 것 같으면 default로 둡니다.
사실 자바에서 패키지 기반으로 접근 제어를 한다는 사실이 맘에 들진 않아요 ㅋㅋ (코틀린을 쓰면 그런 고민할 필요가 없습니다 갓틀린 짱짱..)

답은 없으니 각 제어자 목적에 맞게 고민하고 판단하셔서 결정하시면 됩니다 :)

yeon-06 · 2022-03-02T08:38:19Z

src/main/java/lotto/domain/LottoNumbers.java

-        String[] stringArr = input.split(DELIMITER);
-        validateLottoNumbers(stringArr);
-        this.lottoNumbers = convertStringArrToIntegerList(stringArr);
+    public LottoNumbers(List<Integer> numbers) {


최소한의 검증을 거치고 생성자에 파라미터 보내줄 수 있도록 View와 Domain의 로직 분리

yeon-06 · 2022-03-02T08:40:04Z

src/main/java/lotto/domain/LottoTicket.java

@@ -32,4 +32,8 @@ public WinningResult calculateWinningStatistic(WinningNumbers winningNumbers) {
                .collect(collectingAndThen(toList(), Collections::unmodifiableList));
        return new WinningResult(rankings);
    }
+
+    public void buyManualTicket(List<LottoNumbers> manualTickets) {


수동 로또와 자동 로또의 클래스를 분리할까 생각해보다가 불필요하게 느껴져 해당 메소드를 추가했습니다.
setter는 아니지만 내부의 상태를 변하게 하는 메소드임은 분명해서 선언하지 말아야하나 라는 의문이 듭니다😂

조금 더 생각해보았는데 LottoTicket 내부에 선언된 List를 get한 후에 그 값에 add하는 것보다는
현재처럼 메소드를 통해 LottoTicket 내부의 List에 파라미터로 넣은 List로 넣는게 맞다는 생각이 듭니다

DTO와 VO에 대한 생각을 하다보니 DTO처럼 상태 변환이 일어나면 안된다고 생각을 했는데 생각해보니 LottoTicket은 엔티티에 가까운 도메인 모델이지 DTO가 아니네요😂💦

다만 다른 방법도 있지 않을까 라는 생각은 여전히 있습니다
닉이라면 어떤 시도를 해보셨을 것 같나요?

네네 자동로또와 수동로또를 합친다면 저도 위와 같이 했을 거에요.
대신 행위가 buy라는 네이밍과는 맞지 않는 것 같아서, add~ 같은 게 좋지 않을까 싶네요 :)

wbluke

2단계 구현 잘 해주셨네요 👍
간단한 코멘트 몇 가지 남겼으니 참고 부탁드려요 🙂

wbluke · 2022-03-03T12:55:29Z

src/main/java/lotto/controller/LottoController.java

@@ -13,7 +16,8 @@
    public void start() {
        LottoAmount amount = inputAmount();

-        LottoTicket lottoTicket = buyTickets(amount);
+        ManualLottoCount manualLottoCount = inputManualLottoCount(amount);


값을 사용해야 하는 시점에서는 어쩔 수 없죠 ㅎㅎ
그게 아니라면 public 메서드를 열어서 내부에서 처리하게끔 해야하는데, 아래 count 계산 로직의 경우 책임 분리의 관점에서 애매한 부분이 있을수도 있겠네요 :)

wbluke · 2022-03-03T12:56:38Z

src/main/java/lotto/controller/LottoController.java

+        int manualTicketCount = manualLottoCount.getValue();
+        int autoTicketCount = amount.calculateLottoCount() - manualTicketCount;
+
+        LottoTicket lottoTicket = new LottoTicket(autoTicketCount);


LottoTicket을 만드는데 생성자에 단순한 숫자를 넣어주니까 의미 전달이 조금 약한 것 같아요.
(물론 autoTicketCount 라는 변수명으로 어느정도 유추할 수는 있겠지만)
정적 팩토리 메서드를 사용해서 의미를 명확하게 해보면 어떨까요? :)

wbluke · 2022-03-03T13:04:18Z

src/main/java/lotto/domain/LottoNumber.java


 public class LottoNumber {
-    private static final int MIN = 1;
-    private static final int MAX = 45;
+    protected static final int MIN = 1;


일단 저는 상수 같은 경우는 public으로 열긴 하는데요. (불변 상수기 때문에 접근제어를 fit하게 해봤자 큰 의미 없다고 생각해서)
저도 해당 시점에 판단해서 상속이 필요할 것 같으면 protected, 패키지 내에서만 사용할 것 같으면 default로 둡니다.
사실 자바에서 패키지 기반으로 접근 제어를 한다는 사실이 맘에 들진 않아요 ㅋㅋ (코틀린을 쓰면 그런 고민할 필요가 없습니다 갓틀린 짱짱..)

답은 없으니 각 제어자 목적에 맞게 고민하고 판단하셔서 결정하시면 됩니다 :)

wbluke · 2022-03-03T13:08:28Z

src/main/java/lotto/domain/LottoTicket.java

@@ -32,4 +32,8 @@ public WinningResult calculateWinningStatistic(WinningNumbers winningNumbers) {
                .collect(collectingAndThen(toList(), Collections::unmodifiableList));
        return new WinningResult(rankings);
    }
+
+    public void buyManualTicket(List<LottoNumbers> manualTickets) {


네네 자동로또와 수동로또를 합친다면 저도 위와 같이 했을 거에요.
대신 행위가 buy라는 네이밍과는 맞지 않는 것 같아서, add~ 같은 게 좋지 않을까 싶네요 :)

wbluke · 2022-03-03T13:10:03Z

src/test/java/lotto/domain/LottoNumbersTest.java

    @Test
    @DisplayName("숫자가 6개 미만")
    void incorrect4() {
-        assertThatThrownBy(() -> new LottoNumbers("2,5,6")).isInstanceOf(IllegalArgumentException.class)
+        // given
+        List<Integer> numbers = getNumbers(2, 5, 6);


보통 이런 테스트는 항상 edge 케이스를 검증하도록 해야 합니다.
6개 미만인 케이스라면 5개의 숫자로 검증을 해야 하고요, 1 ~ 45 사이의 수를 검증한다면 0과 46을 테스트 케이스로 선정하는 것이 중요합니다 :)

wbluke · 2022-03-03T13:12:01Z

src/test/java/lotto/domain/ManualLottoCountTest.java

+        // when
+        int tryCount = 9;
+
+        // then
+        assertDoesNotThrow(() -> new ManualLottoCount(tryCount, max));


여기도 when절은 검증하고자 하는 행위여야 하기 때문에 조금 달라져야 할 것 같아요.
int tryCount = 9;는 행위가 아니라 테스트를 하기 위한 given절의 내용이겠죠?
보통 assertDoesNotThrow() 같은 경우 when, then이 같이 있는 것으로 이해하면 됩니다 :)

- getLottoTickets -> getLottoTicket 메소드명 변경 - buytManualTicket -> addLottoTicket 메소드명 변경 - List<LottoNumbers>를 파라미터로 받는 생성자 생성하며 final 키워드 제거

- LottoTicket 추가하는 경우 테스트 케이스 추가 - 숫자가 6개 미만인 LottoNumbers 생성 시 경계값으로 테스트

yeon-06 · 2022-03-04T14:07:11Z

안녕하세요 닉!
리뷰 감사합니다~! 리팩토링 진행 후에 다시 리뷰 요청 드려요 😊

이름 짓기

이름 짓는게 정말 어렵네요😂
혹시 닉은 이름 짓는 팁 같은거 있으실까요?👀
클래스 내부의 private 필드 가져올 때 getXXX()을 쓰듯이 일반적으로 사용하는 메소드명 같은게 또 따로 있을지 궁금하네요!

정적 팩토리 메소드

LottoTicket에 정적 팩토리 메소드를 도입했습니다.
자동 로또를 발급하는 경우, 수동 로또를 발급하는 경우를 분명히 구분해두었어요

기타

위 변경 사항 외에는 전체적으로 test 케이스를 수정/추가하고 메소드명 변경이 진행되었습니다.

추가 질문

네오의 강의에서 BiPredicate라는 개념을 새롭게 알게 되었어요
공부하며 블로그에 정리하고 Ranking Enum에 직접 적용도 해보았는데요

count와 hasBonusNumber 여부에 따라 Ranking이 달라지니까 이를 BiPredicate로 적용했는데
View에서 count와 hasBonusNumber 값이 필요해서 결국 코드를 롤백 시켰습니다
블로그 글 하단 - 리팩토링 부분에 코드 포함

Predicate라는 개념을 실무에서 사용하는 상황이 있을지 궁금해요!

yeon-06 · 2022-03-05T02:25:10Z

src/main/java/lotto/domain/Ranking.java

+                .orElse(NONE);
+    }
+
+    private static boolean checkCountAndBonusNumber(Ranking ranking, int count, boolean hasBonusNumber) {


슬랙에서 테스트 케이스를 하나 제시해주신 크루가 계셔서 로직을 다시 보게 되었어요
2등이 아니더라도 hasBonusNumber가 true로 들어오면 NONE으로 반환되는 현상을 개선했습니다

다만 앞으로도 hasBonusNumber가 true인 값들이 생길때마다 if문 처리해주는건 이상할 것 같아요
그렇다고 BiPredicate를 활용하자니 이미 count가 hasBonusNumber가 상수로 존재하는데 이를 활용하지 못하는게 아닐까? 라는 생각이 드네요
닉이라면 어떤 시도를 해보실 것 같나요?😂

음 그런데 현재 Rank 관련 테스트가 깨지고 있어서, 의도한대로 기능이 동작하지 않고 있어요.
한번 확인해주시고, 2등과 3등의 구분이 문제라면 2등을 ealry return 하는 방법도 한번 생각해보셔요 :)

이 부분 체크를 누락됐네요💦 로직 개선했습니다!

wbluke

블로그에 정리해주신 내용 잘 봤습니다 👍
다만 현재 깨지는 테스트가 있어서, 기능이 올바르게 동작하지 않고 있어요.
한번 더 확인 부탁드려요 🙂

wbluke · 2022-03-05T02:57:46Z

src/main/java/lotto/controller/LottoController.java


        if (manualTicketCount != 0) {
-            lottoTicket.buyManualTicket(buyManualTicket(manualLottoCount));
+            lottoTicket.addLottoTicket(buyManualTicket(manualLottoCount));


이름 짓기

이름 짓는게 정말 어렵네요😂
혹시 닉은 이름 짓는 팁 같은거 있으실까요?👀
클래스 내부의 private 필드 가져올 때 getXXX()을 쓰듯이 일반적으로 사용하는 메소드명 같은게 또 따로 있을지 궁금하네요!

getter/setter 같은 네이밍은 관용어구에 가깝지만, 다른 네이밍은 좋은 코드를 많이 보다보면 감이 느는 것 같아요 ㅎㅎ
(감이 는다고 했지 익숙해진다고는 안했습니다,,)
연차 무관하게 어려움을 느끼는 지점이라고 생각해요 :)

wbluke · 2022-03-05T03:00:32Z

src/main/java/lotto/domain/Ranking.java

        return Arrays.stream(Ranking.values())
-                .filter(ranking -> ranking.count == cnt && ranking.hasBonusNumber == hasBonusNumber)
+                .filter(ranking -> checkCountAndBonusNumber(ranking, count, hasBonusNumber))


추가 질문

네오의 강의에서 BiPredicate라는 개념을 새롭게 알게 되었어요
공부하며 블로그에 정리하고 Ranking Enum에 직접 적용도 해보았는데요

count와 hasBonusNumber 여부에 따라 Ranking이 달라지니까 이를 BiPredicate로 적용했는데
View에서 count와 hasBonusNumber 값이 필요해서 결국 코드를 롤백 시켰습니다
블로그 글 하단 - 리팩토링 부분에 코드 포함

Predicate라는 개념을 실무에서 사용하는 상황이 있을지 궁금해요!

오 네 그럼요. Predicate 뿐만 아니라 이를 비롯한 함수형 인터페이스를 공부해놓으면 유용하게 사용할 일이 많습니다 :)

wbluke · 2022-03-05T03:09:00Z

src/main/java/lotto/domain/Ranking.java

+                .orElse(NONE);
+    }
+
+    private static boolean checkCountAndBonusNumber(Ranking ranking, int count, boolean hasBonusNumber) {


음 그런데 현재 Rank 관련 테스트가 깨지고 있어서, 의도한대로 기능이 동작하지 않고 있어요.
한번 확인해주시고, 2등과 3등의 구분이 문제라면 2등을 ealry return 하는 방법도 한번 생각해보셔요 :)

wbluke · 2022-03-05T03:09:21Z

src/main/java/lotto/domain/Ranking.java

                .findAny()
                .orElse(NONE);
    }

+    private static boolean checkCountAndBonusNumber(Ranking ranking, int count, boolean hasBonusNumber) {
+        if (count == 5) {


매직넘버!

5 -> SECOND.getCount() 로 변경 완료하였습니다!

wbluke

로또 미션 고생 많으셨어요~! 🙌
새롭게 학습한 내용으로 다음 미션도 화이팅입니다 🙂

wbluke · 2022-03-05T06:47:39Z

src/main/java/lotto/domain/Ranking.java

+        if (ranking.count != count) {
+            return false;
+        }
+        if (count == SECOND.getCount()) {


Suggested change

if (count == SECOND.getCount()) {

if (count == SECOND.count) {

해당 클래스 내의 로직이니 getter를 거치지 않아도 될 것 같아요 :)

yeon-06 added 15 commits March 2, 2022 11:48

docs: 리드미 갱신 - 요구사항 추가

6d80ecb

refactor: LottoNumbers input 값의 최소한의 검증 View의 책임으로 변경

820a506

style: 변수명 변경 number -> value

f7db213

refactor: 인스턴스 캐싱 방식 변경

cf4410f

- array -> map 활용 - 미리 전부 생성해두지 않고 필요할 때마다 생성해서 저장

refactor: 하드코딩 제거, 형식 통일화

137b7c2

- 하드 코딩 상수화 - String.format()을 사용해 형식 통일

style: 우승 상금이 없는 경우 DEFULAT -> NONE 이름 변경

3b9694f

test: given, when, then 구분

829c37d

refactor: LottoNumber의 접근제어자 private -> protected 변경

c71c797

feat: 수동 로또 개수 클래스 생성

b7a65d2

feat: 수동 로또 입력 기능 구현

ac8befc

feat: 수동 로또 개수 입력 기능 구현

9f08b0c

refactor: 하드코딩 제거

964e57a

style: 오류 문구 수정

e45fd9e

refactor: 수동 구매 시 남은 횟수도 출력되도록 수정

7796450

docs: 리드미 갱신 - 구현 완료 기능 체크

dcd4bcd

yeon-06 commented Mar 2, 2022

View reviewed changes

wbluke suggested changes Mar 3, 2022

View reviewed changes

yeon-06 added 7 commits March 4, 2022 20:58

refactor: 정적 팩토리 메소드 도입

3d94fe2

test: 테스트 케이스 수정

2701bcc

- LottoTicket 추가하는 경우 테스트 케이스 추가 - 숫자가 6개 미만인 LottoNumbers 생성 시 경계값으로 테스트

test: given, when, then 분리

abb7362

test: findRanking 메소드 테스트 코드 추가

ecaba3c

style: 1000단위 구분자 표시

cb8cfb6

style: 메소드명 변경

4646a00

refactor: equals(), hashCode() 재정의

affa474

yeon-06 added 2 commits March 5, 2022 11:16

fix: 2등이 아닌데 보너스볼이 포함되는 경우 NONE으로 반환되는 현상 수정

293fe25

refactor: findRanking() 메소드 분리

581f834

yeon-06 commented Mar 5, 2022

View reviewed changes

style: 사용하지 않는 import 제거

64660e7

wbluke suggested changes Mar 5, 2022

View reviewed changes

fix: 3위가 반환되지 않는 현상 수정

6951fa8

wbluke approved these changes Mar 5, 2022

View reviewed changes

wbluke merged commit af383ca into woowacourse:yeon-06 Mar 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 로또(수동)] 연로그(권시연) 미션 제출합니다. #454

[2단계 - 로또(수동)] 연로그(권시연) 미션 제출합니다. #454

yeon-06 commented Mar 2, 2022

yeon-06 Mar 2, 2022

wbluke Mar 3, 2022

yeon-06 Mar 2, 2022

yeon-06 Mar 2, 2022

wbluke Mar 3, 2022

yeon-06 Mar 2, 2022

yeon-06 Mar 2, 2022

yeon-06 Mar 3, 2022

wbluke Mar 3, 2022

wbluke left a comment

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

yeon-06 commented Mar 4, 2022

yeon-06 Mar 5, 2022

wbluke Mar 5, 2022

yeon-06 Mar 5, 2022

wbluke left a comment

wbluke Mar 5, 2022

wbluke Mar 5, 2022

wbluke Mar 5, 2022

wbluke Mar 5, 2022

yeon-06 Mar 5, 2022

wbluke left a comment

wbluke Mar 5, 2022


		private static final LottoNumber[] cacheLottoNumber = new LottoNumber[MAX + 1];
		private static final Map<Integer, LottoNumber> cacheLottoNumber = new HashMap<>();

	if (count == SECOND.getCount()) {
	if (count == SECOND.count) {

[2단계 - 로또(수동)] 연로그(권시연) 미션 제출합니다. #454

[2단계 - 로또(수동)] 연로그(권시연) 미션 제출합니다. #454

Conversation

yeon-06 commented Mar 2, 2022

2단계 추가 요구 사항

1단계 피드백

고민

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbluke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeon-06 commented Mar 4, 2022

이름 짓기

정적 팩토리 메소드

기타

추가 질문

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbluke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

이름 짓기

Choose a reason for hiding this comment

추가 질문

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbluke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment