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

Step2 로또(자동) #3234

Open
wants to merge 9 commits into
base: letitgobaby
Choose a base branch
from
Open

Conversation

letitgobaby
Copy link

안녕하세요 !
로또 (자동) 단계 진행하여 리뷰 요청 드립니다 !

Copy link

@mskangg mskangg left a comment

Choose a reason for hiding this comment

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

2등 미션도 잘 구현해주셨네요!
다만, 1등~4등 로또 당첨 테스트코드가 보이지 않는것 같습니다 🤔
그리고 전체적으로 클래스 내부변수를 사용하여 set,get 하는 방식에 익숙하신것 같아요!
객체에게 메세지를 보내어 비즈니스를 구현하도록 리팩토링해보시기 바랍니다!
몇가지 피드백 남겨드렸고 확인부탁드립니다 🙇

Comment on lines +20 to +22
- [x] 로또 구입 금액을 입력하면 구입 금액에 해당하는 로또를 발급해야 한다.
- [x] 로또 1장의 가격은 1000원이다.
Copy link

Choose a reason for hiding this comment

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

💯

Comment on lines +10 to +17
private int purchasedPrice = 0;
private List<Integer> prices = Arrays.asList(
0, 0, 5000, 50000, 1500000, 2000000000
);

private int totalEarningPrice = 0;

private List<Integer> winners;
Copy link

Choose a reason for hiding this comment

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

출력을 담당하는 View 클래스인데, 다른 상태들도 갖고있네요!
로또에서의 당첨결과, 통계는 비즈니스 도메인 클래스로 추출할 수 있어보이고, 당첨로또번호도 클래스로 분리할 수 있어보이네요 🤔
LottoOutputView 클래스는 출력의 책임만 갖고있도록 해보아요!

Comment on lines +9 to +13
private final int lottoPrice = 1000;

private int amount;
private int lottoCount;
private List<Integer> winNumbers = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

여기도 같은 리뷰입니당 :)
입력의 책임만 갖을 수 있게 리팩토링해주세용

Comment on lines +13 to +16
List<Integer> mockNumbers = new ArrayList<>();
for (int i = 1; i <= 45; i++) {
mockNumbers.add(i);
}
Copy link

Choose a reason for hiding this comment

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

매번 리스트를 생성하곘네요 🤔
1-45 숫자를 캐싱해보는건 어떠신가요?

Comment on lines +4 to +5

public interface NumberGenerator {
Copy link

Choose a reason for hiding this comment

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

@FuntionalInterface 를 선언할 수 있습니당


public class LotteryContainer {

// private NumberGenerator generator;
Copy link

Choose a reason for hiding this comment

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

불필요한 주석은 삭제해주세용 :)

Comment on lines +12 to +13
private boolean isWinnerCheck = false;
private int matchCount = 0;
Copy link

Choose a reason for hiding this comment

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

내부변수가 과연 필요한지 고민해봐야할것 같아요 🤔
평소 클래스설계를 할 때 어떤 기준으로 내부변수를 정의 하시나요??

Comment on lines +70 to +72
lotto.setWinNumbers(winNumbers);

int matchCount = lotto.getMatchCount();
Copy link

Choose a reason for hiding this comment

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

lotto에서 matchCount를 꺼내 활용하는것 보단

Suggested change
lotto.setWinNumbers(winNumbers);
int matchCount = lotto.getMatchCount();
int matchCount = lotto.match(winNumbers);

메세지를 보내어 객체지향으로 구현하시는건 어떠신가요?

Comment on lines +20 to +23
for (Lottery lotto : lottoContainer.getTotalLottery()) {
System.out.println(lotto.getLottery());
}
System.out.println("");
Copy link

Choose a reason for hiding this comment

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

출력의 책임이 Main 에 남아있네요 🤔

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