[그리디] 강대현 로또 미션 3, 4, 5 단계 제출합니다.

[Kdahyn]

🙋‍♂️ 인사

안녕하세요 수한님! 백엔드 4기 강대현입니다. 이번에도 잘 부탁드립니다.

🚗 구현 내용

1, 2단계

• 로또 구입 금액을 입력받아 해당 금액만큼 로또를 발급한다
• 당첨 번호를 입력받아 일치한 번호 개수에 따라 당첨 결과를 계산한다.

3, 4, 5단계

• 2등 판별을 위해 보너스 볼을 추가한다.
• 사용자가 수동으로 로또 번호를 입력할 수 있게 한다.
• 리팩터링을 진행한다.

🤔 고민한 내용

• WinningResult를 Controller에서 생성할지, DTO 내부에서 생성할지 고민했고, 현재는 출력용 DTO가 스스로 생성하도록 정리했습니다.
• 원시값 포장을 어디까지 적용할지 고민했고, 우선은 최대한 포장해보는 방향으로 구현했습니다.
• Controller의 책임이 크다고 느껴 메서드 분리를 진행했습니다.
• 특히 run() 메서드가 커보여 나누어봤습니다.

❓ 질문사항

1. 현재 toLotto, toLottos가 Controller에 남아 있습니다.
   입력값을 Domain 객체로 변환하는 책임까지 Controller가 가지는 것이 적절한지 궁금합니다.
   Domain의 정적 팩토리 메서드로 옮기는 방법도 생각했지만, 정적 팩토리 사용이 과해지는 것 같아 일단 보류했습니다.

2. 원시값 포장은 어디까지 적용하는 것이 적절할지 궁금합니다. 예를 들어 BonusBall은 LottoNumber의 한 종류로도 볼 수 있고,
   ManualLottoCount는 PurchaseAmount와 연관된 값으로도 느껴졌습니다.
   이런 경우에도 별도 객체로 분리하는 것이 좋은지 궁금합니다.

3. 구현하면서 List<Integer>, Lotto, List<Lotto>, List<List<Integer>> 사이의 변환이 자주 발생했습니다.
   자료형 변환이 이 정도는 자연스러운 수준인지, 아니면 한 방향으로 더 정리하는 것이 좋은지 궁금합니다.

4. BonusBall이 추가되면서 Rank 판별 로직을 if문으로 구현했습니다.
   현재 방식이 적절한지, 더 나은 표현 방식이 있는지 궁금합니다.

[chemistryx]

대현님 안녕하세요~! 저번 미션에 이어 또 만나게 되었는데 저도 잘 부탁드립니다 ㅎㅎ
전체적으로 견고하게 잘 작성해주셔서 코드 관련 개선 포인트 몇가지와 질문에 대한 답변 위주로 진행해보았습니다 😄

질문에 대한 답변

1. toLotto, toLottos의 위치

정적 팩토리 메소드의 사용이 과해질 것 같다고 하셨는데, Lotto.from, Lottos.from 정도는 도메인 입력 형태와 자연스럽게 대응되는 느낌이라 과한 사용은 아니라고 생각해요. 오히려 변환 로직이 Controller에서 사라지고, 같은 변환이 필요한 다른 곳에서 재사용할 수 있다는 장점도 있다고 생각합니다~

2. 원시값 포장 범위

BonusBall과 ManualLottoCount 모두 타당하게 적용된 것 같아요. BonusBall은 LottoNumber와 범위 검증이 동일하더라도 당첨 번호와 중복 불가라는 별도의 비즈니스 규칙이 적용된다는 점에서 의미적으로 다른 개념이라고 생각해요. ManualLottoCount도 마찬가지로 총 구매 가능 수 초과 불가라는 고유한 검증 로직을 가지고 있으니 분리하는 방향이 맞다고 생각합니다!

3. 자료형 변환

View, Domain 간 변환이 발생하는 것은 자연스러운 현상이라고 생각하는데요, 첫번째 질문에서 언급한 것 처럼 정팩메를 도입하면 해당 변환 로직이 한곳에 모여서 현재보다 흐름이 단순해질 수 있을 것 같습니다!

