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

[로또 main2] 박성우 제출합니다 #5

Open
wants to merge 12 commits into
base: main2
Choose a base branch
from
Open

Conversation

seong-wooo
Copy link
Member

@seong-wooo seong-wooo commented Apr 3, 2023

첫 번째 단계랑 비슷해서 큰 어려움 없이 구현했네요!

재밌었습니다.

리뷰로 저를 혼내주세요.

많은 공격 부탁드립니다.

toString을 출력용으로 사용한 것이 조금 아쉽지만
출력을 위한 로직을 작성하는 것이 너무 귀찮아서 toString으로 진행했슴다

Copy link

@ray-yhc ray-yhc left a comment

Choose a reason for hiding this comment

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

main 1에서 부자연스럽게 느껴졌던 부분들이 다 개선되었네요!!
Picker, LottoNumber 등을 만들어서 적용하신 게 인상적이었습니다! 수고하셨습니다~

}

private static int getAnInt(String input) {
return Integer.parseInt(input);
Copy link

Choose a reason for hiding this comment

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

NumberFormatException 에 대한 예외처리가 있으면 좋을것같네요

Suggested change
return Integer.parseInt(input);
try {
return Integer.parseInt(input);
} catch (NumberFormatException ne) {
throw new IllegalArgumentException("[ERROR] 정수를 입력해주세요.");
}

final int manualLottoCount = Input.getManualLottoCount();
validateLottoCount(money, manualLottoCount);

final List<List<Integer>> manualLottos = Input.getManualLottos(manualLottoCount);
Copy link

Choose a reason for hiding this comment

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

image

manualLottoCount = 0인 경우에도 "수동으로 구매할 번호를 입력해 주세요."가 출력됩니다!

Comment on lines +25 to +27
final Ticket manualTicket = getManualTicket(manualLottos);
final int autoLottoCount = money.remainCount(manualLottoCount);
final Ticket autoTicket = Ticket.of(new AutoLottoCreator(new RandomPicker()), autoLottoCount);
Copy link

Choose a reason for hiding this comment

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

manualTicket은 Lotto.create(...)를 사용하고, autoTicket은 Ticket.of(...) 를 사용하네요. 티켓의 생성방식이 다를 뿐이지, 같은 역할을 하는 메소드이니 같은 객체 내에서 처리되도록 하면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

맞네요 이 부분이 조금 부자연스럽네요


public class Input {

private static final String DELIMITER = ", ";
Copy link

Choose a reason for hiding this comment

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

6,5,4,3,2,1로 입력하면 에러가 발생합니다. parseInt에서 공백은 처리가 가능한 걸로 알고 있는데, DELIMITER = ","로 바꿔보면 어떨까요?

Suggested change
private static final String DELIMITER = ", ";
private static final String DELIMITER = ",";

System.out.println("당첨 통계");
System.out.println("---------");
System.out.println(ranks);
System.out.printf("총 수익률은 %.2f입니다.", money.getProfitRate(ranks.getProfit()));
Copy link

Choose a reason for hiding this comment

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

money.getProfitRate(ranks.getProfit())) 는 Application에서 계산한 뒤, double 값으로 넘겨주는 게 어떨까요? 출력과 계산이 같은 곳에 있는 것이 부자연스럽습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇군요.. 뷰 로직에 많이 신경을 못써서 그런가바여 ㅠ

Copy link

Choose a reason for hiding this comment

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

LottoNumber 클래스를 만들 생각은 못했네요! 좋은 아이디어인 것 같습니다

return ranks.entrySet()
.stream()
.filter(entry -> entry.getKey().isNotOtherRank())
.sorted(Entry.comparingByKey())
Copy link

Choose a reason for hiding this comment

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

image

순서가 요구사항과 맞지 않네요!

final int manualLottoCount = Input.getManualLottoCount();
validateLottoCount(money, manualLottoCount);

final List<List<Integer>> manualLottos = Input.getManualLottos(manualLottoCount);
Copy link
Member

Choose a reason for hiding this comment

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

LottoNumber를 구현하셨는데, 여기서는 그냥 Integer를 사용하신 이유가 있으신가요??

Copy link
Member

Choose a reason for hiding this comment

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

24라인과 25라인을 묶어서 manualTicket을 생성하는 메소드를 따로 빼서 만드는게 가독성이 더 좋을 것 같습니다! manualLottos는 getManualTicket에서만 사용되기도 하구요

private static final List<LottoNumber> VALUES;

static {
VALUES = IntStream.rangeClosed(MIN_VALUE, MAX_VALUE)
Copy link
Member

Choose a reason for hiding this comment

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

옹,, 그러면 VALUES는 1부터 45까지 LottoNumber 객체들이 들어있는 리스트가 되는건가요?

Copy link
Member Author

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