[LBP] 현정빈 로또 미션 2-5 단계 제출합니다. #90

JeongBeanHyun · 2025-03-03T09:07:55Z

리뷰어: 김우진님
리뷰이: 현정빈

3주차 리뷰

우진 리뷰어님 안녕하세요. 로또 미션 제출합니다!
2-5 단계 미션을 수행하면서 어려움이 많았습니다...!
특히, 코드 간의 흐름을 파악하는 데 시간이 많이 걸렸습니다. 부족한 부분이 많은 것 같습니다.. 리팩토링 과정에서 보완하겠습니다..!
이번 구현에서 SRP를 잘 준수하였는지, MVC 패턴에 알맞게 작성했는지 궁금합니다.
현재 컨트롤러에서 많은 로직을 처리하고 있는데, 서비스 클래스를 만들어 책임을 분리하는 것이 적절한 방법인지 고민됩니다.

질문

테스트 코드에서 적절하게 테스트를 작성한 것인지 궁금합니다. 예외처리에서 추가할 부분이 있을까요?
SRP 를 잘 준수하였는지 궁금합니다..!
서비스 클래스를 추가할 경우, 예외처리는 서비스에서 하는 것이 적절한지 혹은 컨트롤러에서 하는 것이 적절한지 고민됩니다. 우진 리뷰어님께서는 무엇이 더 적절하다고 생각하시나요?

기능 설명

Controller

LottoController : 사용자 입력을 받아서 로또 티켓을 생성하고, 당첨 결과를 출력합니다.

Model

Lotto : 로또 한 장(6개의 숫자) 정보를 저장합니다.
LottoNumbers : 로또 번호에 대한 검증 로직을 포함합니다.
LottoResult : 로또 당첨 결과를 계산합니다.
LottoTickets : 사용자가 구매한 로또 티켓 여러 장을 관리합니다.
Money : 로또 구매에 사용되는 금액을 관리합니다.
Rank : 로또 당첨 등수를 나타내는 열거형(Enum) 클래스입니다.
WinningNumbers : 당첨 번호와 보너스 번호를 저장합니다.

View

ErrorView : 에러 메시지를 출력합니다.
InputView : 사용자로부터 입력을 받습니다.
ResultView : 로또 결과를 출력합니다.

woogym

로또 미션 수고했어요 정빈! 🫠🫠

생각보다 구현 난이도가 어렵죠? 저도 처음에 할때 엄청 힘들어 했던 기억이 있어요ㅠㅠ
그래도 잘 수행해주신 것 같아서 다행입니다

코멘트에 남겨두었으니 확인 부탁해요!

Q&A

테스트 코드에서 적절하게 테스트를 작성한 것인지 궁금합니다. 예외처리에서 추가할 부분이 있을까요?

정빈이가 재대로 요구사항을 재대로 이해했다면 예외처리는 잘 수행되지 않았을까 싶어요
테스트 코드는 개선이 필요해보이는 부분들이 몇몇 보이긴 했어요
@BeforeEach, @ParmeterizedTest, @MethodSource 학습 키워드로 드릴게요
공부해봅시다

SRP 를 잘 준수하였는지 궁금합니다..!

전반적으로 SRP를 잘 준수하고자 노력한 모습이 보였어요
하지만 메서드 바디 라인이 적다고 해서 단일 책임 원칙이 지켜진 것은 아니에요
코멘트에 남겨두어서 확인할 수 있겠지만 메서드로 묶었다고 하지만 결국 서로 다른 2가지의 동작을 하는 메서드들도 보였어요 좀 더 개선할 수 있을 것 같아요! 화이팅입니다!

서비스 클래스를 추가할 경우, 예외처리는 서비스에서 하는 것이 적절한지 혹은 컨트롤러에서 하는 것이적절한지 고민됩니다. 우진 리뷰어님께서는 무엇이 더 적절하다고 생각하시나요?

예외처리를 어디에 두어야하는지는 개발자들 사이에서도 많이들 의견이 분분한 내용이죠..
서비스 계층, 컨트롤러의 계층 이 2가지의 역할에 대해서 다시 한 번 생각해보면 도움일 될 것 같아요 (모르겠다면 찾아보고 공부해봅시다)
각각의 역할과 책임을 가진 계층인 만큼 정빈이가 정답을 찾아가는 과정에 도움이 될 겁니다
저도 이 부분에 있어서 오래 고민했던 시간이 있었어요, 절대적인 정답은 존재하지 않습니다
상황에 맞게 적절한 방법을 채택해야하죠

