Skip to content

[로또] 김난슬 미션 제출합니다.#4

Open
seulnan wants to merge 20 commits into
Java-JavaScript-Language-Stuty:mainfrom
seulnan:seulnan
Open

[로또] 김난슬 미션 제출합니다.#4
seulnan wants to merge 20 commits into
Java-JavaScript-Language-Stuty:mainfrom
seulnan:seulnan

Conversation

@seulnan

@seulnan seulnan commented Feb 21, 2025

Copy link
Copy Markdown

리뷰하러 와주셔서 감사합니다 ^~^ 로또당첨같이 뜻깊고 얻을 것도 많은 스터디잖아? 야호!

🌟 내가 신경써서 구현한 부분

  • 저번부터 리뷰로 받은 enum을 활용하고자했습니다
  • 프로그래밍요구사항을 반영하고자 굉장히 노력했습니다..메소드도 책임대로 최대한 분리했어요
  • domain과 dto를 활용해서 명확하게 역할을 구분하고자했습니다
  • 도메인 단위테스트를 추가했습니다

❤️‍🔥 피드백이 필요한 부분

  • validator의 책임 분리기준이 애매해요 몇번이고 붙였다가분리했다가..무슨 환승연애도 아니고ㅠㅠㅠ 하나의 파일에 두자니 너무 뚱뚱하고 분리하자니 통일성이 없어보여요..다른 분들의 의견이 궁금합니다
  • 그외에도 객체지향적인&가독성좋은 코드를 짜려고 노력을 많이 했는데 진짜 괜찮은지..너무 오버엔지니어링한 것 같기도하고.. 최대한 재사용이 될 수 있냐? 나중에 확장될 가능성이 있냐?로 판단했습니다

🔍 내가 고려한 예외 케이스

검증 항목 고려한 예외
구입 금액 입력 입력이 숫자가 아닐 경우
1000원 단위가 아닐 경우
당첨 번호 입력 숫자가 아닌 값 입력
1~45 범위를 벗어나는 숫자 입력
중복된 숫자가 포함될 경우
입력값이 부족할 경우
보너스 번호 입력 숫자가 아닌 값 입력
1~45 범위를 벗어나는 숫자 입력
당첨 번호와 중복될 경우
게임 로직 예외 로또 구매 개수가 0개일 경우
LottoRank에서 정의되지 않은 매칭 개수가 들어올 경우 (이론적으로 불가능하지만 안전장치)

- `InputView`: 사용자 입력 시 한 줄 띄우기 추가하여 가독성 개선
  - 구입 금액, 당첨 번호, 보너스 번호 입력 후 개행 추가
- `OutputView`: 당첨 결과 출력 형식 조정
  - 당첨 금액을 천 단위 콤마로 출력하도록 개선
  - 당첨 통계 출력 후 한 줄 띄우기 추가하여 가독성 향상
- `LottoController`: 사용자 입력을 받아 게임을 진행하는 기능 구현
  - `LottoService`를 통해 로또 번호 생성 및 당첨 결과 계산
  - UI 로직을 최소화하여 서비스 계층과 분리
- `LottoService`: 게임 진행을 위한 핵심 로직 구현
  - 구입 금액에 맞춰 로또 티켓 생성
  - 당첨 번호와 비교하여 결과 계산
- UI 로직을 제거하고 순수한 비즈니스 로직으로 분리
- `LottoTicket`: 로또 티켓 객체로 생성 후 관리
  - 랜덤으로 6개의 숫자를 생성하여 Lotto 객체로 저장
- `WinningLotto`: 당첨 번호 및 보너스 번호 검증 기능 추가
  - LottoValidator를 사용하여 검증 후 저장
  - 보너스 번호가 중복되지 않도록 체크
- `LottoValidator`: 로또 번호 검증 기능 추가
  - 1~45 범위의 숫자 6개가 포함되어 있는지 확인
  - 중복된 숫자가 없는지 검증
- `InputValidator`: 사용자 입력값 검증 기능 추가
  - 구입 금액이 1000원 단위인지 확인
  - 숫자가 아닌 값 입력 시 예외 발생
  - 보너스 번호가 당첨 번호와 겹치지 않도록 검증
- `LottoRank`: 당첨 개수 및 보너스 여부에 따라 등수 결정
  - 6개 일치 → 1등, 5개+보너스 → 2등, 5개 일치 → 3등 등
  - `findRank()`를 통해 적절한 등수를 반환하도록 구현
- 당첨 금액을 천 단위 콤마로 출력하도록 개선
- `LottoResultCalculator`: 로또 당첨 결과를 계산하는 유틸리티 클래스 추가
  - `WinningLotto`와 비교하여 당첨 등수를 계산하고 결과를 `LottoResultDto`로 반환
  - `EnumMap`을 사용하여 `LottoRank`별 당첨 개수를 효율적으로 저장
- `NumberUtil`: 총 수익률을 소수점 둘째 자리에서 반올림하는 기능 추가
  - `BigDecimal`을 활용하여 반올림 정확도를 보장
- `LottoResultDto`: 당첨 결과를 저장하고 수익률을 계산하는 DTO 구현
  - EnumMap을 사용하여 `LottoRank`별 당첨 횟수를 효율적으로 저장
  - 총 상금을 계산하여 수익률을 반환하는 메서드 추가
