[그리디] 전서희 로또미션 1,2단계 제출합니다. #92

jeonseohee9 · 2025-03-26T07:42:46Z

안녕하세요! 전서희입니다 !!

1.RankResult 를 map을 쓰지 않고 따로 클래스를 만들어 리스트로 사용하였습니다. 객체지향 관점에서는 RankResult 클래스를 사용하는 것이 나을 것 같아 그대로 사용했으나 코드가 더 복잡해진 것 같다는 느낌이 들어 고민이 됐던 것 같습니다. 저는 RankResult를 유지하는 쪽이 더 나을 것이라 판단했지만 리뷰어님의 의견이 궁금합니다.
2. 클래스 작성 과정에서 중복되는 상수가 있어서 LottoConstants로 따로 분리하였는데 이런식으로 작성해도 괜찮을까요.

전체적으로 클래스 수가 많다고 느끼는데, 지나치게 쪼갠 것은 아닌 지 , 간결하게 만들 수 있는 부분이 있는 지 알려주시면 감사하겠습니다. 이에 대한 조언과 그 외 부족한 부분들 피드백 부탁드립니다!

be-student

안녕하세요 서희님 정말 고생 많으셨어요 🙇

언제나와 비슷하게 늦은 시간에 리뷰를 남겨드릴 것 같아서 미리 죄송하다는 말씀을 드리면서 시작해야 할 것 같네요
요즘 시즌이 일이 바쁘다보니 퇴근이 늦어서 더 딱딱한 말투로 리뷰를 남길 수 있고, 상처가 될 수 있을 것 같은데, 미리 죄송하다는 말씀을 드리고요
원래라면 칭찬도 많이 해드려야되는데, 항상 문제가 되는 것만 잡아내는 것이 익숙해지고, 졸리다보니 쉽지 않네요...

이제부터는 답변을 드릴게요
적어주신 부분이 무슨 뜻인지 알 것 같아요
2가지를 한번에 답변드릴 수 있을 것 같은데요

만약 쪼개는 연습을 하는 과정에서 하는 것이다 라고 하면 아주 잘 쪼개려고 시도해보셨던 것 같아요 👍
반대로 이 코드를 회사에서 짜려고 했다면 ❓ 가 달리겠죠

그래서 제가 어느 관점에서 리뷰를 드리는 것이 좋은지에 대한 고민이 조금 있는데요

전체적으로 구조에서 말씀하신 것처럼 쪼개려고 하다보니 쪼개진 클래스들이 조금 있는 것 같아요
그렇다면 어떻게 진짜 쪼개야 하는지, 합쳐야 하는지를 구분하느냐 라는 따라올텐데요

제가 지금 보는 기준은 다음과 같아요
이 클래스가 단순히 getter, setter, stream, contain 같은 기본 함수만을 제공해주고, 딱히 이 클래스에서 새롭게 비즈니스적으로 작성하는 것이 없다
라고 한다면 저는 이 클래스는 합쳐져야 하고, 다른 클래스를 활용할 수 있는지 고민해보면 좋을 것 같아요
일급 컬렉션이 필요 없다면 그냥 List 나 숫자를 들고다녀도 괜찮을 것 같아요
까지가 회사원으로서의 리뷰이고요

응원하는 입장에서는 열심히 쪼개려고 했던 것은 너무 좋다는 말을 드리고 클래스에 맥락을 조금 더 녹여보면 좋겠다는 생각이 들어요
LottoResult 라는 클래스를 보면 있는 것처럼 이 클래스에서 하는 것이 어울리는 기능들이 분명 다른 클래스들에도 있을텐데, 그 기능들을 열심히 찾아보시면 좋을 것 같아요 그리고 클래스에서 잘 함수로 추가해보면 좋을 것 같아요
가 리뷰어의 입장에서 보는 관점이에요

2가지 관점 중 1가지를 조금 더 진행해보시면 좋을 것 같아요
고생 많으셨어요!
궁금하신 것이 있으시다면 언제든 dm 이나 스레드에 멘션과 함께 남겨주세요
멘션이 아니면 잘 못 보게 되더라고요

be-student · 2025-03-26T16:27:29Z

gradle/wrapper/gradle-wrapper.properties

+distributionUrl=https\://services.gradle.org/distributions/gradle-8.13-bin.zip
+networkTimeout=10000
+validateDistributionUrl=true