비지니스 로직과 일관성 유지를 위한 도메인 유효성 검사
그렇다면 null체크, 데이터 형식 검사와 같은 입력 유효성 검사는 어디서 이루어져야할까?
한 번 고민해보고 정빈이만의 답변을 알려주세요! 꼭 고민해봤으면 좋겠어요 (GPT 사용하지 않고요!)

woogym · 2025-03-05T16:11:50Z

src/main/java/model/LottoNumbers.java

+        }
+    }
+
+    private List<Integer> createLottoNumbers() {


로또 번호를 생성한다? 보다는 더 자세하게 네이밍 할 수 있을 것 같아요

자동 로또 번호를 생성하는 메서드이기 때문에 generateAutoLottoNumbers로 수정하였습니다. 네이밍에 더욱 신경써서 작성해야할 것 같습니다..! 피드백 감사합니다!

woogym · 2025-03-05T16:18:34Z

src/main/java/model/LottoNumbers.java

+    private List<Integer> numbers;
+
+    public LottoNumbers() {
+        this.numbers = createLottoNumbers();
+    }
+
+    public LottoNumbers(List<Integer> numbers) {
+        validate(numbers);
+        this.numbers = List.copyOf(numbers);
+    }


불변 컬렉션 사용을 통한 방어적 복사가 미흡해보여요

현재 List<Integer> numbers는 가변 리스트로서 방어적 복사가 불완전해보여요
List.copyOf()를 통해서 외부에 불변 리스트를 반환하지만, 내부적으로는 가변의 상태를 유지하고 있어요
추가적으로 List.copyOf()를 통해서 불변 리스트로 반환하지만 getSortedNumbers()를 통해서 정렬된 리스트를 새로 만들어주고 있어요 -> 불필요한 리스트 복사와 정렬이 매번 발생하게 되는 구조

개선해봅시다!

네 피드백 감사합니다!
1.가변 리스트 문제

내부 리스트 (numbers)가 가변 상태로 유지되는 문제를 해결하기 위해 final 키워드를 추가하고, 리스트를 List.copyOf()를 통해 한 번만 할당하여 변경할 수 없도록 했습니다.

2.getSortedNumbers()에서 매번 정렬 리스트를 새로 만드는 문제

정렬된 리스트를 한 번만 저장하고, 이후에는 불필요한 정렬 연산이 발생하지 않도록 했습니다.

정렬 로직을 sortedNumbers() 메서드로 분리하여 객체 생성 시 한 번만 정렬하도록 했습니다.

private List<Integer> sortedNumbers(List<Integer> numbers) { return List.copyOf(numbers.stream().sorted().toList()); } public List<Integer> getNumbers() { return numbers; }

woogym · 2025-03-05T16:21:03Z

src/main/java/model/LottoNumbers.java

+    }
+
+    private List<Integer> createLottoNumbers() {
+        List<Integer> shuffledNumbers = new ArrayList<>(LOTTO_NUMBER_POOL);


매번 new ArrayList<>(LOTTO_NUMBER_POOL)를 호출하지 않고 IntStream을 사용하여 바로 새로운 리스트를 생성할 수 있을 것 같아요

네 LOTTO_NUMBER_POOL 변수를 삭제하고, 우진 리뷰어님께서 추천해주신 IntStream을 사용하여 리스트를 생성하도록 수정하였습니다!

List<Integer> shuffledNumbers = IntStream.rangeClosed(LOTTO_MIN_NUMBER, LOTTO_MAX_NUMBER) .boxed() .collect(Collectors.toList());

woogym · 2025-03-05T16:25:02Z

src/main/java/model/Lotto.java

+    public List<Integer> getSortedNumbers() {
+        return lottoNumbers.getSortedNumbers();
+    }


네이밍과 해당 메서드의 동작이 다른 것 같아요
또한 디미터의 법칙에서는 "내부의 내부에 접근하지 말라"를 제안하고 있어요 해당 코드는 이를 위반하고 있는 것 같아요
클라이언트가 Lotto 클래스의 내부 구조도 알게 되어 결합도도 높아보여요

정빈이는 어떻게 생각해요?

기존의 getSortedNumbers()는 정렬된 번호를 반환하는 것으로 해석될 수 있습니다. 그러나 실제로는 LottoNumbers 의 메서드를 그대로 호출하는 구조였습니다. 그래서 메서드명과 실제 동작이 일치하지 않았습니다.

메서드명을 getNumbers()로 변경하여 로또번호를 가져온다는 의미를 명확하도록 하였습니다.

디미터의 법칙을 이해하는 데 어려웠습니다.. 제가 이해한 바로는 ‘내가 소유한 객체의 내부 로직을 알아서는 안된다.’였습니다. 더 조사해보니 소유한 객체의 메서드는 호출해도 괜찮으나 내부 로직까지 신경써야 하는 경우면 디미터의 법칙을 위반한다는 것을 깨닫게 되었습니다! 그래서 현재 코드에서는 getSortedNumbers() 메서드가 로또번호를 보여줄 뿐만 아니라 정렬해주는 역할까지 담당하고 있었기 때문에 내부 로직이 노출된다는 것을 알게되었습니다. 그래서 LottoNumbers 에서 정렬하는 sortNumbers() 메서드와 로또 번호를 조회하는 getNumbers() 메서드로 분류하였습니다. 그리고 로또번호 조회만 가능한 getNumbers()를 사용하면 디미터의 법칙을 위반하지 않는 것 같다고 판단하였습니다…! 아래와 같이 수정하였는데, 제가 알맞게 이해하고 리팩토링한 것인지 궁금합니다..!

public List<Integer> getNumbers() { return lottoNumbers.getNumbers(); }

woogym · 2025-03-05T16:30:49Z

src/main/java/model/LottoResult.java

+    private Rank determineRank(Lotto ticket, WinningNumbers winningNumbers) {
+        List<Integer> ticketNumbers = ticket.getSortedNumbers();
+        int matchCount = (int) ticketNumbers.stream()
+                .filter(winningNumbers.getNumbers()::contains)
+                .count();
+        boolean hasBonus = ticketNumbers.contains(winningNumbers.getBonusNumber());
+
+        if (matchCount == 6) return Rank.FIRST;
+        if (matchCount == 5 && hasBonus) return Rank.SECOND;
+        if (matchCount == 5) return Rank.THIRD;
+        if (matchCount == 4) return Rank.FOURTH;
+        if (matchCount == 3) return Rank.FIFTH;
+        return Rank.NONE;
+    }


요구사항 - "하나의 메서드는 10줄을 넘지 않는다"를 충족하고 있지 못한 것 같아요 수정해봅시다

요구사항에 맞게 메서드 분리를 통해 10줄을 넘지 않도록 수정하였습니다. 피드백 감사합니다!

private Rank determineRank(Lotto ticket, WinningNumbers winningNumbers) { int matchCount = countMatchingNumbers(ticket, winningNumbers); boolean hasBonus = hasBonusNumber(ticket, winningNumbers); return getRank(matchCount, hasBonus); } // 일치하는 숫자 개수 계산 private int countMatchingNumbers(Lotto ticket, WinningNumbers winningNumbers) { return (int) ticket.getNumbers().stream() .filter(winningNumbers.getNumbers()::contains) .count(); } // 보너스 번호 포함 여부 확인 private boolean hasBonusNumber(Lotto ticket, WinningNumbers winningNumbers) { return ticket.getNumbers().contains(winningNumbers.getBonusNumber()); } private Rank getRank(int matchCount, boolean hasBonus) { if (matchCount == 6) return Rank.FIRST; if (matchCount == 5 && hasBonus) return Rank.SECOND; if (matchCount == 5) return Rank.THIRD; if (matchCount == 4) return Rank.FOURTH; if (matchCount == 3) return Rank.FIFTH; return Rank.NONE; }

woogym · 2025-03-05T16:55:38Z

src/main/java/model/WinningNumbers.java

+    private void validate(List<Integer> numbers, int bonusNumber) {
+        if (numbers.size() != SIZE) {
+            throw new IllegalArgumentException("당첨 번호는 6개의 숫자로 입력해야 합니다.");
+        }
+        if (numbers.stream().distinct().count() != SIZE) {
+            throw new IllegalArgumentException("중복된 숫자가 있습니다.");
+        }
+        if (numbers.stream().anyMatch(n -> n < MIN_NUMBER || n > MAX_NUMBER)) {
+            throw new IllegalArgumentException("당첨 번호는 1부터 45 사이여야 합니다.");
+        }
+        if (bonusNumber < MIN_NUMBER || bonusNumber > MAX_NUMBER) {
+            throw new IllegalArgumentException("보너스 볼은 1부터 45 사이여야 합니다.");
+        }
+        if (numbers.contains(bonusNumber)) {
+            throw new IllegalArgumentException("보너스 볼은 당첨 번호와 중복될 수 없습니다.");
+        }
+    }


요구사항인 하나의 함수가 10줄을 넘지 않는다를 준수하고 있지 못한 것 같아요 🫠
이와 별개로 만약 검증해야하는 규칙이 더욱더 많아진다면 거대 메서드가 만들어질 것 같은데 어떻게 개선할 수 있을까요?

검증 로직이 추가될 경우 기존 validate() 메서드가 지나치게 커지고, 유지보수가 어려워질 가능성이 있어 각 검증을 개별 메서드로 분리하는 방식으로 개선하였습니다. 검증해야하는 규칙이 많아진다면 단일 책임을 잘 준수하도록 메서드로 각각 분리하여 개선하는 것이 좋은 방향성 같습니다..! 피드백 감사합니다!

public void validate(List<Integer> numbers, int bonusNumber) { validateSize(numbers); validateDuplicates(numbers); validateNumberRange(numbers); validateBonusNumber(bonusNumber); validateBonusNotInWinningNumbers(numbers, bonusNumber); } private void validateSize(List<Integer> numbers) { if (numbers.size() != SIZE) { throw new IllegalArgumentException("당첨 번호는 6개의 숫자로 입력해야 합니다."); } } private void validateDuplicates(List<Integer> numbers) { if (numbers.stream().distinct().count() != SIZE) { throw new IllegalArgumentException("중복된 숫자가 있습니다."); } } private void validateNumberRange(List<Integer> numbers) { if (numbers.stream().anyMatch(n -> n < MIN_NUMBER || n > MAX_NUMBER)) { throw new IllegalArgumentException("당첨 번호는 1부터 45 사이여야 합니다."); } } private void validateBonusNumber(int bonusNumber) { if (bonusNumber < MIN_NUMBER || bonusNumber > MAX_NUMBER) { throw new IllegalArgumentException("보너스 볼은 1부터 45 사이여야 합니다."); } } private void validateBonusNotInWinningNumbers(List<Integer> numbers, int bonusNumber) { if (numbers.contains(bonusNumber)) { throw new IllegalArgumentException("보너스 볼은 당첨 번호와 중복될 수 없습니다."); } }

woogym · 2025-03-05T16:56:51Z

src/main/java/view/ResultView.java

+            System.out.println(ticket.getSortedNumbers());
+        }
+    }
+


