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 #3251

Open
wants to merge 2 commits into
base: geatrigger
Choose a base branch
from
Open

Step3 #3251

wants to merge 2 commits into from

Conversation

geatrigger
Copy link

step3 2등 추가 요구사항 추가하였습니다!

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.

안녕하세요 기찬님, 3단계 진행 고생하셨습니다.
2단계 코멘트가 누락된 것들이 몇 개 보이는데요!
이번 단계 리팩터링을 진행하면서 함께 고민해주시면 좋겠습니다.
궁금하신 점 코멘트, DM 남겨주세요 🙇‍♂️

import java.util.Scanner;

public class BonusBallInputView implements InputView<BonusBall> {
Scanner scanner = new Scanner(System.in);
Copy link
Member

Choose a reason for hiding this comment

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

#3099 (comment)

기찬님 견해가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

매번 새로운 인스턴스를 생성하는 건 비효율적인 것 같네요
생성자에서 입력받아 사용하도록 바꿀게요!

Comment on lines +14 to 15

public static List<Lottery> buy(Integer price, LotteryStrategy lotteryStrategy) {
Copy link
Member

Choose a reason for hiding this comment

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

#3099 (comment)

로또금액을 VO 로 포장하면 더 신뢰할 수 있는 값이 되겠네요.

Comment on lines +10 to +12
public LotteryNumber value() {
return value;
}
Copy link
Member

Choose a reason for hiding this comment

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

더욱 더 VO 스럽게 사용하기 위해 동등성을 부여해주면 어떨까요? (equals & hash code)

Comment on lines 3 to 9
public enum WinPrize {
FIRST_PLACE(2000000000),
SECOND_PLACE(1500000),
THIRD_PLACE(50000),
FOURTH_PLACE(5000),
SECOND_PLACE(30000000),
THIRD_PLACE(1500000),
FOURTH_PLACE(50000),
FIFTH_PLACE(5000),
LOST(0);
Copy link
Member

Choose a reason for hiding this comment

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

enum 을 통해서 상금과 당첨 개수, 보너스볼 여부도 관리해볼 수 있을까요? 🤔

Comment on lines 31 to 37
List<Integer> winLotteryNumbers = new ArrayList<>();
for (int i = 0; i <= Lottery.LENGTH; i++) {
for (int i = 0; i < RANK_LENGTH; i++) {
winLotteryNumbers.add(0);
}
for (int number : numberOfMatchNumbers) {
winLotteryNumbers.set(number, winLotteryNumbers.get(number) + 1);
for (int i = 0; i < numberOfMatchNumbers.size(); i++) {
checkBonusBallAndAdd(hasBonusBalls.get(i), numberOfMatchNumbers.get(i), winLotteryNumbers);
}
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.

3 participants