- UI와의 의존성을 줄이고 비즈니스 로직과 분리
- `OutputView`: 당첨 결과를 낮은 등수(3개 일치)부터 높은 등수(6개 일치) 순으로 출력하도록 변경
- 코드 스타일 개선: Indent depth 2 이하 유지 및 메서드 길이 15줄 이하로 제한
  - `formatLottoRank()` 메서드 분리하여 출력 코드 단순화
  - `printResults()`에서 직접 처리하던 등수 정렬 및 출력 로직을 분리하여 가독성 개선
- `getValidatedInput()` 메서드를 추가하여 중복된 `while (true)` 반복 로직 제거
- `InputValidator`의 검증 메서드를 함수형 인터페이스를 활용해 동적으로 호출하도록 변경
- `validateNumberRange()` 메서드를 추가하여 범위 검증 로직을 일괄 처리
- `parseInteger()` 및 `parseIntegerList()` 메서드를 추가하여 예외 처리 일괄 적용
- `validateBonusNumber()` 및 `validateWinningNumbers()`에서 중복된 예외 처리 제거 및 코드 가독성 향상
- `WinningLotto`가 `String`이 아닌 `List<Integer>`를 직접 받도록 수정
- 컨트롤러에서 검증된 데이터를 그대로 사용하여 중복 검증을 제거
- 유지보수성을 높이고, `InputValidator`에서 모든 검증을 수행하도록 책임 분리
- `WinningLotto`는 DB 엔티티가 아닌 핵심 도메인 객체이므로 `domain` 패키지로 이동
- 비즈니스 로직을 포함하는 객체이므로 `entity`보다 `domain`이 더 적절
- ORM 도입 시 `entity` 패키지를 JPA 엔티티 관리 용도로 활용 가능
- 패키지 변경으로 유지보수성과 코드 명확성 향상
- `NUMBER_FORMAT`으로 상수 네이밍 변경하여 컨벤션 준수
- `Arrays.stream()` 대신 `List.of().stream()` 사용하여 불필요한 변환 제거
- `formatLottoRankString()` 메서드를 분리하여 중복 코드 제거 및 가독성 향상
- `printProfitRate()` 메서드 분리하여 `printResults()` 가독성 개선
daeGULLL added a commit to 2025JavaGroupStudy/java-lotto-7 that referenced this pull request Feb 27, 2025

@daeGULLL daeGULLL left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상세한 테스트랑 엄청 깔끔한 컨트롤러 코드가 인상적이었습니다!! 그리고 모델에서 내부 디렉토리로 도메인이랑 dto 나누는 것 등등 여러개 배워갑니다...최후의 과제 화이팅해요ㅠㅠ

++ validator는 하나로 다 몰아넣어도 괜찮다구 생각해요 보니 별로 뚱뚱하진 않던데용??

throw new IllegalArgumentException("[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미 만들어놓은 validator를 재사용해도 좋을 것 같아요!!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 이게 기존코드에 에러를 던지는 코드가 있더라구요 저도 이걸 validator로 따로 빼려다가 그냥 기존코드를 유지하는 방식이 더 좋을것같아서 남겨놨었어요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

근데 지금 생각해보니까 중복코드라 그냥 validator를 재사용하는 것이 좋을듯합니다! 좋은 의견 감사해요 @daeGULLL

List<Integer> numbers = new ArrayList<>(Randoms.pickUniqueNumbersInRange(LOTTO_MIN_NUMBER, LOTTO_MAX_NUMBER, LOTTO_SIZE));
Collections.sort(numbers);
return new Lotto(numbers);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LottoTicket에서 Lotto를 생성하면 두 도메인이 분리되기 어렵다보니 Lotto 내부에서 팩토리 패턴으로 생성하는 방안을 써보는걸 추천드려용

만약 저렇게 바꾸신다면 static 변수들도 lotto로 옮기셔도 될듯? static이 붙은 클래스 내 변수는 인스턴스 변수가 아니라 조건에 위배안되더라구요

.setScale(2, RoundingMode.HALF_UP)
.doubleValue();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

한자리수 표시 기능을 따로빼는거보다 전 string format의 %.1f 로 기능을 구현했는데, 이게 더 간단한 것 같아서 알려드림당 요 넘버 유틸 사용하는게 더 편하다 하심 이렇게 하셔도 될듯

@seulnan seulnan Feb 28, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀해주신 방식도 충분히 좋은 방법이네요,, 구현당시에는 나중에 기능이 확장되거나 형식이 바뀔 가능성을 고려해서 유틸로 분리했었어요. 하지만 단순히 출력용 변환이라서 말씀해주신 방식이 더 적절할 것 같네요! 😊 연서님 의견 덕분에 더 간결한 방법을 고민해볼 수 있었어요. 좋은 피드백 감사합니다! @daeGULLL

int amount = parseInteger(input);
if (amount <= 0 || amount % 1000 != 0) {
throw new IllegalArgumentException("[ERROR] 구입 금액은 1,000원 단위여야 합니다.");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 에러 메세지들도 enum으로 빼서 관리하는게 더 좋을것 같아용

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants