[2단계 - 로또(수동)] 이프(송인봉) 미션 제출합니다. by sinb57 · Pull Request #461 · woowacourse/java-lotto

sinb57 · 2022-03-02T13:06:50Z

안녕하세요, 코니~ 이프입니다!!!

2단계를 진행함에 있어서, 지난번 PR에 남겨주신 코멘트가 많은 도움 되었습니다!!

지난번에 남겨주신 코멘트에 대한 답변은 아래에 적어두겠습니다!

2단계도 잘 부탁드립니다!

* refactor: Balls, 모든 번호를 미리 캐싱해놓는 방식에서 요청할 때마다 생성하는 방식으로 변경

sinb57 · 2022-03-02T13:18:31Z

src/main/java/lotto/view/utils/Delimiter.java


-    public static String[] splitWithComma(final String targetString) {
-        return targetString.split(COMMA.unit);
+    public List<String> splitWith(final String targetString) {
+        final int limitForSplitAllElement = -1;
+        return List.of(targetString.split(this.unit, limitForSplitAllElement));
    }

-    public static String appendSpaceBehind(final String targetString) {
-        return targetString + SPACE.unit;
-    }
-
-    public static String joinWithComma(final List<String> strings) {
-        return String.join(COMMA.unit + SPACE.unit, strings);
+    public String joinWith(final List<String> strings) {
+        return String.join(this.unit + SPACE.unit, strings);


구분자가 수정된다면 splitWithComma(), joinWithComma() 메서드의 이름이 바뀌어야 하겠죠. Delimiter라는 친구에게 책임을 위임하긴 했으나 그 구분자가 무엇인지를 메서드 이름이 상세하게 드러내고 있어요.

이름을 통해 해당 메서드의 역할을 명시하고 싶었는데, 지적해주신 것처럼 메서드가 구분자와 너무 밀접한 관계를 이루고 있네요.ㅜ 해당 부분은 static 키워드를 제거하고 메서드명을 수정함으로써 구분자와의 관계를 끊었습니다!

sinb57 · 2022-03-02T16:01:39Z

src/main/java/lotto/exception/LottoExceptionStatus.java

+import lotto.utils.TicketSize;

-    String getMessage();
+public enum LottoExceptionStatus {


이와 같이 하는 경우, LottoExceptionStatus에 미리 정해져있는 예외메시지 이외의 값을 LottoException 에 담는 것을 차단할 수 있는 장점이 있습니다.
개발자는 throw new LottoException("이거 예외인가요?")와 같이 함부로 String 값을 담아서 예외를 던질 수 없고, LottoExceptionStatus를 implement한 enum 클래스를 통해 예외메시지를 관리해야만 합니다.
때문에 ExceptionStatus 개념의 도입은 예외메시지를 별도 관리한다는 점에서 유지보수가 편리해지며, 사전에 지정된 예외메시지 외의 값을 임의로 사용하지 못하도록 강제하기 때문에 프로그래밍 관점에서 안전성이 보장됩니다.

예외에 함부로 String 값을 담으면 안 되는 걸까요? 만약 프로그램이 커진다면 예외도 많아질텐데, 새로운 예외가 생길 때마다 LottoExceptionStatus의 구현체를 만들어야 하는 것이 부담이 되지는 않을까요? 예외 메시지를 별도 관리한다는 점에서 유지보수가 편리해진다고 하셨는데, 지금 LottoExceptionStatus의 구현체가 모두 흩어져 있기 때문에 원하는 예외 메시지를 담은 구현체가 이미 있는지 찾기도 쉽지 않아 보여요.

꼭 제 의견이 정답이라기보다는, 다른 사람은 이렇게 생각할 수도 있다는 정도로 봐주셔도 될 것 같아요.

좋은 말씀 감사합니다ㅜ!! 이 부분에 대한 제 생각은 처음과 달리 지금은 달라졌는데요.
우선 예외메시지를 Enum으로 관리하고 LottoException의 생성자 매개변수 타입이 LottoExceptionStatus인 것에 대해서는 변함이 없으나, LottoExceptionStatus의 구현체로 관리하는 것에 대해서는 지적하신 부분을 수용하였습니다.

하나씩 말씀드리자면, 우선 LottoExceptionStatus라는 Enum 클래스를 통해 예외메시지를 다루게 된 이유에 대해 설명드리겠습니다. 예외메시지를 Enum으로 관리하는 것은 관리측면에서도 확장측면에서도 큰 장점이 있다고 생각합니다. 관리측면에서 바라봤을 때, 특정 예외메시지를 수정하기 위해 프로젝트 전체를 확인할 필요 없이 예외메시지가 모여있는 LottoExceptionStatus 클래스만 확인하고 수정하면 되므로 관리하기 용이하다는 장점이 있습니다. 그리고 LottoExceptionStatus를 Enum 클래스로 생성한 이유는 확장성 때문인데, 에러코드와 같이 예외메시지와 연관된 추가적인 필드값을 추가/관리해야 하는 상황에서 코드 변경에 사용될 비용을 고려한다면 처음부터 Enum 클래스로 관리하는 것이 오히려 더 유리하다고 판단했습니다.

LottoException의 생성자 매개변수 타입으로 LottoExceptionStatus로 제한한 이유는 LottoExceptionStatus의 목적 때문인데요. 앞서 서술하였듯 LottoExceptionStatus은 예외메시지를 한 곳에서 관리하려는 목적을 지니고 있습니다. 예외메시지를 한 곳에서 관리하기 위해 LottoExceptionStatus Enum 클래스를 생성한 것인데, LottoExceptionStatus에 예외메시지를 정의해놓지 않고서, 함부로 LottoExcpetion을 생성하고 사용하는 것은 LottoExceptionStatus의 존재 의의에 어긋난다고 생각했습니다. 따라서 LottoException의 생성자 매개변수를 String 타입이 아닌 LottoExceptionStatus로 제한함으로써 예외메시지는 해당 클래스에서 관리되어야 함을 보장했습니다.

그에 반해 LottoExceptionStatus 구현체 활용에 대해서는 이번 미션2를 진행하면서 생각이 바뀌었는데요. 지금은 LottoExceptionStatus Enum 클래스 안에 모든 예외메시지를 담아둔 것에 비해, 기존에는 LottoExceptionStatus를 interface로 두고서 MoneyExceptionStatus 등의 Enum 구현체를 생성하여 예외메시지를 도메인별로 묶었습니다. 당시에는 연관된 메시지끼리 묶어서 분리했기 때문에 유지보수가 좋다고 생각했지만, 코니의 피드백을 보고서 예외메시지를 한곳에 모아 놓은 이유가 관리를 용이하게 하기 위함인데 굳이 도메인별로 쪼갤 필요가 있을까 하는 의문을 갖게 되었습니다. 오히려 로또 수동 구매 기능 구현 과정에서는 이 예외메시지는 어느 구현체에 넣어야하는거지? 라는 생각을 가질 정도로 저 스스로 구현체 활용에 대해 혼란을 느끼기도 했습니다. 그래서 결국 도메인별로 분리한 인터페이스 구현체를 하나로 합쳐서 지금의 LottoExceptionStatus를 이용하게 되었습니다.

오히려 로또 수동 구매 기능 구현 과정에서는 이 예외메시지는 어느 구현체에 넣어야하는거지? 라는 생각을 가질 정도로 저 스스로 구현체 활용에 대해 혼란을 느끼기도 했습니다.

이렇게 직접 느껴야만 배울 수 있는 것들이 많더라구요 👍 저도 공감이 많이 됩니다.

sinb57 · 2022-03-02T16:19:32Z

src/main/java/lotto/view/input/InputView.java

    public int requestMoney() {
-        return parseMoney(reader.readLine());
+        final String inputLine = reader.readLine();
+        return parseNumber(inputLine, LottoExceptionStatus.MONEY_MUST_BE_NUMERIC);


메서드를 보통 어떤 순서로 배치하고 있으신가요?

클린 코드에서 다루듯이 수직 거리(세로 밀집도)를 생각하며 배치하고 있습니다.

저번 PR 코멘트에서 선택해주신 코드 부분이 InputView#parseNumber 인데요.
parseNumber를 최상단에 배치한 이유에 대해 궁금해하신 것으로 생각됩니다.

parseNumber의 위치를 requestMoney의 아래로 배치할 수 있지만,
저는 parseNumber가 requestMoney 뿐만 아니라 requestTicketCount, requestTicketNumbers 등에서
사용되기 때문에 이들 집합군의 상단으로 requestMoney 위에 배치했습니다.

배치순서 1. parseNumber 2. requestMoney 3. requestTicketCount 4. requestTicketNumbers

하지만 다른 이는 종속 관계는 무조건 위에서 아래로 나열되어야 한다는 주장으로 다음과 같이 배치할 수도 있습니다.

배치순서 1. requestMoney 2. parseNumber 3. requestTicketCount 4. requestTicketNumbers

그러나 저는 단순히 스타일의 차이라고 생각하는데요.
코니는 이런 상황에서 어떠한 방식으로 메서드를 배치하나요?

사실 메서드 배치 순서는 일관성을 지키는 게 생각보다 어렵고, 스타일 차이로 볼 수 있는 여지도 많이 있어요. 하지만 결국 클래스에서 무슨 일을 하는지, 어떤 책임을 지고 있는지를 명확히 드러내는 것은 공개된 인터페이스, 즉 public 메서드이기 때문에, 그리고 보통 코드는 위에서 아래로 읽어내려가기 때문에, 후자의 방식이 훨씬 선호되고 저는 전자를 본 기억이 없네요. 게다가 parseNumber() 메서드의 경우 특별한 로직이 있지도, 크게 중요하지도 않은 메서드이구요.

메서드의 배치 순서는 코드를 읽기 편하게 하기 위함이라고 생각해요.
때문에 배치 순서 만큼이나 메서드의 결집도 중요하다고 생각하는데요.
위의 예시에 따르면 첫번째 배치순서의 경우, 2,3,4가 군집을 형성하므로
1 메서드가 무슨 역할을 수행하는지 먼저 알고 넘어가면
뒤따라오는 2,3,4를 읽기 수월해진다고 생각했어요.

물론 이는 두번째 배치순서 예시도 마찬가지지만요.
그렇다면 전자와 후자의 차이는 공통적으로 사용되는 메서드를
먼저 알려줄거냐 / 도중에 알려줄거냐의 스타일 차이라고 생각했어요.

컨벤션은 역시 어렵네요.ㅠㅜ
혹시 리뷰를 위해서 코드를 읽으실 때 parseNumber의 위치때문에 혼란을 느끼셨나요?
아니면 그저 독특해서 물어봐주신건가요?

컨벤션은 코드를 읽기 좋게 하는, 사람들과의 약속이니까요!
만약 혼란을 느끼셨다면, 제 컨벤션이 읽기 안 좋다는 것을 의미하니 이번 기회에 고치겠습니다!!
하지만 그것이 아니라면.. 앞으로 좀더 지켜보며 다른 분들의 의견도 들어보려고 합니다!!!

sinb57 · 2022-03-02T16:33:01Z

src/main/java/lotto/domain/ticket/TicketValidator.java

+    private static boolean isNumberDuplicated(final List<Integer> ballNumbers) {
+        return ballNumbers.stream()
+                .anyMatch(ballNumber -> Collections.frequency(ballNumbers, ballNumber) > 1);


"지나친" 중복이란 게 있을까요? 중복이라는 단어로 충분한 의미를 전달할 수 있지 않나요? 😃

지난번에 BallNumberDuplication#isExcessiveDuplicated 메서드를 지적해주셨습니다.
이 부분은 자동차 경주 미션에서 리뷰어가 isNumberDuplicated 의 1를 지적한 것으로부터 시작되었습니다.
이미 isNumberDuplicated라는 메서드명에서도 알 수 있듯이 중복을 확인하는 역할을 수행하기 때문에
1이라는 숫자는 그 의미를 파악하기에 무리가 없으리라 판단을 했지만, 피드백을 적용하고자 노력했고
고민 끝에 중복 허용 개수라는 의미를 부여하게 되었습니다. 그 결과 BallNumberDuplication Enum 클래스를
만들어 해당 클래스에 isExcessiveDuplicated 메서드로 책임을 전가했습니다.

허나 처음 지녔던 생각처럼 1이라는 숫자를 분리할 필요가 없다면 BallNumberDuplication 클래스를
제거하고 위와 같이 작성하는 것이 코니가 지적한 부분을 해결하는 방법이라 생각됩니다.

sinb57 · 2022-03-02T16:34:17Z

src/main/java/lotto/view/output/OutputMessage.java

-    TICKET_COUNT_FORMAT("%d개를 구매했습니다."),
-    PROFIT_RAGE_FORMAT("총 수익륭은 %.2f입니다."),
+    TICKET_COUNT_FORMAT("수동으로 %d장, 자동으로 %d개를 구매했습니다."),
+    PROFIT_RAGE_FORMAT("총 수익률은 %.2f입니다."),


저번엔 죄송했습니다ㅜ 수정하겠다고 코멘트를 달았지만 미처 확인하지 못했습니다ㅠㅜ
수익륭 이번에는 제대로 수정했습니다!!!

sinb57 · 2022-03-02T16:45:21Z

src/main/java/lotto/view/output/OutputMessage.java

@@ -2,16 +2,18 @@

 public enum OutputMessage {


일반적으로(?) 특정 클래스 내에서만 사용되는 상수라면 그 안에서 선언하여 사용하는 게 더 편할 것 같은데 굳이 별도 클래스에 분리하신 점, 그리고 여기에 특별히 enum을 사용하신 이유를 질문한 것으로 보시면 될 것 같아요.

지금은 OutputMessage 클래스를 LottoView에서만 사용하고 있지만,
추후 새로운 View가 추가되었을 때 해당 클래스를 재사용할 수 있다는 장점이 있으며,
해당 클래스는 상수들의 집합으로 구성되었기 때문에 목적에 맞게 enum 클래스로 생성했습니다.

충분히 합리적인 의견이라 생각되어요 👍

다만 이에 관해 제 의견도 조금 드려보자면,

확장 가능성을 미리부터 너무 고민할 필요는 없어요. 구체적인 정보 없이 미리 한 고민들이 나중에 할 고민을 덜어주기보다는, 오히려 유지보수 용이성을 저하시키는 경우가 많은 것 같습니다.

예를 들어 OutputMessage에도, 콘솔 뷰에 한정되어 사용될 수밖에 없는 메시지들이 대다수입니다. 환경이 변하면 대부분 메시지도 변해야 하고, 아마 OutputMessage는 재사용하기 어려울 거예요.

그리고 코드를 전체적으로 보면 enum 사용을 선호하시는 것으로 보입니다. 그런데 enum은 말씀하신 것처럼 관련이 있는 상수들의 집합을 나타내기 위해 주로 사용합니다. 하지만 MoneyUnit이나 TicketSize 같은 경우 의미적으로나 사용하시는 모습으로 보나 상수들의 집합이라기에는 거리가 있어 보입니다. 그래서 왜 여기에 enum을 사용하셨는지 의문을 가지게 하는 것 같아요.

아하!! enum 클래스를 사용하는 기준에 대해 지적해주시는 것이었군요!
제가 사용하는 enum은 OutputMessage나 LottoExceptionStatus와 같이 상수의 집합 목적도 있지만,
단순히 상수를 분리하는 것이 아니라 MoneyUnit이나 TicketSize 처럼
해당 상수가 책임져야 하는 행동을 같이 분리하기 위해 enum 클래스를 사용하고 있습니다!!
주로 후자처럼 상수와 행동을 묶기 위해 enum 클래스를 이용하고 있습니다!

sinb57 · 2022-03-02T17:52:49Z

src/main/java/lotto/dto/TicketDto.java


    public List<Integer> getBallNumbers() {
-        return this.ballNumbers;
+        return List.copyOf(ballNumbers);


비록 이부분은 아니었지만, 지난번에 List.copyOf를 사용한 이유를 물어보셔서 답변드리도록 하겠습니다!!
다른 클래스들의 생성자나 getter 코드를 보시면 아시겠지만, 방어적 복사 및 불변성 보장을 위해 신경썼습니다.

코드가 많이 혐오스럽지만ㅎ.......
Collections.unmodifiableList는 원본 컬렉션의 영향을 받고 있음을 확인할 수 있으며,
new ArrayList는 데이터 추가/삭제가 가능한 것을 볼 수 있습니다.

number3과 number4는 원본 컬렉션의 영향을 받지 않음과 동시에,
데이터 추가/삭제 시 UnsupportedOperationException를 통해
컴파일 타임에서 데이터 변조를 막아 불변성을 보장 받습니다.

물론 List.copyOf와 stream().collect(Collectors.toUnmodifiableList())가
완전히 불변성을 보장 받는 것은 아닙니다.
위의 예시와 달리 아래의 예시를 보시면, 컬렉션의 타입이 원시값이 아닌 객체인 경우
컬렉션의 요소에 대한 상태 변경은 막지 못하는 것을 확인할 수 있습니다.

대신 number4와 같이 stream을 통해 모든 요소에 대한 복사를 진행하고
Collectors.toUnmodifiableList로 생성한 경우는, 원본 컬렉션과 완전히 독립된 객체를 생성함과 동시에
UnsupportedOperationException을 통해 불변성을 보장받음을 알 수 있습니다.

하지만 그럼에도 TicketDto#getBallNumbers에서 List.copyOf를 이용한 이유는,
첫번째 예시와 마찬가지로 컬렉션의 요소가 원시값이기 때문입니다!

sinb57 · 2022-03-02T18:02:57Z

src/main/java/lotto/dto/AnalysisDto.java

+    private final Map<Rank, Long> rankCounts;
    private final double profitRate;

-    public AnalysisDto(final List<Rank> ranks, final int ticketCount) {


이전에 AnalysisDto가 다른 역할을 많이 맡고 있다고 지적해주셔서 감사합니다!

처음에는 단순하게 생각해서, DTO가 생성 당시에 당첨 통계을 계산해서 지니고 있으면
따로 Analysis 클래스를 만들 필요도 없고, 책임도 부여되어 있으니 좋다고 착각했습니다.

애당초 LottoController와 LottoService와 같은 잘못된 설계를 통해 로직을 짜다보니
DTO에 비즈니스 로직을 넣으려는 잘못된 생각을 가져버린듯 합니다.

지금은 Analysis을 생성해 당첨 통계 계산 책임을 지게 했습니다.

sinb57 · 2022-03-02T18:11:26Z

src/main/java/lotto/LottoApplication.java

+import lotto.dto.WinningTicketDto;
+import lotto.view.LottoView;

 public class LottoApplication {


LottoService는 현재 Tickets를 인스턴스 변수로 가지고 있는데, 이를 보통 "상태를 가지고 있다"고 표현하곤 하죠. 그런데 LottoService가 이 발행된 티켓이라는 상태를 가지고 있는 것이 적절한 걸까요?

덕분에 MVC에 대해서 잘못 생각하고 있었음을 깨달았습니다.
프리코스 때부터 지금껏 쭈욱 웹 MVC를 생각하며 미션에 임하고 있었습니다.
때문에 Controller와 Service 계층이 필요할 것이라고 생각했고,
이 과정에서 Tickets의 상태를 어떻게 관리하지 고민하다가
안되는 걸 알면서도 어쩔수 없다는 자기위안으로 인스턴스 변수를 생성해두었습니다.
돌이켜보면 이도 저도 아닌 코드 덩어리였습니다..

이번에 미션2를 진행하면서 기존의 구조를 개편했습니다. 기존의 LottoService를 제거했고,
LottoApplication이 MVC의 컨트롤러로써 프로그램의 실행 흐름을 제어하는 역할을 수행하도록 만들었습니다.

변경된 구조에 대해서는 코니는 어떻게 생각하시는지 궁금합니다!

1단계보다 많이 깔끔해지고 명확해진 것 같습니다 👍

choihz

안녕하세요, 이프!

2단계도 잘 구현해 주셨네요 👍 이전 PR과 이번 PR에 남겨주신 코멘트를 모두 확인하고, 답이 필요한 경우 코멘트를 달아 두었습니다. 이외에도 더 궁금한 점이나 의견이 있으시다면 언제든 코멘트 또는 DM 주세요~

choihz · 2022-03-03T15:10:18Z

src/main/java/lotto/view/input/InputView.java

@@ -6,8 +6,6 @@

 import lotto.exception.LottoException;


위에 사용하지 않는 import 문이 있네요. 전체적으로 확인해 주세요.

choihz · 2022-03-03T15:12:18Z

src/main/java/lotto/view/output/OutputMessage.java


-    TICKET_COUNT_FORMAT("%d개를 구매했습니다."),
-    PROFIT_RAGE_FORMAT("총 수익륭은 %.2f입니다."),
+    TICKET_COUNT_FORMAT("수동으로 %d장, 자동으로 %d개를 구매했습니다."),


앞에는 "장", 뒤에는 "개"네요. 미션 예시에 이렇게 나와 있어 그대로 사용하신 것으로 보이고 크게 중요하지는 않지만요 ㅎㅎ

맞아요ㅎㅎ지적해주시니 이제서야 이게 보이네요!! 수정하겠습니다~

choihz · 2022-03-03T15:32:48Z

src/main/java/lotto/view/output/OutputMessage.java

@@ -2,16 +2,18 @@

 public enum OutputMessage {


충분히 합리적인 의견이라 생각되어요 👍

다만 이에 관해 제 의견도 조금 드려보자면,

확장 가능성을 미리부터 너무 고민할 필요는 없어요. 구체적인 정보 없이 미리 한 고민들이 나중에 할 고민을 덜어주기보다는, 오히려 유지보수 용이성을 저하시키는 경우가 많은 것 같습니다.

예를 들어 OutputMessage에도, 콘솔 뷰에 한정되어 사용될 수밖에 없는 메시지들이 대다수입니다. 환경이 변하면 대부분 메시지도 변해야 하고, 아마 OutputMessage는 재사용하기 어려울 거예요.

그리고 코드를 전체적으로 보면 enum 사용을 선호하시는 것으로 보입니다. 그런데 enum은 말씀하신 것처럼 관련이 있는 상수들의 집합을 나타내기 위해 주로 사용합니다. 하지만 MoneyUnit이나 TicketSize 같은 경우 의미적으로나 사용하시는 모습으로 보나 상수들의 집합이라기에는 거리가 있어 보입니다. 그래서 왜 여기에 enum을 사용하셨는지 의문을 가지게 하는 것 같아요.

choihz · 2022-03-03T15:39:11Z

src/main/java/lotto/domain/ticket/TicketManager.java

+import lotto.domain.winning.WinningTicket;
+import lotto.utils.Rank;
+
+public class TicketManager {


사실 ~Manager라는 이름은 굉장히 흔하게 사용되어요. 하지만 많은 경우, 이 이름은 객체의 역할을 잘 드러내지 못하는 것 같습니다. TicketManager를 사용하는 클라이언트의 입장에서 이 객체에게 어떤 역할을 기대하게 될까요? 티켓 생성? 티켓 보관? 티켓 관리?

또한 이런 모호한 이름은 결국 모든 애매한(?) 로직들을 다 끌어당기는 참사를 야기하기도 합니다 🥲

맞습니다!! 이름이 애매하죠ㅜㅠ 마땅한 이름이 없어서 Manager를 사용했습니다..
지금은 나름대로(?) 만족스런 이름으로 바꿨습니다! TicketManager -> TicketBundles

choihz · 2022-03-03T15:39:51Z

src/main/java/lotto/domain/ticket/TicketManager.java

+    private final Tickets preparedTickets;
+    private final Tickets generatedTickets;


인스턴스 변수의 이 이름들은 의미를 바로 파악하기 어려운 것 같네요.

TicketGenerator에 따라 로또 생성 방식이 달라진다는 생각에,
TicketManager의 인스턴스 변수명을 manual(수동)/automatic(자동)으로 두지 않고
prepared(넘겨받은)/generated(자체생성한)으로 의미부여했습니다.

하지만 로직을 다시 들여다보니 전략패턴에 따른 로또 생성 로직을
Tickets가 가져야하는 의구심(오버라이딩까지 해가면서???)을 갖게 되었고,
이에 따라 해당 로직을 Tickets에서 한단계 위의 TicketManager로 옮겼습니다.

로또 자동 생성을 TicketManager가 책임지게 되자, TicketManager에 보다 명확한 의미 부여가 가능해졌습니다!!ㅎ

choihz · 2022-03-03T16:03:00Z

src/test/java/lotto/domain/ball/BallStorageTest.java

+    @DisplayName("동일한 Ball 객체를 반환해야 한다.")
+    @ParameterizedTest(name = "[{index}] 번호 : {0}")
+    @ValueSource(ints = {1, 2, 44, 45})
+    void cachedBallsSameTest(final int ballNumber) {
+        final Ball actualBall = BallStorage.getBall(ballNumber);
+        final Ball expectedBall = BallStorage.getBall(ballNumber);
+        assertThat(actualBall).isSameAs(expectedBall);
+    }


위에 있는 테스트 메서드와 중복 아닌가요?

어라라....ㅎㅎ..없앴습니다!

choihz · 2022-03-03T16:03:48Z

src/test/java/lotto/domain/ball/BallStorageTest.java

+    @DisplayName("번호가 동일해도, 캐싱되지 않은 Ball 객체와 일치해선 안된다.")
+    @ParameterizedTest(name = "[{index}] 번호 : {0}")
+    @ValueSource(ints = {1, 2, 44, 45})
+    void ballsNotSameIfNotGeneratedThroughBallStorageTest(final int ballNumber) {
+        final Ball actualBall = BallStorage.getBall(ballNumber);
+        final Ball expectedBall = new Ball(ballNumber);
+        assertThat(actualBall).isNotSameAs(expectedBall);
+    }


이건 의미 있는 테스트인지 잘 모르겠네요 🤔

아! 여기서 의도한 것은 isNotSameAs가 아니라 isNotEqualTo입니다!! (코드 수정했습니다.)

이 테스트는 DisplayName에서 밝힌 바와 같이 번호가 같아도 동일한 객체로 보지 않겠다는 의도를 지니고 있습니다.

BallStorage는 Ball 객체가 과도하게 생성되는 것을 방지하기 위해 (재사용하기 위해서)
Ball 객체가 캐싱하는 목적으로 생성되었습니다.

따라서 어플리케이션 내의 모든 Ball 객체는 BallStorage를 통해서만 생성되고 관리되어야만
BallStorage의 생성 목적(캐싱)에 부합하다고 생각했습니다.

이를 위해서 Ball 생성자의 접근제어자를 default-package 로 지정함으로써
같은 패키지에 존재하는 BallStorage를 통해서만 Ball 객체가 생성되도록 제한해두기는 했지만,

그럼에도 어떠한 예기치 못한 상황에 의해 Ball 객체가 생성되었다고 가정한다면
이 경우, 생성된 Ball 객체는 다른 Ball 객체와의 비교 연산을 수행하게 될 것인데
값이 같아도 같지 않도록 한다면, 개발자는 해당 문제를 인지하고 해결할 것이라 생각했습니다.
(그래서 Ball에 equals와 hashcode를 재정의해두지 않았습니다.)

잠재적 위협(의도치 않은 버그)은 사전에 방지해야 한다는 생각을 지니고 있는데,
코니는 이러한 부분과 저의 관점에 대해 어떻게 생각하시는지 궁금합니다!!!

choihz · 2022-03-03T16:11:27Z

src/main/java/lotto/exception/LottoExceptionStatus.java

+import lotto.utils.TicketSize;

-    String getMessage();
+public enum LottoExceptionStatus {


오히려 로또 수동 구매 기능 구현 과정에서는 이 예외메시지는 어느 구현체에 넣어야하는거지? 라는 생각을 가질 정도로 저 스스로 구현체 활용에 대해 혼란을 느끼기도 했습니다.

이렇게 직접 느껴야만 배울 수 있는 것들이 많더라구요 👍 저도 공감이 많이 됩니다.

choihz · 2022-03-03T16:13:04Z

src/main/java/lotto/LottoApplication.java

+import lotto.dto.WinningTicketDto;
+import lotto.view.LottoView;

 public class LottoApplication {


1단계보다 많이 깔끔해지고 명확해진 것 같습니다 👍

choihz · 2022-03-03T16:28:33Z

src/main/java/lotto/view/input/InputView.java

    public int requestMoney() {
-        return parseMoney(reader.readLine());
+        final String inputLine = reader.readLine();
+        return parseNumber(inputLine, LottoExceptionStatus.MONEY_MUST_BE_NUMERIC);


사실 메서드 배치 순서는 일관성을 지키는 게 생각보다 어렵고, 스타일 차이로 볼 수 있는 여지도 많이 있어요. 하지만 결국 클래스에서 무슨 일을 하는지, 어떤 책임을 지고 있는지를 명확히 드러내는 것은 공개된 인터페이스, 즉 public 메서드이기 때문에, 그리고 보통 코드는 위에서 아래로 읽어내려가기 때문에, 후자의 방식이 훨씬 선호되고 저는 전자를 본 기억이 없네요. 게다가 parseNumber() 메서드의 경우 특별한 로직이 있지도, 크게 중요하지도 않은 메서드이구요.

sinb57 · 2022-03-04T11:20:05Z

피드백 감사합니다!!! 지적해주신 사항에 대해서 다시 한번 깊게 고민해보았습니다!

피드백 주신 사항에 대해 답변을 달아놓았으며, 행여 처음에 주신 피드백에 달아놓은 답변을 놓치실까봐
아래의 두 discussion에 대해서는 링크를 표기해두었습니다!!

- 메서드 배치 순서에 대한 답변
https://github.com/woowacourse/java-lotto/pull/461#discussion_r819330459
- enum 사용 스타일에 대한 답변
https://github.com/woowacourse/java-lotto/pull/461#discussion_r819333644

주요 변경 사항은
Tickets의 로또 생성 로직을 TicketBundles에게 넘겨준 것과 Rank의 리패토링입니다!!

귀찮고 힘드실텐데 매번 좋은 피드백을 주셔서
항상 감사함을 느끼고 있습니다!! 이번에도 잘 부탁드리겠습니다~!!!

choihz

안녕하세요, 이프!

남겨주신 코멘트와 새로운 수정사항들을 확인했고, 질문에는 코멘트를 달아 두었습니다. 1단계에서부터 지금까지 충분히 잘 해주셔서 이제 머지해도 될 것 같아요. 그동안 정말 고생 많으셨고, 로또 미션을 함께할 수 있어 즐거웠어요.

그럼, 다음에 다시 만나요~ 👋

choihz · 2022-03-05T10:31:24Z

src/main/java/lotto/utils/Rank.java

@@ -0,0 +1,67 @@
+package lotto.utils;


Rank는 어플리케이션을 관통하는 주요 규칙임과 동시에, 거의 대부분의 도메인과 View에서도 사용되고 있습니다.
공용의 느낌을 강하게 느꼈기 때문에, 특정 도메인에 종속된 패키지가 아닌 상위의 utils 패키지로 이동시켰습니다!!

보통 유틸이라는 단어는, 비즈니스 로직과 직접적인 관계가 없고 범용적으로 사용될 수 있는 도구들을 가리킵니다. 예를 들어 Java의 java.util 패키지를 살펴보면 좋을 것 같아요.

따라서 이러한 측면에서 보면 현재 utils 패키지에 있는 모든 클래스들은 유틸이라고 보기엔 어려운 것 같습니다. 모두 다 중요한 비즈니스 로직을 담고 있으니까요.

그렇네요!! utils 패키지의 모든 클래스를 domain으로 옮기겠습니다!! 감사합니다ㅎ

choihz · 2022-03-05T10:40:10Z

src/main/java/lotto/view/input/InputView.java

    public int requestMoney() {
-        return parseMoney(reader.readLine());
+        final String inputLine = reader.readLine();
+        return parseNumber(inputLine, LottoExceptionStatus.MONEY_MUST_BE_NUMERIC);


메서드의 배치 순서는 코드를 읽기 편하게 하기 위함이라고 생각해요.
때문에 배치 순서 만큼이나 메서드의 결집도 중요하다고 생각하는데요.
위의 예시에 따르면 첫번째 배치순서의 경우, 2,3,4가 군집을 형성하므로
1 메서드가 무슨 역할을 수행하는지 먼저 알고 넘어가면
뒤따라오는 2,3,4를 읽기 수월해진다고 생각했어요.

물론 이는 두번째 배치순서 예시도 마찬가지지만요.
그렇다면 전자와 후자의 차이는 공통적으로 사용되는 메서드를
먼저 알려줄거냐 / 도중에 알려줄거냐의 스타일 차이라고 생각했어요.

이전 코멘트에서 책 『클린 코드』를 언급해 주셔서, 저도 해당 책을 인용해 볼게요. 단, 아시겠지만 이 책에서 제시하는 내용이 상황에 관계 없이 무조건 옳다는 의미는 아닙니다.

코드는 위에서 아래로 이야기처럼 읽혀야 좋다. 한 함수 다음에는 한 함수 다음에는 추상화 수준이 한 단계 낮은 함수가 온다. 즉, 위에서 아래로 프로그램을 읽으면 함수 추상화 수준이 한 번에 한 단계씩 낮아진다. (p.46)

한 함수가 다른 함수를 호출한다면 두 함수는 세로로 가까이 배치한다. 또한 가능하다면 호출하는 함수를 호출되는 함수보다 먼저 배치한다. 그러면 프로그램이 자연스럽게 읽힌다. 규칙을 일관적으로 적용한다면 독자는 방금 호출한 함수가 잠시 후에 정의되리라는 사실을 예측한다. (p.104)

choihz · 2022-03-05T10:45:00Z

src/test/java/lotto/domain/ball/BallStorageTest.java

+    @DisplayName("번호가 동일해도, 캐싱되지 않은 Ball 객체와 일치해선 안된다.")
+    @ParameterizedTest(name = "[{index}] 번호 : {0}")
+    @ValueSource(ints = {1, 2, 44, 45})
+    void ballsNotSameIfNotGeneratedThroughBallStorageTest(final int ballNumber) {
+        final Ball actualBall = BallStorage.getBall(ballNumber);
+        final Ball expectedBall = new Ball(ballNumber);
+        assertThat(actualBall).isNotEqualTo(expectedBall);
+    }


BallStorage는 Ball 객체가 과도하게 생성되는 것을 방지하기 위해 (재사용하기 위해서)
Ball 객체가 캐싱하는 목적으로 생성되었습니다.

따라서 어플리케이션 내의 모든 Ball 객체는 BallStorage를 통해서만 생성되고 관리되어야만
BallStorage의 생성 목적(캐싱)에 부합하다고 생각했습니다.

이를 위해서 Ball 생성자의 접근제어자를 default-package 로 지정함으로써
같은 패키지에 존재하는 BallStorage를 통해서만 Ball 객체가 생성되도록 제한해두기는 했지만,

그럼에도 어떠한 예기치 못한 상황에 의해 Ball 객체가 생성되었다고 가정한다면
이 경우, 생성된 Ball 객체는 다른 Ball 객체와의 비교 연산을 수행하게 될 것인데
값이 같아도 같지 않도록 한다면, 개발자는 해당 문제를 인지하고 해결할 것이라 생각했습니다.
(그래서 Ball에 equals와 hashcode를 재정의해두지 않았습니다.)

잠재적 위협(의도치 않은 버그)은 사전에 방지해야 한다는 생각을 지니고 있는데,
코니는 이러한 부분과 저의 관점에 대해 어떻게 생각하시는지 궁금합니다!!!

캐싱이라는 것은 현재 로또 프로그램의 주요한 비즈니스 로직이라기보다는 사실 있어도 되고 없어도 되는, 코드상으로만 의미가 있는 부가기능이 아닐까요? 지금은 캐싱 로직을 적용했지만 나중에는 뺄 수도 있는 것이고요. 그래서 캐싱을 위해 Ball에 equals()와 hashcode()를 재정의하지 않았다는 것은 굉장히 어색하게 느껴집니다.

sinb57 added 30 commits March 1, 2022 17:41

test: LottoServiceTestProvider 분리

34c8464

refactor: getter 반환 값을 List.copyOf로 변경

03686f3

refactor: TicketsDto 클래스 제거

bfdeb90

test: 테스트 픽스처, Array.asList 에서 List.of 로 변경

b72c8b3

refactor: 예외 메시지 상수명 및 검증 메서드명 변경

f059f0c

refactor: Balls 캐싱 방식 수정

0e52e70

* refactor: Balls, 모든 번호를 미리 캐싱해놓는 방식에서 요청할 때마다 생성하는 방식으로 변경

test: displayName 수정 및 중복 코드 메소드 추출, 테스트 픽스처 추가

a461116

test: InputViewTest 테스트 코드 작성

2bea502

test: test 로직 DI 컨테이너 생성

de52426

test: LottoControllerTest 로또 구입 기능 테스트

75857c8

test: CustomTicketGenerator#initTickets 명칭 수정

80e8423

test: LottoControllerTest 당첨 번호 및 보너스 볼 입력 예외 테스트

eff80c5

test: LottoControllerTest 당첨 통계 확인 테스트

5772205

refactor: Validator 클래스, 인스턴스 제거

76da6ff

test: import 순서 정렬

fd0c1b2

test: BallsTest 번호 캐싱 테스트, 객체 비교 로직을 isSameAs로 수행

9c8a5a4

test: TicketsTest 로또 개수 일치 테스트, 구현된 메서드로 대체

df0e272

test: TicketsTest 당첨 등수 확인 테스트, 불필요한 매개변수 제거

0bf872a

refactor: Delimiter 클래스, 구분자 상수에 대한 메서드 의존성 제거

71a39f5

refactor: BallNumberRange 로또 번호 범위에 대한 상수 이름 변경

57ddcf9

refactor: 매개변수명 및 상수명 등으로부터 credit 명칭 제거

379edb5

test: BallTest, DisplayName 명시적인 이름으로 변경

bd1aee2

refactor: Tickets#calculateRank 매개변수 명시적인 이름으로 변경

98bc5e7

refactor: AnalysisDto#getRankCounts 반환 값 불변 보장

e70ee5a

test: LottoControllerTest, DisplayName 명시적인 이름으로 변경

6ed7a3e

test: MoneyTest, DisplayName 명시적인 이름으로 변경

0ffe049

test: RandomTicketGeneratorTest, DisplayName 명시적인 이름으로 변경

e84f621

test: TicketsTest, DisplayName 명시적인 이름으로 변경

7c1d196

test: TicketTest, DisplayName 명시적인 이름으로 변경

9db42f8

test: AnalysisDtoTest, DisplayName 명시적인 이름으로 변경

97fa8a3

sinb57 added 7 commits March 2, 2022 14:48

test: 수동으로 구매할 로또의 개수에 대한 예외 테스트

6d7fe50

refactor: OutputView#printAnalysisRankCounts 메서드 분리

f2c41e2

refactor: 의도를 명확히 하도록 로직 및 명칭 변경

64f7b33

refactor: 사용하지 않는 메서드 제거

c554aea

refactor: List 컬렉션을 사용하도록 변경

ac49621

refactor: 의도를 명확히 하도록 명칭 변경

823b8ba

refactor: 예외메시지 상수명 오타 수정

be37d36

sinb57 commented Mar 2, 2022

View reviewed changes

sinb57 added 8 commits March 3, 2022 11:25

refactor: Money 클래스, equals 와 hashCode 오버라이딩

c19a056

refactor: Ball 클래스, equals 와 hashCode 오버라이딩

35eef45

test: BallStorage, 캐싱 반환 여부 테스트 추가

3a3fde6

refactor: Ball/Money 클래스, equals 와 hashCode 제거

96610c5

refactor: Rank 패키지 이동 (utils)

18650bf

fix: 수동으로 구매할 로또의 개수로 음수 입력이 허용되는 이슈 해결

f72ee83

refactor: LottoView, TicketCountValidator 클래스 추출

fa01b19

docs: README 업데이트

eaf962d

choihz suggested changes Mar 3, 2022

View reviewed changes

sinb57 added 9 commits March 4, 2022 15:01

style: 코드 컨벤션에 맞춰 수정

0246f33

refactor: 로또 수량 출력 메시지 수정

9c1cb13

refactor: 로또 생성 로직의 책임 이동, TicketManager 명확한 의미 부여

160fcb3

test: BallStorageTest, 중복 테스트 제거 및 동등성 실패 테스트 수정

4bac6ef

refactor: TicketManager 등수결과 concat 로직, stream 활용

abb6f6e

refactor: Ticket 메서드 순서 변경

f6c48ab

refactor: Rank Enum 클래스, 내부 생성 로직 수정

3fbb491

refactor: TicketManager 이름 변경 (TicketBundles)

d22e2e3

refactor: Rank BonusMatched 내부 클래스, Optional 제거

0e32db2

choihz approved these changes Mar 5, 2022

View reviewed changes

choihz merged commit 2b2eac2 into woowacourse:sinb57 Mar 5, 2022

sinb57 mentioned this pull request Mar 7, 2022

[로또 미션] 이프 최종 리뷰 정리 woowacourse-study/2022-back-end-code-review-study#4

Merged

		private final Tickets preparedTickets;
		private final Tickets generatedTickets;

Conversation

sinb57 commented Mar 2, 2022

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!

sinb57 Mar 4, 2022 • 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!

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!

choihz 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinb57 commented Mar 4, 2022

Uh oh!

choihz 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

sinb57 Mar 4, 2022 •

edited

Loading