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

Step3 - 로또(2등) #3199

Merged
merged 4 commits into from
May 15, 2023
Merged

Step3 - 로또(2등) #3199

merged 4 commits into from
May 15, 2023

Conversation

gisungPark
Copy link

리뷰 잘 부탁드립니다.

HAE\1161893 added 2 commits May 13, 2023 23:59
- 보너스 번호 추가
- 로또 2등 당첨 여부 확인
Copy link

@lxxjn0 lxxjn0 left a comment

Choose a reason for hiding this comment

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

기성님, 피드백 반영하시느라 수고하셨습니다!

이번 단계는 크게 변경되는 부분은 없어서 크게 피드백을 드릴 부분은 없었네요. 단지 몇 가지 부분에 대해서 한번 더 확인해보시면 좋을 것 같아 리뷰 남겨놨습니다. 확인 부탁드려요 :)

src/main/java/step2/domain/LottoResultReport.java Outdated Show resolved Hide resolved
src/main/java/step2/domain/LottoResultReport.java Outdated Show resolved Hide resolved
src/main/java/step2/domain/LottoResultReport.java Outdated Show resolved Hide resolved
Comment on lines 37 to 39
if(count == 5 && compareTarget.isContain(bonusNumber)) {
return Rank.SECOND;
}
Copy link

Choose a reason for hiding this comment

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

지금은 2등을 Rank 클래스가 아닌 LottoTicket에서 판단하고 있어요. 이렇게 된다면 책임과 역할이 다른 두 클래스에 분산되고 있는 상황인데 해당 로직을 Rank가 가질 수 있도록 해보면 어떨까요?

Copy link
Author

@gisungPark gisungPark May 14, 2023

Choose a reason for hiding this comment

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

당첨 번호와 클라이언트 티켓간의 검증의 역할을 LottoTicket 클래스가 가지고 있다고 생각합니다.
만약 Rank enum 클래스가 2등을 판단하는 역할을 가지게 된다면, 해당 판단을 위한 티켓 번호 데이터를 가져야 할텐데요.
Rank 클래스 내에 아래와 같은 메서드가 필요하게 될 것 같으며, 이는 Rank 의 역할에서 벗어나는것 같습니다.
다른 방법이 있다면 피드백 부탁드리겠습니다.
감사합니다.

public Rank isSecond (int bonusNumber) {
    if ( this != THIRD ) return this;
    // 보너스 번호 포함 여부 확인을 위한 로직 필요
}

Copy link

Choose a reason for hiding this comment

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

만약 Rank 클래스에 일치하는 숫자의 개수와 보너스 번호의 일치 여부를 넘겨준다면 어떨까요? 그렇다면 Rank 클래스에서 티켓 번호 데이터를 가지고 있지 않더라도 가능하지 않을까요?

예를 들어 SECOND의 경우 (matchCount, bonusMatch) -> matchCount == 5 && bonusMatch라는 predicate 필드를 가지고 있다면 티켓의 데이터가 없더라도 Rank는 2등 여부를 식별할 수 있으며 보너스 번호와 일치하는 번호가 있는 지를 판단하는 역할은 로또 티켓에게 그대로 유지시켜줄 수 있으니 적절하게 대응이 될 것 같아요!! 지금의 코드는 로또의 순위를 정하는 로직을 LottoTicket에 가져와서 책임과 역할이 이상했던 것 같아요!!

HAE\1161893 added 2 commits May 15, 2023 01:04
- 상수 제거
- HashMap 메서드 활용
- 클라이언트 로또 티켓이 당첨 번호와 보너스 번호로 비교 검증 진행
Copy link

@lxxjn0 lxxjn0 left a comment

Choose a reason for hiding this comment

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

이번 단계에서는 특별히 반영해야 하는 피드백은 따로 없을 것 같아서 4단계를 진행하면서 좀 더 다양한 부분에 대해 피드백 해보면 좋을 것 같아요!!

앞선 피드백에서 적절한 상수 사용의 예시를 참고할 수 있는 다른 리뷰 링크를 첨부해뒀어요!! 이 부분 한번 참고해보시면 좋을 것 같고 추가로 Rank 클래스에서 보너스 숫자 유무로 등수를 판단할 수 있는 아이디어를 간단하게나마 전달드려봤는데 다음 단계 미션을 진행하시면서 같이 반영해봐주셔도 좋을 것 같습니다 👍🏼

Comment on lines +16 to +18
Integer value = lottoResultReport.getOrDefault(rank, 0);
lottoResultReport.put(rank, value + 1);
return lottoResultReport.get(rank);
Copy link

Choose a reason for hiding this comment

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

@lxxjn0 lxxjn0 merged commit 06308c1 into next-step:gisungpark May 15, 2023
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