불필요한 개행이 보여요

네 불필요한 개행을 미처 확인하지 못한 것 같습니다..! 더 꼼꼼히 확인하겠습니다. 감사합니다!

woogym · 2025-03-05T16:57:56Z

src/main/java/view/ResultView.java

+    private static void printWinningDetails(LottoResult lottoResult) {
+        Map<Rank, Integer> matchCountMap = lottoResult.getMatchCountMap();
+        Rank[] orderedRanks = {Rank.FIFTH, Rank.FOURTH, Rank.THIRD, Rank.SECOND, Rank.FIRST};
+
+        for (Rank rank : orderedRanks) {
+            String bonusText = getBonusText(rank);
+            System.out.printf("%d개 일치%s (%d원)- %d개%n",
+                    rank.getMatchCount(),
+                    bonusText,
+                    rank.getPrizeMoney(),
+                    matchCountMap.getOrDefault(rank, 0));
+        }
+    }


마찬가지로 함수가 10줄을 넘어가고 있는 것 같아요

메서드 분리로 요구사항을 준수하도록 수정하였습니다. 요구사항을 잘 지키도록 주의하겠습니다! 피드백 감사합니다!

woogym · 2025-03-05T17:01:31Z

src/main/java/view/ResultView.java

+    public static void printWinningStatistics(LottoResult lottoResult, int totalCost) {
+        System.out.println("\n당첨 통계");
+        System.out.println("---------");
+
+        printWinningDetails(lottoResult);
+        printProfitRate(lottoResult, totalCost);
+    }


