-
Notifications
You must be signed in to change notification settings - Fork 92
[그리디] 하수한 로또 미션 3, 4, 5단계 제출합니다. #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: chemistryx
Are you sure you want to change the base?
Conversation
LottoFactory.createLotto()를 통해 무작위 생성하는 경우 사전에 정의된 보너스 번호(1)과 중복되어 테스트가 실패하는 케이스 발생
if (manualCount > 0) { | ||
purchaseManualNumbers(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controller에서 이미 로또 수 검증을 하고 있는데, 실행 단계에서도 같은 검증이 중복되고 있네요.
검증 로직은 하나만 두어도 괜찮을 거 같아요.
if (manualCount < 0) {
throw new IllegalArgumentException("로또 수는 0 또는 양수만 입력할 수 있습니다.");
}
추가적으로 로또 수 검증 로직은 Controller에 두는 게 적절할 지, 아니면 Model에 두는 게 더 적절할지 한 번 고민해보시면 좋겠습니다. (정답은 없습니다 :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 Controller에서는 manualCount
가 음수인 경우를 1차적으로 필터링하고, 이후 PurchaseExecutor
에서는 양수인 경우에만 InputView.getManualNumbers()
를 호출하여 수동 번호를 입력받도록 하고 있습니다.
말씀하신대로 어느정도 중복되는 느낌이 들긴 하나, PurchaseExecutor
에 있는 검증 로직(if (manualCount > 0)
)을 없앨 경우 InputView.getManualNumbers()
가 무조건 1회는 실행되게 되어 0을 입력받게 되는 경우에도 수동으로 구매할 번호를 입력해 주세요.
메시지가 출력되는 문제가 있었습니다.
따라서 저런 방식으로 두번 검증하여 0인 경우에는 메시지 자체도 출력되지 않도록 구현하였는데, 혹시 더 좋은 방법이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"수동으로 구매할 번호를 입력해 주세요." 메시지가 출력되는 문제가 있었다는 사실은 몰랐네요.
다른 방법이라고 한다면 inputView에서 0을 바로 처리하는 방법도 있습니다.
ex) 0이 입력되면 바로 빈 배열을 return
하지만 이렇게 되면 코드에서 ‘빈 리스트를 반환하는 이유’가 명확히 드러나지 않고,
로직의 책임 분리 측면에서도 적합하지 않은 것 같습니다.
따라서, 저 역시 수한님께서 하신 방법을 사용할 거 같네요. 자세한 설명 감사드립니다 :)
} | ||
|
||
this.type = type; | ||
this.numbers = new HashSet<>(numbers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중복 제거를 위해 Set을 사용하신 거 맞죠? 😊 로또의 자료구조를 고민하셨다는 게 잘 느껴져서 좋게 봤습니다.
다만 Set의 대표적인 구현체인 HashSet, LinkedHashSet, TreeSet 중에서 HashSet을 선택하신 이유가 조금 궁금합니다.
예를 들어 로또 번호는 출력 시 정렬된 상태로 보여주기 때문에 TreeSet도 고려할 수 있을 것 같은데요.
혹시 HashSet을 선택하신 데 특별한 이유가 있으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 맞습니다~! 로또 구조의 특성상 중복을 제거할 필요가 있다고 판단하여 Set
을 통해 구현하였습니다.
여러 Set 구현체 중 HashSet을 선택한 이유는 다음과 같은데, 혹시 설명이 더 필요한 부분이 있다면 언제든지 말씀해주세요..!
구현체 | 중복 | 입력 순서 보장 | 정렬 |
---|---|---|---|
HashSet |
X | X | X |
LinkedHashSet |
X | O | X |
TreeSet |
X | X | O |
- 먼저
중복
조건의 경우 셋 다 허용하지 않기 때문에 모두 만족했습니다. 입력 순서 보장
의 경우 굳이 보장할 필요가 없다고 생각하여LinkedHashSet
은 제외했습니다.- 마지막으로
정렬
의 경우 출력 예시를 보면 로또 번호들이 정렬되어 출력이 된 것을 확인할 수 있었습니다.
따라서 초기에는TreeSet
을 적용하려고 했으나,TreeSet
의 경우 Set의 원소LottoNumber
에 대해Comparable<T>
를 구현해야 하는 부담이 있었습니다.
게다가 Set에 대한 연산을 수행하는 과정에서 원소들이 꼭 정렬되어있을 필요는 없다고 판단하여, 출력 시에만 정렬된 상태로 출력될 수 있도록 구현을 진행했습니다..!
LottoRegistry registry = new LottoRegistry(); | ||
|
||
Lotto winningLotto = LottoFactory.createLotto(); | ||
Set<LottoNumber> numbers = Set.of( | ||
new LottoNumber(1), new LottoNumber(2), new LottoNumber(3), | ||
new LottoNumber(4), new LottoNumber(5), new LottoNumber(6)); | ||
|
||
Lotto winningLotto = Lotto.of(numbers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 위 아래 test코드에 전부 쓰이는 로직으로 보이네요 :)
혹시 @BeforeEach
어노테이션을 아시나요?
@BeforeEach
란?
각 테스트 실행 전에 공통 초기화 로직을 실행해주는 JUnit5 어노테이션
감이 잘 안 잡히실 수 있어서, 예시를 하나 보여드리면 아래와 같습니다.
@BeforeEach
void setUp() {
registry = new LottoRegistry();
winningNumbers = Set.of(
new LottoNumber(1), new LottoNumber(2), new LottoNumber(3),
new LottoNumber(4), new LottoNumber(5), new LottoNumber(6)
);
winningLotto = Lotto.of(winningNumbers);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이 로직은 해당 클래스에 있는 메서드 전체에 적용되는 초기화 로직이라,
추후 수한님이 어떤 메서드를 만드느냐에 따라 필요 없을 수도 있어요.
그래도 알고 계시면 좋을 것 같아서 적어둡니다 🙂
코드가 길지 않으니 지금은 꼭 필요하지 않더라도, 한 번 적용해보는 경험을 가져보면 좋을 거 같아요.
그래야 나중에 정말 필요할 때 '아, 이런 게 있었지' 하고 떠올리기 쉬우니까요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안그래도 테스트 코드 작성 과정에서 중복되는 공통 초기화 로직이 있어서 마음에 걸렸는데, 좋은 해결 방법이 있었네요!
381d084 커밋에 반영했습니다~!
if (manualCount < 0) { | ||
throw new IllegalArgumentException("로또 수는 0 또는 양수만 입력할 수 있습니다."); | ||
} | ||
|
||
if (PRICE_PER_LOTTO * manualCount > balance) { | ||
throw new IllegalArgumentException("금액이 부족합니다."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 에러 처리를 꼼꼼하게 신경 쓰신 점이 잘 느껴졌습니다.
다만 현재 코드를 보면, 에러가 발생했을 때 프로그램이 바로 종료되는 흐름이라 조금 아쉬운 부분이 있습니다.
예외가 발생했을 때 단순히 프로그램이 종료되는 것이 아니라 다시 입력을 받을 수 있도록 처리한다면 사용자 경험이 훨씬 좋아질 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀하신대로 지금 로직의 경우 예외가 발생한 경우 프로그램이 바로 종료되는 방식입니다..!
확실히 사용자 경험 측면에서 아쉬운 부분이 있는건 사실이라 다시 입력받는 방식으로 수정 진행해보겠습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 수정사항 3768376 커밋에 반영했습니다!
관련해서 작업하던 중 궁금한 점이 생겼는데, 전체 코멘트에 이어서 달아놓겠습니다!
@Test | ||
void 금액에_맞는_로또를_구매할_수_있다() { | ||
LottoRegistry registry = new LottoRegistry(); | ||
int balance = 5000; | ||
int count = 5; | ||
int balance = PRICE_PER_LOTTO * count; | ||
|
||
PurchaseExecutor executor = new PurchaseExecutor(registry, balance); | ||
PurchaseExecutor executor = new PurchaseExecutor(registry, balance, 0); | ||
executor.execute(); | ||
|
||
int expectedSize = balance / PRICE_PER_LOTTO; | ||
assertThat(registry.getLottos()).hasSize(expectedSize); | ||
assertThat(registry.getLottos()).hasSize(count); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드를 작성할 때 많이 쓰이는 방식 중 하나가 Given-When-Then 패턴입니다.
Given-When-Then 패턴이란?
- Given : 테스트를 준비하는 단계
ex) 초기 세팅 - When : 실제 동작을 실행하는 단계
ex) 검증할 메소드 호출 - Then : 실행 결과를 검증하는 단계
ex) assert로 결과 확인
이렇게 작성하면 테스트 코드의 의도가 준비 → 실행 → 검증 흐름으로 명확히 드러나 테스트를 파악하기가 더 수월해집니다.
Given-When-Then 패턴을 한 번 적용해볼까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 해당 패턴을 적용함으로써 테스트 코드의 흐름이 더 명확히 드러나는 장점이 있겠네요!
사실 저번 리뷰어님께서도 언급해주신 내용이긴 한데, 제가 테스트 코드를 작성할때마다 매번 까먹는 내용인 것 같습니다 🥲
이번 기회를 통해 한번 더 해당 패턴에 입각하여 테스트 코드를 업데이트해보았는데, 한번 확인 부탁드려요..!
반영 커밋: f9bce69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전반적으로 Given-When-Then 패턴 잘 쓰신 거 같네요 ㅎㅎㅎ
가독성 있게 작성하셔서 잘 하셨습니다!! 수고하셨습니다 :)
public static List<Set<Integer>> getManualNumbers(int count) { | ||
System.out.println("\n수동으로 구매할 번호를 입력해 주세요."); | ||
List<Set<Integer>> numbers = new ArrayList<>(); | ||
|
||
for (int i = 0; i < count; i++) { | ||
numbers.add(parseNumbers(scanner.next())); | ||
} | ||
|
||
return numbers; | ||
} | ||
|
||
private static Set<Integer> parseNumbers(String input) { | ||
return Arrays.stream(input.split(",")) | ||
.map(Integer::parseInt) | ||
.collect(Collectors.toSet()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분을 보니 현재 InputView에서 문자열을 입력받은 뒤 곧바로 파싱까지 해서 숫자 리스트를 만드는 것 같네요.
숫자 리스트를 여기서 직접 만드는 이유가 있을까요? (정답은 없습니다. 단지 수한님의 의견이 궁금해요)
그리고 split(",") 부분은 ,가 매직 넘버처럼 보이는데, 상수로 분리해두면 가독성이 더 좋아질 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 입력 받은 숫자들의 경우 실제 비즈니스로직에서 사용하기 위해 String
-> Set<Integer>
-> Set<LottoNumber>
로 변환하는 3단계 작업이 필요합니다.
물론 InputView
상에서 단순히 입력 받은 값 자체를 넘겨주어 이를 처리하는 로직에서 파싱도 담당할 수 있지만, 해당 로직을 한군데서 모두 담당하기에는 그 양이 많다고 판단하여 이를 InputView
와 각 비즈니스로직이 분담하는 방식을 채택했습니다.
지금 방식의 경우 SRP를 완전히 준수한다고 보긴 어려울 수 있지만, 이정도는 InputView
에서 담당하여 보내주는 것이 다른 비즈니스로직에서 처리하는데 좀 더 편리하지 않을까 생각하여 적용하였는데, 혜빈님의 의견이 궁금합니다!
- split 부분은 미처 생각하지 못했는데 수정 진행하겠습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저라면 NumberParser
라는 객체를 따로 만들어서 InputView
는 단순히 문자열 입력만 담당하도록 했을 거 같습니다 :)
ex)
NumberParser
에서String → Set<Integer>
변환LottoFactory
에서Set<Integer>
→Set<LottoNumber>
변환
왜냐하면 저는 객체를 분리할 때 "에러가 났을 때 어디부터 들어가야 자연스러운가?”를 기준으로 삼는 데,
숫자 파싱 에러라면 InputView
보다는 NumberParser
에서 에러를 찾는 게 더 직관적이라고 생각했기 때문입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다만 저는 수한님 의견에도 동의합니다 :)
지금 수준의 파싱은 단순히 java 제공 함수인 split에 의존하는 정도라,
오히려 객체를 분리하는 게 더 복잡해질 수 있다는 점에서 충분히 합리적인 선택이라고 생각합니다.
좋은 의견 공유해주셔서 감사합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혜빈님께서 말씀해주신 NumberParser
를 도입하는 방법도 좋은 것 같습니다! 언급하신대로 파싱 오류가 발생할 경우 별도의 Parser가 있다면 그곳에서 오류를 던지거나 핸들링할 수 있으니 확실히 더 직관적인 접근으로 보이네요..!
말씀하신대로 지금은 간단한 파싱 로직이라 추후에 좀 더 복잡한 로직이 추가된다고 하면 별도의 Parser 구현을 통해 기능 분리를 진행해보도록 하겠습니다. 좋은 인사이트 공유 감사합니다! 👍👍
대체적으로 에러 발생 로직에 신경을 많이 쓰신 게 느껴졌습니다. 사실 지금 수준의 요구사항에서는 꼭 필요한 부분은 아니지만, 알고도 안 쓰는 것과 몰라서 못 쓰는 건 큰 차이니까요. 명절이니까 부담 갖지 마시고, 천천히 리뷰 남기시고 편할 때 연락 주세요! |
연휴임에도 불구하고 빠르게 리뷰 진행해주셔서 감사합니다..! 🙇
+) |
❓<질문에 대한 답>1️⃣-1
|
수한님! 시험 기간인데도 미션까지 병행하느라 정말 수고 많으십니다 😭 시험 기간이니 답변은 천천히 주셔도 괜찮습니다 :) |
안녕하세요 혜빈님! 리뷰어로써는 처음 뵙는 것 같은데, 이번 로또 미션 3, 4, 5단계 리뷰를 부탁드리게 되었습니다 ㅎㅎ..
이번 단계의 경우 기존 Map을 통해 하드코딩 되어있던 등수를 Enum으로 다시 작성하고, 추가된 입력 부분에 대한 예외 처리를 중점적으로 진행하였습니다!
현재 프로젝트 구조는 다음과 같습니다:
현재 애플리케이션의 실행 흐름은 다음과 같습니다:
2-1. 개수가 양수인 경우 사용자로부터 개수 만큼의 수동 번호를 입력 받습니다.
다음은 구현 과정에서 발생한 질문 / 고민사항입니다.
구현 관련해서 궁금하신 부분이 있다면 언제든지 말씀해주세요!!