4. Rank if문 형태

현재 방식도 명확하고 읽기 쉬워서 충분히 괜찮은 것 같아요. 다른 방식으로는 Rank 자체에 판별 조건을 내장하는 형태도 생각해볼 수 있을 것 같아요.

public boolean matches(int matchCount, boolean bonusMatched) {
    if (this == FIVE_BONUS_MATCH) {
        return matchCount == 5 && bonusMatched;
    }

    return this.matchCount == matchCount;
}

public static Rank from(int matchCount, boolean bonusBallMatched) {
    return Arrays.stream(values())
        .filter(rank -> rank != MISS)
        .filter(rank -> rank.matches(matchCount, bonusBallMatched))
        .findFirst()
        .orElse(MISS);
}

이렇게 하면 나중에 Rank가 추가될 때 from() 메소드 수정 없이 확장할 수 있다는 장점이 있는데, 현재 규모에서는 취향 차이에 가까워 보이긴 하네요 🤔

개선 제안

현재 잘못된 입력값이 들어오면 예외가 그대로 전파되면서 프로그램이 종료되는데요, 유효하지 않은 입력 시 재시도할 수 있게 처리하면 사용자 경험 측면에서 더 좋아질 수 있을 것 같습니다!

[chemistryx]

와일드카드 import가 있네요 👀

이전에 명준님 PR에 남긴 코멘트인데 참고하셔서 개선해보면 좋을 것 같습니다!
next-step/java-racingcar-simple-playground#181 (comment)

[Kdahyn]

앗 이런 게 있었군요😅 저도 인텔리제이에서 자동으로 import 해주다 보니 모르고 있었네요. 이참에 인텔리제이 설정도 하고 내용도 수정했습니다!

[chemistryx]

OutputView에서 정렬을 수행하도록 결정하신 이유가 있나요? Lotto가 생성될 때 정렬된 상태를 유지하거나 Lottos::toNumberLists()에서 정렬해서 넘겨주는 방식도 고려해볼 수 있을 것 같아서용

[Kdahyn]

처음에는 RandomNumberGenerator에서 정렬해서 생성했는데, 이후에는 정렬이 도메인보다는 출력 형식에 가깝다고 판단해서 OutputView로 옮겼습니다.
다만 말씀해주신 것처럼 로또 번호가 항상 정렬된 상태를 유지하면 View의 책임도 줄고 구조도 더 일관적일 수 있겠다고 생각되었습니다. 저는 Lotto 생성자에서 만드는 것이 좋다고 생각되어 한번 수정해봤는데 리뷰어님 생각은 어떠신가요?

[chemistryx]

좋은 판단인 것 같아요! 정렬이 출력 형식에 가깝다는 고민도 충분히 이해되는데, 로또 번호는 어떤 경로로 생성되든 항상 정렬된 상태가 자연스럽다는 점에서 Lotto 생성자에서 처리하는 게 맞는 방향이라고 생각해요. View가 데이터를 변환하는 책임을 갖지 않아도 되고, OutputView의 sortLotto() 메소드도 사라지니 더 깔끔해진 것 같네요 👍 👍

[chemistryx]

현재 메시지 포맷팅 책임이 DTO에 있는데, 나중에 Rank가 추가되면 이 부분도 함께 수정해야할 것 같아요. Rank에 메시지를 같이 정의해두고 여기서는 불러와서 사용하는 것은 어떨까요?
e.g.,

FIVE_BONUS_MATCH(5, 30000000, "5개 일치, 보너스 볼 일치"),
SIX_MATCH(6, 2000000000, "6개 일치"),
// ...

public String getDisplayName() { return displayName; }

[Kdahyn]

말씀해주신 방향대로 Rank로 메시지 생성 책임을 이동했습니다.
다만 displayName을 Rank에 두는 것이 domain이 출력 형식까지 가지는 것처럼 느껴졌습니다. 이런 정도의 표현 정보는 MVC 패턴을 해치는 것과는 다르게 봐도 괜찮을지 궁금합니다.

[chemistryx]