ResultView 클래스의 다른 메서드와 동일한 내용입니다
MVC패턴에서 View는 Model을 알고 있어서는 안됩니다 Controller를 통해서 상호 작용을 해야하죠
하지만 지금 정빈이의 코드는 View가 Model의 내부 구현을 알고 있어야하고 또 알고 있습니다(인자로 model을 받아오고 있음)
ex) Map<Rank, Integer> matchCountMap = lottoResult.getMatchCountMap();

model이 변경되면 view의 변경또한 필연적입니다
저번 피드백과 동일한 피드백인 것 같은데 개선해봅시다!

피드백을 반영하여 View가 Model을 직접 참조하지 않도록 수정했습니다.
ResultView는 LottoResult나 LottoTickets 같은 Model을 직접 알지 않고, Controller에서 변환된 데이터를 받아 출력만 담당하도록 리팩토링 하였습니다..!
피드백 감사합니다!

woogym · 2025-03-05T17:02:24Z

src/test/java/controller/LottoControllerTest.java

+        Money validMoney = new Money(5000);
+        Money invalidMoney1 = new Money(900);  // 1000원 미만
+        Money invalidMoney2 = new Money(1500); // 1000원 단위가 아님


의미있는 변수명을 작성한다면 주석이 필요없을 것 같아요!
개선해봅시다!

