Skip to content

Step4 #4156

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

Open
wants to merge 7 commits into
base: youlalala
Choose a base branch
from
Open

Step4 #4156

wants to merge 7 commits into from

Conversation

youlalala
Copy link

안녕하세요.

수정 사항 요약

  1. Lotto가 List 대신 Set을 가지도록 수정
    ㄴ 2단계에서 받은 피드백을 반영

  2. LottoNumber 클래스를 새로 추가
    ㄴ 3단계까지는 자동 로또만 존재했기 때문에 1~45 숫자 캐싱은 LottoNumberGenerator 내에서만 관리했음.
    ㄴ 그러나 수동 로또 입력이 도입되면서 숫자 유효성 검증과 관리가 분리되면 좋겠다고 판단해, LottoNumber 클래스를 별도로 분리하고 정적 캐싱 방식으로 구현

  3. 2등 당첨 로직 표현 방식을 개선
    ㄴ 3단계에서 피드백 받은 2등 당첨의 표현방식이 다른 것을 적용

  4. 입력 버퍼 관련 문제를 해결하기 위해 readInt() 메서드를 추가
    ㄴ 기존의 nextInt()는 숫자 뒤의 \n이 버퍼에 남아 있어 이후 입력에 영향을 주는 문제가 있었음.
    ㄴ이를 해결하기 위해 readInt() 메서드를 만들어 nextLine()과 함께 처리.

감사합니다 :)

Copy link
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

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

안녕하세요 유라님! 마지막 단계도 잘 진행해주셨습니다 👍
PR 에 구현해주신 내용을 이목요연하게 정리해주신 덕분에 코드리뷰가 매우 편리하네요 😄

동작 상 조금 더 보완해볼 부분이 보이는데요,

image

이미 구현역량이 뛰어나신 분이라 따로 피드백은 생략하겠습니다 😄
궁금하신 점 편하게 질문주세요!

Comment on lines +33 to 36
private void validateLottoNumbers() {
if (lottoNumbers.size() > LOTTO_NUM_COUNT) {
throw new IllegalArgumentException("로또 번호는 6개여야 합니다.");
}
Copy link
Member

Choose a reason for hiding this comment

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

별건 아니지만! 이런 경우엔 lottoNumber.size() != LOTTO_NUM_COUNT 가 그대로 유지되어도 괜찮을거 같아요 😄

Comment on lines -32 to 39
if (uniqueNumbers.size() != LOTTO_NUM_COUNT) {
if (lottoNumbers.size() < LOTTO_NUM_COUNT) {
throw new IllegalArgumentException("로또 번호는 중복될 수 없습니다.");
Copy link
Member

Choose a reason for hiding this comment

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

여기서는 요 표현식이 가장 정확하겠네요 👍

Comment on lines +6 to +20

public class LottoNumber {

static final int LOTTO_MIN_NUM = 1;
static final int LOTTO_MAX_NUM = 45;

public static final Map<Integer, LottoNumber> lottoNumberMap = IntStream.rangeClosed(LOTTO_MIN_NUM, LOTTO_MAX_NUM)
.boxed()
.collect(HashMap::new, (map, number) -> map.put(number, new LottoNumber(number)), HashMap::putAll);

private final int number;

private LottoNumber(int number) {
this.number = number;
}
Copy link
Member

Choose a reason for hiding this comment

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

LottoNumber vo 가 생겼고, 이에 대해 캐싱해주셨군요 👍
보통 LottoNumber 에 대한 캐싱 값이 어딨는지 알고 싶을 땐 LottoNumber 클래스파일을 먼저 열어볼테니, 좋은 코드 배치라고 생각합니다 😄 👍

Comment on lines +9 to +10
static final int LOTTO_MIN_NUM = 1;
static final int LOTTO_MAX_NUM = 45;
Copy link
Member

Choose a reason for hiding this comment

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

접근 제한자를 default(package private) 을 선택해주셨네요!
요렇게 접근제한자를 선택하신 이유가 궁금해요!

Comment on lines +9 to +25

@Test
void of_1에서_45사이_숫자면_같은_객체_반환한다() {
assertThat(LottoNumber.of(2)).isEqualTo(LottoNumber.of(2));
}

@Test
void of_1미만일_경우_예외를_던진다() {
assertThatThrownBy(() -> LottoNumber.of(0))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
void of_45초과일_경우_예외를_던진다() {
assertThatThrownBy(() -> LottoNumber.of(46))
.isInstanceOf(IllegalArgumentException.class);
}
Copy link
Member

Choose a reason for hiding this comment

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

경계값을 이용한 꼼꼼한 테스트 💯

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