Conversation
There was a problem hiding this comment.
안녕하세요~ 대현님! 리뷰어 이진입니다.
로또 미션 어럽진 않으셨나요~ 구현하시느라 고생많으셨어요!!
PR 본문에 고민해주신 내용과 궁금한 지점을 남겨주셔서 저도 대현님의 시각에서 한번 더 바라볼 수 있었던 것 같습니다~
아래 정리해주신 내용에 대한 답변 남겨놨어요!
한번 확인해주시도 궁금한 점이나 모호한 점이 있으면 언제든 DM으로 연락주세요~! 기다리고 있겠습니다 ㅎㅎ 화이팅 :)
🤔 고민한 내용
Rank를 어떻게 구현할까 고민하다 enum으로 구성해봤습니다.
단순한 값을 표현하는 객체는 record로 구현해봤습니다.
원시값 포장을 적용해보려고 노력했습니다.
메서드가 한 가지 일만 하도록 최대한 분리해보려고 했습니다.
오 되게 좋은 고민을 많이 해주셨네요~~ 👍👍👍
enum이랑 record를 배워가는 미션이었을 것 같습니당
Lotto 가 좀 두껍다고 생각되는데 이정도는 괜찮은건지 아니면 책임을 나눠야하는지 궁금합니다.
세부 코멘트로 남겨놨어요~
WinningResult로 넘겨도 View가 간접적으로 Domain을 알게 된다고 생각하는데 굳이 WinningStatistics 같은 Domain 객체를 그대로 넘기지 않고 별도 객체로 변환하는 방식이 효율적인지 리뷰어님의 의견이 궁금합니다.
좋은 고민이에요! 핵심은 "Domain 객체가 변경되었을 때 그 영향이 어디까지 퍼지는가"입니다. 혹시 "View에 Domain 객체를 그대로 넘겼을 때, 나중에 Domain 내부 구조가 바뀌면 어떤 일이 생길까?"라는 관점에서 한번 생각해보시면 어떨까요? 🤔
이 고민 정말 좋아요! View가 Domain을 직접 알게 되는 것과, 별도 객체로 변환해서 넘기는 것 사이에서 고민하신 거잖아요.
1단계에서는 원시값을 그대로 사용하다가, 2단계에서 PurchaseAmount, LottoNumber를 도입하면서 구조 변경이 꽤 많이 발생했습니다. 요구사항에 맞게 잘 진행된 것인지 궁금합니다.
맞아요, 원시값을 값 객체로 포장하면 구조 변경이 많이 발생하는 건 자연스러운 현상이에요! 오히려 그 변경의 범위가 크다는 건, 그만큼 해당 값이 여러 곳에서 검증 없이 사용되고 있었다는 신호이기도 해요. 포장을 통해 검증과 책임이 한 곳으로 모이면서 전체 구조가 더 안정적으로 바뀐 거라고 보시면 됩니다.
View로 값을 전달하기 위해 toNumberLists() 같은 메서드를 두었는데, 이런 식으로 출력용 형태로 변환하는 메서드를 도메인 객체가 가지는 것이 적절한지 궁금합니다.
좋은 질문이에요! 핵심은 이 메서드가 "View를 위한 변환"인지, "도메인 객체가 자신의 내부 정보를 외부에 안전하게 제공하는 방법"인지의 구분입니다. toNumberLists()가 내부 필드를 그대로 노출하는 게 아니라 Integer로 변환해서 반환한다면, 이건 도메인 객체가 자신의 표현 방식을 스스로 결정하는 것이므로 적절한 책임이에요. 반면 View의 출력 포맷에 맞추기 위한 로직(예: 문자열 조합, 정렬 등)이 들어가기 시작하면, 그건 도메인의 책임을 넘어서는 신호이라고 생각이 들어요~
| import java.util.List; | ||
|
|
||
| public class LottoController { | ||
| private final InputView inputView = new InputView(); |
There was a problem hiding this comment.
필드를 직접 주입해주고 있네요! 대현님의 코드를 전반적으로 살펴봤을 때, 도메인 객체에는 생성자 주입 방식을 사용하고 계신 반면, Controller나 InputView에는 직접 주입하고 계신 것 같아요. 이렇게 나누신 이유가 있으시다면 들어보고 싶어요!
There was a problem hiding this comment.
view를 static으로 두면 메서드 확장에 제약이 있다고 생각했고 그 과정에서 내부에서 직접 생성해 사용했습니다. view와 controller에도 생성자 주입방식을 사용한다는 생각을 못해봤네요😅 덕분에 이번 피드백을 통해 객체를 내부에서 직접 생성하는 방식보다 생성자 주입 방식이 변경에 더 유연하다는 걸 알게 됐어요!
Application에서 필요한 객체를 생성한 뒤 controller에 주입하도록 수정했습니다.
| @@ -0,0 +1,20 @@ | |||
| package domain; | |||
|
|
|||
| public record LottoNumber(int number) { | |||
There was a problem hiding this comment.
LottoNumber 를 record class 방식으로 작성해주셨어요 이를 선택하신 이유가 있으실까요?
There was a problem hiding this comment.
LottoNumber는 변경하지 않는 객체라고 판단해 record를 사용했습니다. 또한 record를 사용하면 단순 접근자 같은 반복되는 코드를 줄일 수 있다고 생각했습니다.
There was a problem hiding this comment.
대현님만의 record 사용 이유를 설명해주셔서 좋네요~
맞습니다, record에서는 다음과 같은 특징이 있지요:
- 모든 필드에 자동으로
private final선언 - getter 자동 생성
그러면 이 특징을 그대로 class로 옮겨보면 이런 모습이 될 텐데요:
public final class LottoNumber {
private static final int MIN_NUMBER = 1;
private static final int MAX_NUMBER = 45;
private final int number;
public LottoNumber(int number) {
validate(number);
this.number = number;
}
private void validate(int number) {
validateRange(number);
}
private void validateRange(int number) {
if (number < MIN_NUMBER || number > MAX_NUMBER) {
throw new IllegalArgumentException("로또 번호는 1부터 45 사이여야 합니다.");
}
}
public int number() {
return number;
}
}이렇게 하면 대현님이 작성하진 로직이 이전과 동일하게 동작할까요?
There was a problem hiding this comment.
처음에는 동일하게 작동하지 않을까? 라고 생각했는데 실제로 넣어보니 테스트 코드에서 오류가 발생했네요.
그 이유를 확인해보니 record는 equals()와 hashCode()를 자동으로 제공해서 값 기반 비교가 가능하다는 차이가 있었습니다. 이번 기회에 자바가 값을 어떻게 비교하는지도 알게 됐습니다!
| private final List<Lotto> lottos; | ||
|
|
||
| public Lottos(List<Lotto> lottos) { | ||
| this.lottos = new ArrayList<>(lottos); |
There was a problem hiding this comment.
코드를 보니 방어적 복사 방식을 두 가지 사용하고 계시네요! 여기서는 new ArrayList<>()를, Lotto 클래스에서는 Collections.unmodifiableList()를 쓰셨는데, 각각의 차이와 그렇게 선택하신 이유가 있을까요?
There was a problem hiding this comment.
시간을 나눠서 코드를 짜다보니 제각각으로 복사를 진행했네요...😶 둘의 차이를 명확히 모르던 터라 이번 기회에 new ArrayList<>()는 별도의 새 리스트를 만드는 방법이고 Collections.unmodifiableList()는 읽기전용으로 보이게만 한 방법이라는 점을 알게됐어요!
new ArrayList<>() 가 더 적절하다 생각하여 new ArrayList<>()로 통일했습니다.
There was a problem hiding this comment.
new ArrayList<>() 방식의 방어적 복사를 사용해서 외부와의 참조를 끊어내셨군요 👍
| this.prizeMoney = prizeMoney; | ||
| } | ||
|
|
||
| public static Rank from(int matchCount) { |
There was a problem hiding this comment.
matchCount를 필드로 잘 설계해두셨네요! 이미 각 Rank가 matchCount를 알고 있으니, from() 메서드에서도 이 필드를 활용해볼 수 있지 않을까요? 지금처럼 if 분기로 하나씩 비교하면 Rank가 늘어날 때마다 분기문도 함께 늘어나서 관리가 어려워질 수 있을 것 같아서요!
There was a problem hiding this comment.
이제 보니 if 분기로 나누는 것이 꽤나 비효율적이네요. stream으로 Rank를 순회해 matchCount에 해당하는 Rank를 찾는 방식으로 수정했습니다.
|
|
||
| class LottoNumberTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
지금도 좋지만, 같은 검증 로직에 대해 경계값 여러 개를 하나의 테스트로 묶어서 처리할 수 있는 방법도 있을 것 같아요~
There was a problem hiding this comment.
좀 더 좋은 방법이 있지 않을까? 라는 생각이 있었는데 역시 있었군요! ParameterizedTest를 통해 하나의 테스트로 묶어봤습니다.
src/main/java/domain/Lotto.java
Outdated
|
|
||
| public class Lotto { | ||
| private final List<LottoNumber> numbers; | ||
| private final int LOTTO_SIZE = 6; |
There was a problem hiding this comment.
지금 코드로는 상수가 매 인스턴스마다 할당이 될 것 같아요!
만약 수정한다면 어떻게 수정할 수 있고, 지금과 어떤 차이가 있길래 그 방법을 사용하셨는 지도 함께 남겨주시면 좋을 것 같습니다~
There was a problem hiding this comment.
앗 이 부분을 인스턴스로 두고 있었네요. static으로 선언하도록 수정했습니다! static으로 두면 객체가 아니라 클래스에 할당되어 매 객체마다 생성되지 않고, 해당 값의 성격도 더 분명하게 드러난다고 생각합니다!
| private static final int LOTTO_SIZE = 6; | ||
|
|
||
| @Override | ||
| public List<LottoNumber> generate() { |
There was a problem hiding this comment.
로또 번호 생성 로직을 직접 구현하신 점 좋았어요!
현재 generate를 호출할 때마다 1~45 범위의 LottoNumber 객체를 매번 새로 생성하고 있는 것 같은데요, 혹시 이 숫자들이 가지는 특성을 생각해보면 매번 새로 만들어야 할 필요가 있을까라는 생각도 함께 들었ㅇ요
1 ~ 45라는 범위가 고정되어 있고, 같은 값을 가진 객체는 사실상 동일하다는 점을 고려했을 때, 이 객체들을 재사용할 수 있는 방법이 있을지 한번 고민해보시면 좋을 것 같아요~
There was a problem hiding this comment.
static로 선언하게 되면 이런 값의 성격을 더 분명하게 드러낼 수 있다는 점을 알게 되었습니다. static에 대한 개념은 알고 있었지만 실제 코드에서 어떻게 활용되는지는 잘 와닿지 않았는데, 이번에 static에 대해 잘 알아가는 것 같습니다🤭
번호 목록은 static으로 한 번만 생성해두고, generate()에서는 new ArrayList<>()로 복사한 뒤 사용하도록 수정했습니다.
There was a problem hiding this comment.
좋네요~!
static의 개념을 아는 것과 실제 코드에서 적절히 활용하는 것은 다른 영역인데, 이번 경험을 통해 그 간극을 좁히신 것 같아서 좋습니다. new ArrayList<>()로 복사해서 사용하신 부분도 깔끔하네요! 👍
| private static final int LOTTO_SIZE = 6; | ||
|
|
||
| @Override | ||
| public List<LottoNumber> generate() { |
There was a problem hiding this comment.
한 가지 같이 생각해보면 좋을 부분이 있는데요, generate() 메서드 내부에서 정렬까지 수행한 뒤 반환하고 계시잖아요. 만약 다른 개발자가 이 메서드를 처음 사용한다고 상상해보면, 메서드 이름이나 시그니처만 보고 "반환값이 이미 정렬되어 있다"는 걸 알 수 있을까요? 🤔
출력 요구사항에서 정렬된 로또 번호가 필요하다면, 내부 구현을 모르는 개발자가 바깥에서 한번 더 정렬하는 코드를 추가할 수도 있을 것 같아서요.
There was a problem hiding this comment.
처음에 출력 예시에서 정렬된 로또 숫자를 보고 코드를 작성하다보니 자연스레 번호를 생성할 때부터 정렬하도록 구현했네요. 다시 생각해보니 domain 안에서는 로또가 정렬될 필요가 없다고 생각되어 OutputView에서 정렬하여 출력하도록 수정하였습니다
src/main/java/domain/Lottos.java
Outdated
| .toList(); | ||
| } | ||
|
|
||
| public WinningStatistics createWinningStatistics(Lotto winningLotto) { |
There was a problem hiding this comment.
Lottos를 일급 컬렉션으로 설계하신 점 좋습니다! 👍
Lotto 가 좀 두껍다고 생각되는데 이정도는 괜찮은건지 아니면 책임을 나눠야하는지 궁금합니다.
본문에 직접 언급해주시기도 하셨네요~ 이런 감각이 생기고 계신 것 같아서 좋아요!
조금 더 방향성을 드려볼게요. createWinningStatistics 메서드가 현재 Lottos 안에 위치해 있는데요, Lottos가 "로또 묶음"을 표현하는 역할이라면, 당첨 통계를 생성하는 것도 이 객체가 책임져야 할 일인지 한번 생각해보시면 좋을 것 같아요 🤔
정답은 없지만, "이 객체가 지금 몇 가지 역할을 하고 있지?"라는 질문을 던져보시면 책임 분리에 대한 좋은 힌트를 얻으실 수 있을 거예요 😊
There was a problem hiding this comment.
조금씩 늘어가고 있는거 같아 뿌듯하네요!
LottoShop에서 당첨 결과까지 판단하는게 좋을까 생각하다 WinningStatistics에서 당첨결과를 아는게 자연스럽다고 생각해 정적 팩토리 메서드를 통해 WinningStatistics가 직접 Lottos와 winningLotto를 입력받아 생성하도록 했습니다.
다만 수정하는 과정 중에 Lottos 외의 클래스에서 구현한다면 결국 Lottos의 값을 가져와야 하는데 getter를 사용하는 것은 어색하다 생각하다고 생각하여 Lottos가 stream 을 제공하는 메서드를 사용했습니다, 이것이 적절한 방법인지 리뷰어님의 생각이 궁금합니다!
There was a problem hiding this comment.
리뷰 반영해주신 거 확인했습니다 좋네요👍
대현님이 getter가 어색하다고 느낀 이유를 좀 더 들어보고 싶네요! 지금 Lottos의 값이 필요한 건 바뀌지 않는 사실이기에, 값을 어떻게 가져올지를 고민해봐야 하는 지점이군요!
혹시 getter와 Stream이 어떤 점에서 다르다고 느끼셨는지 궁금해요! 저도 처음엔 비!슷하게 고민했었는데, 생각해보면 둘 다 결국 값을 가져오는 방식이라 역할 자체는 비슷한 거 아닌가 싶어서요!
다만 Stream은 현재 호출을 한 번만 하고 재사용하지 않아서 문제가 없지만, 아래와 같은 상황이면 어떤 문제가 발생하고 그 이유는 무엇일까요?
// Stream 반환
Stream<Lotto> stream = lottos.stream();
stream.forEach(...); // 첫 번째 사용
stream.forEach(...); // 두 번째 사용 -> ??참고로 List를 반환하면 여러 곳에서 몇 번이든 꺼내 쓸 수 있고, for문이든 stream이든 호출하는 쪽이 선택할 수 있다는 장점도 있지 않을까 싶어요!
There was a problem hiding this comment.
getter가 어색한 이유는 Lotto 에 createWinningStatistics가 있었을때는 getter가 필요 없기도 헀고, getter 자체도 외부에 값을 그대로 노출해서 좋지 않다고 생각했습니다. 그래서 찾아보다 직접 노출 하지 않는 Stream으로 처리해보자고 생각했습니다.
Stream은 값을 담고 있는 객체가 아니라 파이프라인이기에 forEach, collect, count 같은 최종연산이 끝나면 소비되어 사용할 수 없기 때문에 두 번째 사용부터는 예외처리가 된다는 것을 알게 됐습니다!
원래의 의도였던 외부 수정 방지와 여러 번 순회할 수 있는 점에서 List로 반환하는 것이 제가 생각한 의도와 잘 맞아서 수정했습니다.
| .toList(); | ||
| } | ||
|
|
||
| private List<WinningResult> createWinningResults(WinningStatistics winningStatistics) { |
There was a problem hiding this comment.
Controller에서 당첨 결과를 조합하는 로직을 직접 작성하신 점, 동작 자체는 잘 되고 있어요! 👍
한 가지 같이 생각해볼 부분이 있는데요, createWinningResults에서 Rank의 필드들(getMatchCount(), getPrizeMoney())을 하나하나 꺼내서 WinningResult를 만들고 있잖아요. 이렇게 되면 Rank의 내부 구조가 Controller까지 노출되는 셈인데, 혹시 이 부분이 캡슐화 관점에서 어떤 의미를 가질지 생각해보신 적 있으신가요? 🤔
그리고 WinningStatistics가 이미 Map<Rank, Integer> 형태로 Rank를 키로 가지고 있는데, 이 구조를 활용하면 당첨 결과 생성을 더 자연스러운 곳에서 처리할 수 있지 않을까요?
대현님이 현재 방식을 선택하신 이유도 짐작이 가요 — statistics에 MISS까지 포함되어 있다 보니 구분이 필요했던 것 같아요. 그렇다면 "당첨에 해당하는 Rank만 골라내는 책임은 누구에게 있을까?"라는 질문을 한번 던져보시면 좋을 것 같습니다.
There was a problem hiding this comment.
출력용 DTO를 Controller 에서 생성하고 싶다 보니 Rank의 내부 값에 직접 접근하는 구조를 만들게 되었습니다.
다시 보니 Controller가 Rank의 내부 값에 직접 접근하는 구조는 캡슐화 관점에서 아쉽다고 생각했습니다.
그래서 당첨에 해당하는 Rank를 판별하는 책임은 Rank에 있다고 보고 isWinning()을 추가했습니다. 또한 WinningStatistics가 이미 Map<Rank, Integer> 형태로 당첨 통계를 가지고 있는 점을 활용해, WinningResult 생성도 WinningStatistics에서 처리하도록 수정했습니다.
다만 이 과정에서 domain이 출력용 DTO를 직접 생성하는 구조가 적절한가? 라는 고민이 들었습니다. 현재는 Controller가 Rank 내부를 알지 않도록 하는 쪽을 우선해 수정했는데, 이 방향에 대해서도 의견 주시면 감사하겠습니다.
There was a problem hiding this comment.
고민의 흐름이 좋습니다!
Controller가 Rank 내부를 아는 것보다 WinningStatistics가 DTO를 아는 쪽을 선택한 판단, 저도 이 규모에서는 합리적이라고 생각해요.
한 가지 더 생각해볼 점을 드리자면 — 지금 WinningStatistics가 WinningResult를 직접 생성하고 있는데, 만약 출력 형태가 바뀌어서 WinningResult의 필드가 달라진다면 WinningStatistics도 함께 수정해야 하잖아요. 즉 출력 요구사항 변경이 도메인을 건드리는 상황이 되는 건데요.
이 방향을 유지하면서도 그 영향을 줄일 수 있는 방법이 있을지 고민해보시면 좋겠습니다!
There was a problem hiding this comment.
3가지 정도의 방법을 생각해봤는데 리뷰어님의 생각은 어떠신지 궁금합니다.
-
createWinningResults를 다시 Controller에 옮기되,WinningStatistics는 Map<Rank, Integer> 같은 데이터를 활용해Rank는 모르도록 한다. -
WinningResult로 변환하는 책임을 별도의 클래스(
WinningResultTransfer)로 분리한다. -
WinningStatistics는 Map<Rank, Integer>만 제공하고, WinningResult 쪽에서 정적 팩토리 메서드로 필요한 형태로 변환한다.
개인적으로는 controller를 거쳐 DTO에 전달하는 1번이 단순하다 느껴져 더 적절하다고 생각하는데 리뷰어님의 생각이 궁금합니다!
test: WinningStatistics 테스트 수정
2Jin1031
left a comment
There was a problem hiding this comment.
대현님~ 리뷰 반영하시고 코멘트까지 달아주신 것 모두 확인했습니다! 고생 많으셨어요~!
충분히 고민하신 흔적이 보이고, 이번 미션의 핵심 포인트들을 잘 잡으신 것 같아요. 이대로 merge해도 좋겠지만, 몇 가지 더 이야기 나눠보고 싶은 부분이 있어서 코멘트를 조금 남겨봤습니다!
늦은 시간에 드리는 거라 부담 갖지 마시고 가볍게 확인해주시면 됩니다~!
src/main/java/domain/Lotto.java
Outdated
| this.numbers = new ArrayList<>(numbers); | ||
| } | ||
|
|
||
| public List<LottoNumber> getNumbers() { |
There was a problem hiding this comment.
보통 getter는 도메인 객체에서 매우 자주 보이는 패턴이기도 하고, 메서드 내용을 볼 필요가 없어서 클래스 하단에 위치해놓는 편입니당~ 참고용으로 생각해주셔도 좋을 것 같네요 ㅎㅎ
src/main/java/view/InputView.java
Outdated
| return Arrays.stream(input.split(",")) | ||
| .map(String::trim) | ||
| .map(Integer::parseInt) | ||
| .sorted() |
There was a problem hiding this comment.
앗, 그렇네요.. 다시 보니 입력 단계에서 정렬할 필요는 없어 수정했습니다.
| class LottoShopTest { | ||
|
|
||
| @Test | ||
| void 구입금액만큼_로또를_구매한다() { |
There was a problem hiding this comment.
구입금액만큼 로또를 구매한다는 메서드명만을 본다면 검증을 어떤 것을 한다고 기대할까요?
There was a problem hiding this comment.
테스트명과 검증 내용이 맞지 않았던 것 같아, 구매 개수를 검증하는 테스트와 생성된 번호를 검증하는 테스트로 분리해 재구성했습니다!
| class LottosTest { | ||
|
|
||
| @Test | ||
| void 로또_목록을_번호_리스트로_변환한다() { |
There was a problem hiding this comment.
toNumberLists() 메서드에 대한 테스트를 작성해주셨네요! 한 가지 궁금한 점이 있는데요, 이 메서드가 내부적으로는 stream 변환을 수행하고 있잖아요. 이 테스트를 통해 어떤 상황에서의 안전성을 확보하고 싶으셨는지 궁금합니다!
There was a problem hiding this comment.
toNumberLists()가 Lottos를 List<List<Integer>> 형태로 잘 변환하는지 확인하고 싶어 작성했습니다. 그 당시에 LottosTest를 뭘 작성할까 고민하다 안에 있던 메서드인 toNumberLIsts()를 검증하게 됐습니다!
🙋♂️ 인사
안녕하세요 이진님! 백엔드 4기 강대현입니다. 잘 부탁드립니다.
🚗 로또 미션 1,2단계
로또 구입 금액을 입력하면 구입 금액에 해당하는 로또를 발급하고 로또 당첨 번호를 받아 일치한 번호 수에 따라 당첨 결과를 보여준다.
🤔 고민한 내용
Rank를 어떻게 구현할까 고민하다 enum으로 구성해봤습니다.❓ 질문사항
Lotto가 좀 두껍다고 생각되는데 이정도는 괜찮은건지 아니면 책임을 나눠야하는지 궁금합니다.WinningResult를 만들어 출력에 필요한 값만 전달하도록 구성했습니다. 결국WinningResult로 넘겨도 View가 간접적으로 Domain을 알게 된다고 생각하는데 굳이WinningStatistics같은 Domain 객체를 그대로 넘기지 않고 별도 객체로 변환하는 방식이 효율적인지 리뷰어님의 의견이 궁금합니다.PurchaseAmount,LottoNumber를 도입하면서 구조 변경이 꽤 많이 발생했습니다. 요구사항에 맞게 잘 진행된 것인지 궁금합니다.toNumberLists()같은 메서드를 두었는데, 이런 식으로 출력용 형태로 변환하는 메서드를 도메인 객체가 가지는 것이 적절한지 궁금합니다.