굉장히 합리적인 고민인 것 같아요. 엄격하게 보면 말씀하신대로 도메인이 출력 형식을 갖는 것 처럼 느껴지기도 하는데, 제가 생각했을 때 displayName은 등수를 어떻게 표현할 지를 결정하는 친구가 아니라 이 등수의 이름이 무엇인가에 가까운 것이라고 생각해요.

예를 들어 FIVE_BONUS_MATCH라는 enum 상수명 자체가 이미 의미를 담고 있고, displayName은 그걸 사람이 읽을 수 있는 형태로 표현한 것이니 도메인 지식의 일부로 봐도 무방할 것 같다고 생각해요!

[Kdahyn]

앗, 그렇게도 볼 수 있겠군요! 말씀해주신 관점대로 보면 displayName도 도메인 지식의 일부로 볼 수 있겠네요. 좋은 설명 감사합니다!

[chemistryx]

요 검증 메소드의 경우 this 필드에 접근하지 않으므로 static으로 선언해도 무방할 것 같은데 어떻게 생각하시나요?
++ LottoNumber내 선언된 검증 메소드도 동일해요~~

[Kdahyn]

앗, 그렇군요! 말씀해주신 대로 this 필드에 접근하지 않는 검증 메서드들은 static으로 변경했습니다.

[chemistryx]

키가 Rank인 경우에는 EnumMap 사용도 고려해볼 수 있을 것 같아요. 기존 HashMap에 비해서 성능이 더 좋고, 키가 Rank enum이라는 의도도 더 명확하게 드러낼 수 있을 것 같아요~

관련 아티클: [Java] EnumMap이란?

[Kdahyn]

EnumMap이라 것도 있었군요. WinningStatistics는 Rank를 key로 사용하는 구조라 더 잘 맞는다고 생각해 적용해봤습니다.

[Kdahyn]

좋은 리뷰 남겨주셔서 감사합니다, 수한님 😊
말씀해주신 두 부분을 반영해서 수정해봤습니다.

먼저 toLotto, toLottos는 말씀해주신 방향대로 각각의 객체에 정적 팩토리 메서드를 두는 방식으로 변환 책임을 이동했습니다. Controller에서 변환 로직이 빠지니 역할이 더 분명해졌고, 전체 흐름도 이전보다 읽기 쉬워진 것 같습니다.

두 번째로 입력 오류 시 재시도 로직도 적용해봤습니다. 구현하면서 별도 클래스로 분리할지 고민했는데, 현재는 입력 흐름을 제어하는 책임에 가깝다고 판단해서 Controller 내부에 두었습니다. 이 방향이 적절한지 궁금합니다.

추가로, 예외 메시지들이 현재 여러 객체에 흩어져 있는 상태라 이를 한곳에서 모아 관리하는 방식도 고려해봤습니다. 이 부분에 대해서도 수한님의 의견을 들어보고 싶습니다!

[chemistryx]

@Kdahyn 두 부분 모두 잘 반영해주셨네요 👍 👍
재시도 로직 위치에 대해서는 Controller에 두신 방향이 충분히 타당하다고 생각해요. InputView에서 처리하면 파싱 오류만 잡을 수 있는데, Controller에 둠으로써 비즈니스 로직 내에서 발생한 검증 예외도 잡을 수 있다는 점에서 좋다고 생각해요~

예외 메시지 관리는 각 도메인 객체가 자신의 검증 규칙과 메시지를 함께 소유하는 것도 응집도 측면에서는 나쁘지 않다고 생각해요. 현재 규모에서는 지금 방식으로 충분하다고 생각하고, 메시지가 많아지거나 다국어 지원(?) 같은 요구사항이 생기면 그때 분리를 고려해도 늦지 않을 것 같습니다!

이 외에도 미션 진행하시면서 궁금하셨던 점이나 다른 질문 사항 있으시면 언제든지 편하게 말씀해주세요~

[Kdahyn]

감사합니다! 말씀해주신 설명 덕분에 왜 Controller에서 재시도 로직을 두는 방향이 타당한지 더 명확하게 이해할 수 있었습니다.

