Skip to content
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

feat : 로또 2등 추가 #3952

Open
wants to merge 1 commit into
base: seulpi
Choose a base branch
from
Open

feat : 로또 2등 추가 #3952

wants to merge 1 commit into from

Conversation

seulpi
Copy link

@seulpi seulpi commented Oct 23, 2024

리뷰요청드립니다.

Copy link
Member

@nooose nooose left a comment

Choose a reason for hiding this comment

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

안녕하세요 슬비님
잘 구현하셨습니다!
코멘트 남겼어요 확인 후 재요청 부탁드려요

Comment on lines +26 to 34
public LottoDetailMatch countMatches(Lotto winningNumbers, int bonusNumber) {
int matchCount = (int) numbers.stream()
.filter(winningNumbers.getNumbers()::contains)
.count();

boolean isMatchedBonusNumber = bonusNumber != 0 && numbers.contains(bonusNumber);

return new LottoDetailMatch(matchCount, isMatchedBonusNumber) ;
}
Copy link
Member

Choose a reason for hiding this comment

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

LottoDetailMatch을 통해서 일치한 수를 반환한 것도 좋아보이지만,
작성하신 LottoRank 를 반환하도록 만들어보면 코드 흐름이 자연스럽게 흘러갈 것으로 보이네요 😄

.collect(Collectors.toList());
return countMatch(matchResult);
}
// 순위에 맞게 몇개 일치하는지 갯수를 센다
Copy link
Member

Choose a reason for hiding this comment

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

TODO나 FIXME 같은 주석 태그가 아니라면 주석을 지양하는 것이 좋을 것 같아요

메소드 네이밍만으로 해석이 가능하도록 작성하는 것이 첫번째이고,
그래도 부족하다면 java doc 문법을 활용해서 메소드를 설명하는 것이 좋다고 생각합니다.

    /**
     * 테스트 메소드
     */
     public test() {
     }

image

@@ -27,11 +28,11 @@ public static void gameStart() {
lottoPrint.lottoPurchasedCount(lottoTickets);
Copy link
Member

Choose a reason for hiding this comment

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

image

로또 번호가 정렬되어있어야 하지 않을까요? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

image

로또 번호(16)와 보너스 번호(16)가 중복되는 상황이네요
이 부분도 확인 부탁드립니다!

Copy link
Member

Choose a reason for hiding this comment

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

image

요구사항에는 없지만 일반적인 상황은 아닌 것 같습니다.

@@ -23,10 +23,14 @@ public Set<Integer> getNumbers() {
return this.numbers;
Copy link
Member

Choose a reason for hiding this comment

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

모든 원시값과 문자열을 포장

로또안에 있는 Integer도 1~45 숫자만 허용하는 객체로 만들어보면 좋겠습니다.
그럼 Lotto가 가진 책임을 덜어낼 수 있겠네요

Copy link
Member

Choose a reason for hiding this comment

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

로또에 대한 단위 테스트가 약간 부족하다고 느껴집니다.

사소한 단위 테스트도 학습 차원에서 만들어 보면 좋을 것 같아요 😀

  • 숫자 범위 검증
  • 번호 6개가 아닐 때 예외 검증
  • ...


private final int count;
private final String rank;
private final long amount;
private final boolean requireBonus;
Copy link
Member

Choose a reason for hiding this comment

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

보너스 필드가 추가되었네요 👍👍

, THIRD_PRIZE (5, "3", 1500000, false)
, FOURTH_PRIZE (4, "4", 50000, false)
, FIFTH_PRIZE (3, "5", 5000, false)
, NON_WINNER (0, "낙첨", 0, false);

private final int count;
private final String rank;
Copy link
Member

Choose a reason for hiding this comment

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

rank 문자열은 view에 의존적이지 않을까요?

@nooose
Copy link
Member

nooose commented Oct 24, 2024

이번 미션 커밋은 하나였는데요
이번 피드백 이후부터 커밋 단위를 작게 나눠보시면서 작업하면 좋을 것 같습니다.

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