이런 파일의 경우에는 commit 에 섞이지 않도록 해주시는 것이 좋을 것 같아요
commit 을 할 때 git add . 같은 형태로 진행하게 되면 이런 문제가 자주 발생할 것 같아서, gui or intellij 를 활용해서 확인할 수 있는 방법을 찾아보셔도 좋을 것 같아요

be-student · 2025-03-26T16:29:19Z

src/main/java/model/Lotto.java

+
+public class Lotto {
+
+    private final List<Integer>  numbers;


https://dev-jhl.tistory.com/entry/IntelliJ-IDEA-%EC%A0%80%EC%9E%A5-%EC%8B%9C-%EC%95%A1%EC%85%98-Actions-on-Save-%EA%B8%B0%EB%8A%A5-%EC%B6%94%EA%B0%80
intellij 의 이런 기능을 활용해서 저장시에 무조건 정렬이 되도록 진행해주시면 좋을 것 같아요

be-student · 2025-03-26T16:34:46Z

src/main/java/view/InputHandler.java

+    private static List<Integer> parseNumbers(String input) {
+        return Arrays.stream(input.split(","))
+                .map(String::trim)
+                .map(Integer::parseInt)
+                .collect(Collectors.toList());
+    }


여기서 문자열을 넣으면 어떻게 될까요?
"a" 라는 것을 넣는다고 하면 아마 Integer.parseInt 에서 에러가 나겠죠?
그렇다면 사용자에게 예상하지 못한 액션이 생길 것 같아요
이런 경우에 integer 로만 이루어졌는지 검사하거나, try catch 로 검사해서 우리가 알 수 있는 예외로 변환해주는 작업을 해주면 좋을 것 같아요

be-student · 2025-03-26T16:39:49Z

src/main/java/model/Money.java

+    public int getTicketCount() {
+        return amount / PRICE_PER_LOTTO;
+    }
+
+    public double calculateProfitRate(int prize) {
+        return (double) prize / amount;
+    }


객체지향의 장점이 한 클래스만 보고서 충분히 이해가 가도록 하는 것이 목표인 경우가 많은데요
Money 클래스를 보는데, 이때 ticket 의 맥락이 섞이거나 비율을 구하는 것은 약간 다른 느낌인 것 같아요

만약 계산해야한다면?
calculateProfitRate-> calculateRate
getTicketCount -> divideByThousand 와 같은 조금 더 general 한 네이밍이 되면 좋을 것 같은데 어떠신가요

넵 더 명확한 것 같습니다!

be-student · 2025-03-26T16:42:08Z

src/main/java/model/Lottos.java

+import java.util.List;
+import java.util.stream.Stream;
+
+public class Lottos {


저는 이런 경우에는 일급 컬렉션을 만들면 안된다고 생각해요
일급 컬렉션 = 단순히 List 를 래핑하는 것을 제외한 도메인에 해당하는 것들이 필요하다고 생각해요
예를 들어서 LottoResult 를 계산해준다던가...? 와 같은 그런 작업들을 해주는 것이 없다면 그냥 List 로 들고다녀도 된다고 보는 편입니다!

나중에 기능이 추가될 시 필요할 수도 있지 않을까 라고 생각해서 미리 감싸두는 형태로 설계했습니다. 다만, 리뷰어님의 말씀대로 단순히 로또를 감싸고 있는 역할만 하기 때문에 현재는 과한 것 같다는 생각이 듭니다!

일급 컬렉션을 무조건 만들어야 한다는 요구사항을 지키기 위해서 역할을 부여하시는 것에 성공하셨네요 👍
고생 많으셨습니다

be-student · 2025-03-26T16:47:43Z

src/main/java/model/Rank.java

+    public static Rank findByMatchCount(int matchCount) {
+        return Arrays.stream(values())
+                .filter(rank -> rank.matchCount == matchCount)
+                .findFirst()
+                .orElseThrow(() -> new IllegalArgumentException("matchCount에 해당하는 Rank가 없습니다: " + matchCount));
+    }


이 부분도 만약 성능을 생각한다면, map<MatchCount, Rank> 형태로 미리 static 변수에 할당해두고, 단순히 map.get으로 데이터를 넣게 된다면 성능상 이점이 꽤 클 것 같아요

be-student · 2025-03-26T16:51:38Z

src/main/java/model/WinningNumbers.java

+public class WinningNumbers {
+
+    private final Lotto winningNumbers;
+
+    public WinningNumbers(List<Integer> numbers) {
+        this.winningNumbers = new Lotto(numbers);
+    }
+
+    public boolean contains(int number) {
+        return winningNumbers.contains(number);
+    }
+
+}


저는 이 클래스도 그냥 Lotto 만을 들고 다니는 것과의 차이를 잘 모르겠어요
이런 경우에 물론 네이밍을 통해서 조금 더 직관적인 것을 주는 것은 좋지만, 클래스가 늘어난다는 것은 관리 포인트가 늘어난다는 관점에서 보면 조금 더 간단하게 진행해도 좋을 것 같아요
그리고 WinningNumbers 가 Lotto 를 가지고 있는 것이 좋은 방향일까요?
추첨 방식에 진짜 로또 방식인 bonus number 가 생긴다면 Lotto 와 winningNumbers 를 들고 있을까요? 만약 당첨 숫자가 7개가 된다면 이때도 Lotto 로 유지할 수 있을까요?
저는 이런 경우를 대비해서 서로 다른 도메인의 경우에는 그냥 서로 다른 클래스 or List 만을 들고다녀도 좋을 것 같아요

WinningNumbers가 Lotto에 너무 의존적이고 변경에 있어 유연하지 않은 것 같네요..또, 두 클래스 간 경계 역시 모호한 것 같습니다.
Lotto에서 사용자가 구매한 번호만 관리하고 WinningNumbers에서는 List 형태로 당첨번호의 책임만 갖게하거나 WinningNumbers를 삭제하는 방향으로 수정해보겠습니다!
감사합니다

be-student · 2025-03-26T16:52:59Z

src/main/java/view/OutputHandler.java

+            System.out.println(rank.getMatchCount() + "개 일치 (" + rank.getPrize() + "원)- " + count + "개");
+        }
+
+        int totalPrize = lottoResult.getTotalPrize();


돈의 경우에는 long 으로 항상 다루는 습관을 가져도 좋을 것 같아요!
로또의 경우에 21억은 그냥 넘어갈 수 있다보니 오버가 되면 진짜 슬프거든요...

be-student · 2025-03-26T16:53:18Z

src/main/java/view/InputHandler.java

+
+    public static Money inputMoney() {
+        System.out.println("구입금액을 입력해 주세요.");
+        int amount = scanner.nextInt();


여기 금액쪽도 long 으로 받아주면 좋을 것 같아요!

be-student · 2025-03-26T16:54:28Z

src/main/java/model/RandomLottoGenerator.java

+        List<Integer> allNumbers = new ArrayList<>();
+        for (int i = LOTTO_MIN; i <= LOTTO_MAX; i++) {
+            allNumbers.add(i);
+        }


요 앞부분까지는 항상 공통적으로 생성될 것 같아서 static 변수로 옮겨도 좋을 것 같아요

jeonseohee9 · 2025-03-28T04:36:46Z

안녕하세요!

주요 변경사항은
WinningNumber클래스 제거
Lottos 에 countRanks 메서드 추가
InputHandler에서 Lotto validate검사 -> Lotto 내부에서 validate검사 변경
RandomLottoGenerator 에서 ALL_NUMBERS static 으로 만듦
RankResult 삭제, RankCounter 추가
LottoResult 삭제
테스트코드 추가
입니다.

rankCounter에서 Lotto에 접근해 matchcount를 얻고 ,rank 계산 로직을 모두 포함하고 있는데 ,별도 클래스를 만들어 이 둘을 분리하는게 나을까요?

또, OutputHandler에서

Map<Rank, Integer> rankCounts = lottoResult.getRankCounter().getRankCounts();
for (Rank rank : Rank.values()) {
int count = rankCounts.get(rank);
}

이 부분을 담당하는 게 맞지 않을 거 같은데 따로 빼주는 게 맞을까요..?

리뷰 감사합니다!

be-student · 2025-03-28T18:12:33Z

안녕하세요 서희님
벌써 2번째 리뷰가 왔다갔다 하게 되었네요

코드에 대해서 고민을 해주신 것이 확실히 보이는 것 같아요 고생 많으셨어요 👍

제가 리뷰를 할 때 이런 것들이 있으면 제 입장에서도 훨씬 더 리뷰를 달기도 쉽기에 한번쯤 말씀을 드리려고 하는데요
주요 변경점을 적어주실 때, 왜 이 변경점이 가장 좋다고 생각했고, 다른 대안들은 어떤 것이 있었고, 그 대안들의 어떤 것들이 마음에 안들었다는 것을 한바퀴 쭉 알려주신다면 서희님의 고민을 더 잘 이해할 수 있을 것 같아요
제 나름대로의 예를 만들어본다면

RandomLottoGenerator 에서 ALL_NUMBERS static 으로 만듦

이라고 했을 때, 기존에는 static 으로 만들지 않았는데, 상수로 분리하면 맥락이 코드상에 더 잘 들어나는 것 같았다. 파일을 나누는 것과 나누지 않는 것을 고민했을 때, 파일을 나누게 되면 상수들만 한번에 볼 수 있다보니 상수에 대한 변경을 더 쉽게 할 수 있어 보였고, 상수들에 대한 관리가 편해보였다. 단점으로는 여러 클래스를 매번 봐야하는 것이 힘들어보였기에 이렇게 진행하게 되었다 2가지중 제 기준에서는 별도 클래스로 보는 것이 더 좋다고 생각하는데, 리뷰어님은 어떻게 생각하시나 궁금하다 or 현업에서는 어떻게 하는지 궁금하다

처럼 진행하거나

요구사항에서 모든 값을 래핑하라고 적혀있는데, 정확하게 어디까지 래핑해야하는지 모르겠다. 로또를 일급 컬렉션으로 만드는 것이 가장 좋은지, 아닌지 모르겠다 등등
이런 느낌으로 고민의 과정을 잘 적어주시면 잘 적어주실수록 저도 서희님도 서로서로에게 좋은 영향이 있을 것 같아서 이 부분은 꼭 부탁드리려고 합니다

그럼 긴 이야기는 끝났고 주신 질문들에 대한 답변부터 달고 진행해보려고 합니다

rankCounter에서 Lotto에 접근해 matchcount를 얻고 ,rank 계산 로직을 모두 포함하고 있는데 ,별도 클래스를 만들어 이 둘을 분리하는게 나을까요?

제 입장에서는 랭크를 계산해주는 일 을 하기 위해서 로또에서 몇개가 일치하는지 확인한다, 일치하는 개수에 따라서 계산을 한다 라는 일을 해주는 것은 자연스러운 것 같아요
물론 분리를 하는 것도 가능하지만 모든 코드의 분리에는 응집도와 결합도를 신경써야 합니다
랭크를 계산해주는 일을 하기 위해서는 저런 작업들이 무조건 필요하고 그 곳에서만 쓰일 것이다보니 응집도의 측면에서 도움이 많이 되겠죠

또, OutputHandler에서

for (Rank rank : Rank.values()) {
int count = rankCounts.get(rank);
}

이 부분을 담당하는 게 맞지 않을 거 같은데 따로 빼주는 게 맞을까요..?

항상 이런 것을 여쭤보실 때는 제가 적어주신 것처럼 1번 안이 어떤 것이고, 2번 안이 어떤 것이고 그것에 따른 장단점이 어떤 것이 있어보였다 라고 그 배경을 설명해주시면 좋을 것 같아요
이 부분만으로는 제가 명확하게 어떤 것을 의도하고 말씀해주시는 것인지를 잘 모르겠어요
만약 view 에서 domain 을 접근하는 것이 맞는지 모르겠다 라면 접근해도 된다 쪽일 것 같고요

앞으로도 잘 부탁드려요 🙇

be-student

일단 2번째 리뷰 요청이다보니 무조건 approve 를 드리긴 하겠지만, 추가적으로 반영을 하시고 dm 으로 요청을 주시면 제가 언제든 추가로 봐드릴테니 요청주세요 🙇

be-student · 2025-03-28T17:19:01Z

src/test/java/RandomLottoGeneratorTest.java

+    @Test
+    void 로또_번호는_범위_내에_있다() {
+        RandomLottoGenerator generator = new RandomLottoGenerator();
+        Lotto lotto = generator.generate(1).get(0);
+        boolean allInRange = lotto.getNumbers().stream()
+                .allMatch(n -> n >= 1 && n <= 45);
+        assertTrue(allInRange);
+    }


제 기준에서는 이런 테스트는 작성하지 않는 편인데요 이 테스트로 라이브러리의 버그를 찾아낼 수 있다면 정말 좋겠지만 랜덤성이 있는 라이브러리에 대한 테스트는 라이브러리를 믿는 편인 것 같아요

be-student · 2025-03-28T17:23:06Z

src/main/java/model/Lotto.java

+    }
+
+    public List<Integer> getNumbers() {
+        return Collections.unmodifiableList(numbers);


복사 신경써서 해주시는 것 좋네요 👍

be-student · 2025-03-28T17:28:19Z

src/main/java/view/OutputHandler.java

+        System.out.println("당첨 통계\n---------");
+
+        Map<Rank, Integer> rankCounts = counter.getRankCounts();
+        for (Rank rank : Rank.values()) {


보통 enum.values() 를 사용하는 경우는 모든 enum 을 map 형태로 바꿔서 빠르게 접근하도록 하거나, EnumSet, EnumMap 과 같은 자료구조를 사용하거나, enum 의 모든 것을 그냥 순서에 대해서 신경쓰지 않고 가져올 때만 사용하는 것 같아요
enum 의 순서는 언제든 바뀔 수 있기에, 이렇게 두는 것 보다는 무엇인가 다른 필드 or 순서를 직접 지정해주시는 것이 좋습니다

be-student · 2025-03-28T17:29:23Z

src/main/java/view/InputHandler.java

+        while (true) {
+            try {
+                System.out.println("구입금액을 입력해 주세요.");
+                long amount = Long.parseLong(scanner.nextLine().trim());
+                return new Money(amount);
+            } catch (NumberFormatException e) {
+                System.out.println("숫자만 입력해주세요.");
+            } catch (IllegalArgumentException e) {
+                System.out.println(e.getMessage());
+            }
+        }


indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.
예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메서드)를 분리하면 된다.

저희 요구 사항에 이런 것이 있는데요
혹시 이런 것을 지켜서 한번 진행해주실 수 있으실까요?

be-student · 2025-03-28T17:30:51Z

src/main/java/model/Lottos.java

+import java.util.List;
+import java.util.stream.Stream;
+
+public class Lottos {


일급 컬렉션을 무조건 만들어야 한다는 요구사항을 지키기 위해서 역할을 부여하시는 것에 성공하셨네요 👍
고생 많으셨습니다

be-student · 2025-03-28T17:41:26Z

src/main/java/model/Lotto.java

+
+public class Lotto {
+
+    private final List<Integer> numbers;


로또 번호를 특정 객체로 포장해보면 어떨까요? 예를 들어, List numbers; 처럼요.
로또 번호에 대한 검증 로직도 그쪽에 추가되면 좋을 것 같아요. ex)로또 번호는 1~45 사이의 정수이다.

be-student · 2025-03-28T17:44:05Z

src/main/java/model/Rank.java

+    static {
+        for (Rank rank : values()) {
+            matchCountToRankMap.put(rank.matchCount, rank);
+        }
+    }


적용해주신 것 아주 좋네요 👍

be-student · 2025-03-28T17:57:56Z

src/main/java/model/RankCounter.java

+        if (!Rank.isValidMatchCount(matchCount)) {
+            return;
+        }


저는 보통 이런 케이스에서 그냥 return 을 하면 안되고, 예외를 발생시켜야 된다고 보는 편인데요
예외를 발생시키지 않고, 그냥 진행하게 되면 나중에 데이터가 꼭 예상과 달라지다보니 그때가서 찾기가 어렵더라고요

0~2개일경우는 그냥 return을 사용해서 통계 계산을 하지 않는 편이 나을 것 같아 위와 같은 방법으로 처리하였습니다!

be-student · 2025-03-28T17:58:55Z

src/main/java/view/InputHandler.java

+
+    private static final Scanner scanner = new Scanner(System.in);
+
+    public static Money inputMoney() {


InputView, OutputView 메서드들이 전부 static 키워드를 사용하더라구요
static을 사용하게된 서희님의 생각/배경이 궁금하네여

사용자 입출력을 담당하기만 하고, 내부에서 상태를 저장해서 관리할 필요는 없다고 생각했습니다!

그러면 이렇게 사용하는 것은 어떻게 생각하시나요?

controller 에서 inputHandler 를 new 를 통해서 사용하고,
input handler 의 scanner 를 private final Scanner scanner = new Scanner(System.in) 을 통해서도 사용할 수 있는데요

단점으로는 생성자에 InputHandler 가 들어가긴 부분이 있지만, 장점으로는 의존성을 명확하게 드러낸다는 것이 장점이긴 합니다
https://fopman.tistory.com/161
여기에 있는 숨은 의존성과 명시적인 의존성쪽을 보셔도 좋을 것 같아요

Input,OutputHandler는 단순히 입출력용이기 때문에 객체를 별도로 만들고 안만들고에 따라 유의미한 차이가 있는 줄 몰랐습니다..
말씀해주신 대로 의존성을 명시적으로 드러내는 방식이 더 명확한 것 같습니다.
다음 수정 시 리팩토링해보겠습니다!

be-student · 2025-03-28T18:00:45Z

src/test/java/LottosTest.java

+    @Test
+    void 로또_모음이_당첨과_일치하는_개수_정확히_카운트된다() {
+        Lottos lottos = new Lottos(List.of(
+                new Lotto(List.of(1, 2, 3, 4, 5, 6)),
+                new Lotto(List.of(1, 2, 3, 10, 11, 12)),
+                new Lotto(List.of(40, 41, 42, 43, 44, 45))
+        ));
+        Lotto winning = new Lotto(List.of(1, 2, 3, 4, 5, 6));
+        RankCounter counter = lottos.countRanks(winning);
+
+        assertEquals(1, counter.getRankCounts().get(Rank.SIX_MATCH));
+        assertEquals(1, counter.getRankCounts().get(Rank.THREE_MATCH));
+    }


이 테스트를 통해서 모든 일치 케이스를 잘 찾아낸다고 확신할 수 있을까요?
4,5 개에 대한 테스트도 있으면 좋을 것 같아요
만약 만든다면 중복이 많아지다보니 ParameterizedTest 에 대해서 학습해보셔도 좋을 것 같고요

jeonseohee9 · 2025-03-30T06:19:56Z

안녕하세요!
제가 저의 고민에 대해 글로 풀어쓰는 게 익숙하지 않다보니 너무 생략되어 작성되었던 것 같습니다..
앞으로 최대한 제 고민의 흐름을 자세하게 풀어써보겠습니다! 코멘트 감사합니다!

위 2번째 질문에 대해 좀 덧붙이자면.. View가 Rank의 내부 구조를 알고 순회하는 방식은 책임의 분리 관점에서 결합이 높지 않나 생각했습니다. 이를 보완하기 위해 RankResult처럼 출력용 DTO를 따로 만들어, Domain은 View에 필요한 형식의 데이터를 전달하고 View는 그걸 단순히 순회해 출력하는 구조가 나을 것 같기도 하였으나, RankCounter를 방어적복사로 전달하고, 출력에 필요한 정보를 담고있기 때문에 그대로 사용하는 것이 효울적인 것 같아 따로 출력용 dto를 만들진 않았습니다. 다만, view에서 직접 map을 다루는 구조가 어색하여 질문을 드렸습니다!

+)RankCounter 내부에서 Lotto와의 매칭 개수를 세고, 그 결과로 Rank를 누적해서 판단하는 로직까지 모두 수행하고 있어서, 이 책임을 분리할 수 있을 지 ,예를 들어 MatchResult 같은 객체를 따로 만드는 것이 어떨 지 고민해보았습니다.
하지만 지금 구조가 응집도 측면에서는 더 낫다고 판단해서 그대로 유지했으나 추후 요구사항이 더 많아지면 번거롭고 확장성 측면에서는 좋지 않은 것 같아 고민이 됩니다.

추가적으로.. LottoConstants같은 경우도 여러 클래스에서 중복되었던 상수들이 있어서 새 클래스로 묶었고,훨씬 깔끔해보이긴 하였으나
코드를 짜면서 상수이름을 까먹거나 해서 contstants 클래스를 참조해보는 등의 귀찮음이 생기긴하더라고요.. 그러나 한번에 관리할 수 있는 장점이 있어 분리했습니다. 이럴 경우 리뷰어님은 보통 어떤 식으로 하시는 지 궁금합니다

be-student

자연스럽게 질문중 1개는 코멘트로 남겼습니다!
상수쪽을 기준으로 봤을 때, 제 경험상으로 상수를 한 클래스에 모아두게 되면, 그 클래스는 결국 너무 많은 상수가 생겨서 어디에 뭐가 있는지 모르다보니 그 상수가 중복으로 관리되기 시작하고, 나중에는 아 이럴거면 그냥 클래스에 만들껄 이라는 생각이 들고는 합니다.
그래서 저는 어지간하면 그 상수가 어디에 그나마 어울릴까를 보고, 거기서 public static 의 형태로 열어두는 것을 선호합니다!
긴 리뷰 고생 많으셨어요!
당연하지만 또 요청주시면 봐드릴게요!

be-student · 2025-03-30T07:45:26Z

src/main/java/controller/LottoStore.java

+
+        OutputHandler.printPurchaseResult(lottos.getLottos());
+
+        Lotto winningLotto = InputHandler.inputWinningLotto();


제가 확장성을 볼 때는 이것도 한 가지 기준인데요
서로 다른 변경 사유로 변경될 수 있는 것을 지금 당장 비슷하다고 하는 이유로 동일하게 사용하고 있는지
입니다
이 케이스에서는 당첨 번호와 로또 1개의 경우에 서로 다른 변경 사유가 있을 수 있어요
로또는 6자리자만, 당첨 번호는 7개가 되는 것도 충분히 가능하고, 그 반대도 가능하죠
이런 경우에 변경을 한다면 Lotto 를 참조하는 모든 코드를 다 뒤져서 어떤 경우에 변경이 되어야 하는지 확인을 해야 합니다.
이런 경우에는 확장성을 고려한다면 같은 Lotto가 아닌 다른 클래스로 분리해주는 것이 좋습니다

be-student · 2025-03-30T07:55:03Z

src/main/java/model/Money.java

+    public int divideByThousand() {
+        return (int) (amount / PRICE_PER_LOTTO);
+    }


이런 함수의 경우에 보통
public int divideBy(int divisor){
if(amount % divisor!=0) throw
amount/divisor
}
과 같은 형태로 짜두는 것이 함수의 개수가 줄어드는 측면에서 메리트가 있을 것 같아요
추가로 돈에 로또에 대한 컨텍스트가 생기는 것은 저는 별로 좋지 않은 것 같아요

돈과 로또의 관계가 사실 거의 없고, 돈의 금액 숫자값으로 그냥 로또를 사는 느낌이다보니, 그 금액 제한과 관련된 것은 돈이 아닌 다른 쪽에 있어야 할 것 같은데 어떻게 생각하시나요?

돈 객체가 계산까지 포함하는 것이 자연스러울 것 같다고 생각했는데,
돈 자체는 로또라는 도메인을 모르는 것이 더 알맞은 것 같습니다.
divideBy(int divisor) 형태로 일반화하거나, 로또 생성 쪽으로 책임을 옮기는 것도 고려해보겠습니다!

be-student · 2025-03-30T08:03:36Z

src/main/java/model/Lottos.java

+    public RankCounter countRanks(Lotto winningLotto) {
+        RankCounter counter = new RankCounter();
+        for (Lotto lotto : lottos) {
+            counter.count(lotto, winningLotto);
+        }
+        return counter;
+    }


Lottos 가 RankCounter 를 참조하는 것보다는 반대로 RankCounter -> Lottos 를 참조하는 관계가 좋지 않을까요?
getter 를 사용하지 않기 위해서 이렇게 해주신 것 같지만, 일반적인 상황을 고려해보면,
로또 결과를 계산해주기 위해서 당첨 번호와 로또들을 받아서 계산한다
쪽이
로또들이 로또 결과를 계산해주기 위해서 당첨 번호를 받아서 계산한다 보다 더 직관적인 것 같아요
보통 이렇게 문장으로 만들면 어떤 것이 어떤 것을 받아야되는지 이해하기 쉬운 것 같아요

be-student · 2025-03-30T08:05:37Z

src/main/java/model/LottoGenerator.java

+public interface LottoGenerator {
+    List<Lotto> generate(int count);
+}


혹시 이렇게 인터페이스를 분리한 이유가 있을까요?
현재 상황에서는 구현체가 1건밖에 없는데, 이런 경우에는 없애는 쪽이 좋을 것 같아요
만약 랜덤을 테스트하기 위해서라면, 이를 상속해서 처리하는 구현체를 만들어서 직접 테스트를 만들어봐도 좋을 것 같아요

저번 자동차 미션에서 배웠던 전략 패턴을 이용해 테스트했던 것이 생각나서 이번에도 똑같은 방식으로 코드를 작성했었습니다. 그러나 이번에는 LottoGenerator를 주입 받아 사용하는 클래스가 없기에 고정된 값을 반환할 generator가 굳이 필요 없는 것 같네요..

be-student · 2025-03-30T08:09:00Z

src/test/java/LottosTest.java

+public class LottosTest {
+
+    @ParameterizedTest
+    @CsvSource({


👍 고생 많으셨어요

be-student · 2025-03-30T08:19:43Z

src/main/java/view/InputHandler.java

+
+    private static final Scanner scanner = new Scanner(System.in);
+
+    public static Money inputMoney() {


그러면 이렇게 사용하는 것은 어떻게 생각하시나요?

controller 에서 inputHandler 를 new 를 통해서 사용하고,
input handler 의 scanner 를 private final Scanner scanner = new Scanner(System.in) 을 통해서도 사용할 수 있는데요

단점으로는 생성자에 InputHandler 가 들어가긴 부분이 있지만, 장점으로는 의존성을 명확하게 드러낸다는 것이 장점이긴 합니다
https://fopman.tistory.com/161
여기에 있는 숨은 의존성과 명시적인 의존성쪽을 보셔도 좋을 것 같아요

be-student · 2025-03-30T08:26:18Z

src/main/java/model/RankCounter.java

+
+public class RankCounter {
+
+    private final Map<Rank, Integer> rankCounts = new EnumMap<>(Rank.class);


RankCounter 와 MatchResult 쪽에 대한 고민을 하시는 것 같아서, 제 생각을 적어드리면요
저는 RankCounter 같은 Counter 클래스는 유틸리티클래스로 만들어도 좋다고 생각합니다
물론 객체지향적인가? 에 대해서는 잘 모르겠지만요

유틸리티클래스로 만들었을 때의 장점으로는 당연하지만 RankCounter 를 아무렇게나 사용해도 된다는 것이 있겠죠
제가 위쪽에서 적어드린 것처럼 RankCounter 에서 Lottos 를 받아서 계산해주는 로직을 둔다고 하면, 여기에서 계산 결과를 계산해서 MatchResult 를 만들고, 이를 View 쪽으로 전달만 해주는 구조가 될 수도 있겠죠

당연히 정답은 아니지만, 이런 구조도 가능하기는 해요
어떤 구조든 만들어보시고 그것에 따른 장단점을 보는 것이 중요하다고 생각합니다

제 생각으로는 View 에서 Map 을 다루는 것도 충분히 가능하다고 생각합니다
대부분의 경우에 view 쪽은 프론트엔드에서 담당하게 될텐데, 프론트엔드 쪽에서는 결국 json 의 map 형태를 파싱해서 처리하게 될테니까요

be-student · 2025-03-30T08:35:35Z

src/main/java/view/InputHandler.java

+            List<LottoNumber> numbers = Arrays.stream(input.split(","))
+                    .map(String::trim)
+                    .map(Integer::parseInt)
+                    .map(LottoNumber::new)
+                    .collect(Collectors.toList());


약간 성능적인 코멘트를 달아보자면
map 은 1개만 쓰는 것이 가장 성능상으로 좋습니다!
같은 연산이라고 해도 map 을 여러번 하게 되면 stream 을 매번 만들다보니 그만큼 메모리도 사용되게 되고요

jeonseohee9 added 4 commits March 26, 2025 13:00

1차 커밋

2711eaa

2차: 리드미 추가

20249bd

리팩터링: LottoSeller 제거

b8d8686

리팩터링

8f3b9c8

be-student suggested changes Mar 26, 2025

View reviewed changes

jeonseohee9 added 2 commits March 28, 2025 12:53

리뷰반영

f30bc61

+테스트코드작성

46025a8

be-student approved these changes Mar 28, 2025

View reviewed changes

jeonseohee9 added 3 commits March 30, 2025 14:06

리뷰반영3

7451399

리뷰반영3

3422e12

리뷰반영3

3e587d7

jeonseohee9 force-pushed the main branch from e938918 to 3e587d7 Compare March 30, 2025 06:13

be-student approved these changes Mar 30, 2025

View reviewed changes

boorownie merged commit 14ffa34 into next-step:jeonseohee9 Mar 31, 2025


		private static final Scanner scanner = new Scanner(System.in);

		public static Money inputMoney() {


		OutputHandler.printPurchaseResult(lottos.getLottos());

		Lotto winningLotto = InputHandler.inputWinningLotto();


		public class RankCounter {

		private final Map<Rank, Integer> rankCounts = new EnumMap<>(Rank.class);

[그리디] 전서희 로또미션 1,2단계 제출합니다. #92

[그리디] 전서희 로또미션 1,2단계 제출합니다. #92

Uh oh!

Conversation

jeonseohee9 commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

be-student 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!

jeonseohee9 Mar 27, 2025 • 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!

jeonseohee9 Mar 27, 2025 • 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!

jeonseohee9 commented Mar 28, 2025

Uh oh!

be-student commented Mar 28, 2025

Uh oh!

be-student 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!

jeonseohee9 commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeonseohee9 commented Mar 26, 2025 •

edited

Loading

jeonseohee9 Mar 27, 2025 •

edited

Loading

jeonseohee9 Mar 27, 2025 •

edited

Loading

jeonseohee9 commented Mar 30, 2025 •

edited

Loading

be-student Mar 30, 2025 •

edited

Loading