이번 미션을 진행하면서 테스트 코드에 대해서도 고민이 많았습니다. 아직은 테스트 코드가 코드를 작성한 뒤 해야하는 과제처럼 느껴져서, 주로 예외가 발생하는 경우나 결과를 확인하는 테스트 위주로 작성해왔던 것 같습니다. 그러다 보니 테스트 코드를 작성했지만 활용하지 않고 직접 Application에서 실행해 보며 코드를 수정하게 됐습니다. 그래서 아직 “이 테스트를 왜 쓰는지”, “어떻게 활용해야 하는지”를 잘 체감하지 못하고 있습니다.

수한님은 테스트 코드를 작성하실 때 어떤 기준으로 테스트 대상을 정하시는지, 또 좋은 테스트 코드를 위해 특히 중요하게 보시는 점이 무엇인지 궁금합니다. 테스트를 실제로 설계와 리팩터링에 활용하는 방법도 함께 조언해주시면 감사할 것 같습니다!

p.s. 인텔리제이에서 테스트 코드 메서드명을 한글로 작성하면 커밋할 때 ASCII 관련 오류가 뜨던데, 혹시 해결할 방법이 있는지 궁금합니다.

[chemistryx]

@Kdahyn 테스트와 관련된 대현님의 고민에 저도 공감합니다 ㅎㅎ
테스트를 왜 쓰는지에 대해서는 저는 크게 두 가지 이유가 있다고 생각하는데요, 먼저 테스트를 작성하다 보면 자연스럽게 테스트하기 쉬운 형태로 코드를 설계하게 된다는 점이에요. 테스트가 어렵게 느껴진다면 해당 객체의 책임이 너무 크거나 의존성이 복잡하다는 뜻일 수도 있다고 생각해요.

두번째로는 지금 규모에서는 직접 실행해보는 게 더 빠르다고 느낄 수 있지만, 코드 규모가 커질수록 내가 수정한 부분이 예상하지 못한 다른 곳에 영향을 주는 경우가 생길 가능성이 높아지는데, 이때 테스트를 통해 사전에 식별할 수 있다는 장점이 있는 것 같아요.

테스트 대상의 경우 저는 주로 외부 의존성이 없는 도메인 객체부터 시작하는 것 같아요. 그리고 특히 엣지 케이스(경계값, 중복 등)에 관한 테스트를 중요하게 생각하는데요, 이 부분은 이미 대현님께서도 잘 작성해주신 것 같습니다 👍

활용 방법에 대해서는, 리팩터링할 때 효과를 가장 잘 느낄 수 있는 것 같아요. 코드를 변경한 뒤 테스트를 돌려서 기존 동작이 그대로인지 바로 확인할 수 있거든요. 애플리케이션 전체를 실행해서 직접 확인하는 대신 변경한 객체의 테스트만 돌리면 되니까 훨씬 빠르게 확인할 수 있어요. 설계 측면에서는 구현 전에 테스트를 먼저 작성해보는 것도 도움이 된다고 생각하는데요, 처음에는 익숙하지 않아서 어렵게 느껴질 수 있지만 지금 당장은 구현 후에라도 테스트를 작성하는 습관을 들이다 보면 자연스럽게 감이 잡힐 것 같아요 🤔

++ 테스트 코드 한글 메소드명 관련 문제는 두 가지 방법이 있는데요, 먼저 메소드명은 영어로 작성하고, @DisplayName 어노테이션을 활용하는 방법이 있어요.

@Test                                                                                                                                                                                                                            
@DisplayName("구입 금액이 1000원 단위가 아니면 예외가 발생한다")
void invalidAmountUnit() {                                                                                                                                                                                                       
    assertThatThrownBy(() -> new PurchaseAmount(1500))                                                                                                                                                                           
        .isInstanceOf(IllegalArgumentException.class);                                                                                                                                                                           
}      

두번째 방법은 클래스 선언 상단에 @SuppressWarnings("NonAsciiCharacters") 어노테이션을 추가하는 방법이에요.

@SuppressWarnings("NonAsciiCharacters")
class PurchaseAmountTest {
// ...
}

지금 상태에서는 두번째 방법이 더 간단해 보이긴 하는데, 한번 보시고 더 합리적인 방법으로 적용하시면 좋을 것 같습니다~!