네 의미있는 변수명인 경우 주석을 삭제하였습니다. 피드백 감사합니다!

JeongBeanHyun · 2025-03-16T15:05:25Z

비지니스 로직과 일관성 유지를 위한 도메인 유효성 검사 그렇다면 null체크, 데이터 형식 검사와 같은 입력 유효성 검사는 어디서 이루어져야할까? 한 번 고민해보고 정빈이만의 답변을 알려주세요! 꼭 고민해봤으면 좋겠어요 (GPT 사용하지 않고요!)

입력 유효성 검사는 사용자가 입력한 값이 올바른 형식인지 확인하는 과정이라고 생각합니다. 그렇기 때문에 잘못 입력된 데이터가 도메인에 전달되기 전에 실행되기 전에 컨트롤러에서 검증하는 것이 적절한 것 같습니다.
그리고 도메인 유효성 검사는 비즈니스 로직과 일관성 유지를 위한 도메인 유효성 검사는 도메인 규칙을 따르는지 확인하는 과정이기 때문에 도메인 내부에서 검증하는 것이 적절하고 생각합니다..!

답변&리팩토링이 늦어져 죄송합니다. 소중한 시간 내어 피드백 해주셔서 정말정말 감사드립니다!!!!

# Conflicts: # src/main/java/view/ResultView.java

JeongBeanHyun added 11 commits February 20, 2025 04:36

feat: Lotto 추가

5e2fb30

feat: controller 추가

f9f5a87

feat: view 추가

1308961

feat: application 추가

d7bfc50

refactor: controller 예외 추가 및 수정

8188fc6

refactor: Lotto 책임분리및 개행 수정

a63a201

refactor: 네이밍 수정

643a77f

refactor: LottoTickets 추가

2be807c

refactor: 피드백 수정

4a44acc

refactor: 1단계 수정

fa5e7ff

feat: 2,3,4,5 단계 추가

b9c9bad

woogym reviewed Mar 5, 2025

View reviewed changes

JeongBeanHyun added 2 commits March 17, 2025 00:13

Merge branch 'jeongbeanhyun' into jeongbeanhyun2

3f73124

refactor: 2,3,4,5 리팩토링

0a91304

# Conflicts: # src/main/java/view/ResultView.java

JeongBeanHyun requested a review from woogym March 16, 2025 15:32

[LBP] 현정빈 로또 미션 2-5 단계 제출합니다. #90

Are you sure you want to change the base?

[LBP] 현정빈 로또 미션 2-5 단계 제출합니다. #90

Uh oh!

Conversation

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

Uh oh!

3주차 리뷰

질문

기능 설명

Uh oh!

woogym left a comment

Choose a reason for hiding this comment

Q&A

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!

JeongBeanHyun Mar 16, 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!

woogym Mar 5, 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!

JeongBeanHyun Mar 16, 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!

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!

JeongBeanHyun Mar 16, 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!

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

Uh oh!

Uh oh!

Uh oh!

JeongBeanHyun commented Mar 3, 2025 •

edited

Loading

JeongBeanHyun Mar 16, 2025 •

edited

Loading

woogym Mar 5, 2025 •

edited

Loading

JeongBeanHyun Mar 16, 2025 •

edited

Loading

JeongBeanHyun Mar 16, 2025 •

edited

Loading

JeongBeanHyun commented Mar 16, 2025 •

edited

Loading