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

[로또] 박성우 제출합니다. #1

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

[로또] 박성우 제출합니다. #1

wants to merge 16 commits into from

Conversation

seong-wooo
Copy link
Member

view 로직이 진짜 구리네요.

특히 마지막 최종 결과 출력할때는 감이 안와서 대충 구현했어요.

여러분의 의견을 받아보겠습니다.

제 코드가 어떻게해야 더 깔끔해질까요?

Copy link

@jeongyuneo jeongyuneo left a comment

Choose a reason for hiding this comment

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

enum 클래스에서 인터페이스 사용이 인상 깊었어요.
많이 배워갑니당👍🏻

Comment on lines +28 to +32
System.out.println("\n당첨 번호를 입력해 주세요.");
final Lotto winLotto = new Lotto(InputView.requestWinNumbers());

System.out.println("\n보너스 번호를 입력해 주세요.");
int bonusBall = InputView.requestInteger();

Choose a reason for hiding this comment

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

이 부분도 모두 입력받는 로직 같은데 InputView가 아닌 Application에서 구현하신 이유가 있으실까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Application의 실행 흐름을 모두 Application class에 작성하였는데.. 딱히 이유는 없습니다

System.out.println(String.format("\n%d개를 구매했습니다.", lottoCount));

final List<Lotto> lottos = getAutoLottos(lottoCount);
lottos.forEach(System.out::println);

Choose a reason for hiding this comment

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

InputView는 선언하시고, OutputView는 선언하지 않으신 이유가 있으실까요?
애플리케이션 중간중간 출력하는 로직이 노출돼서 책임이 모호해지는 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

OutputView를 만든다고해서 출력하는 로직이 크게 달라질 것 같지않아서.,,?
view에는 많이 신경 안썼습니다 ㅠㅠ

import java.util.List;
import java.util.stream.Collectors;

public class InputView {

Choose a reason for hiding this comment

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

유틸성 클래스는 기본 생서자를 private로 선언해 객체 생성을 막아주세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니다. 이런 클래스를 잘 선언하지 않아서 기본 생성자를 추가하는게 어색하네요

Comment on lines +11 to +14
public static int requestInteger() {
final String input = Console.readLine();
return parseInt(input);
}

Choose a reason for hiding this comment

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

requestIngeter를 구매금액과 보너스번호 입력에서 같이 사용하고 있는데, 둘 중 하나라도 int가 아닌 타입으로 변경된다면 에러가 발생하지 않을까요?
두 입력에 대한 책임을 분리하는 것이 좋다고 생각하는데 어떻게 생각하시나요ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

맞는 얘기인 것 같아요!
그런데 이 미션에서는 view 관련 로직보다는 객체지향적인 코드에 집중하려고 합니다.
분리하는게 살짝 귀찮네여
살짝 핑계 대봤습니다.

Comment on lines +16 to +22
public static List<Integer> requestWinNumbers() {
final String input = Console.readLine();

return Arrays.stream(input.split(DELIMITER))
.map(InputView::parseInt)
.collect(Collectors.toList());
}

Choose a reason for hiding this comment

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

InputView 클래스에서 입력이 아닌 책임도 같이 갖고 있어서 어색한 것 같아요.
그리고 InputView라고 했는데 Input을 받기 위해 사용되는 메시지들은 전부 Application에서 보여주고 있네요?

Copy link
Member Author

Choose a reason for hiding this comment

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

view 관련 로직은 어느게 맞는지 잘 모르겠어요..

Comment on lines +52 to +54
assertSimpleTest(() -> assertThatThrownBy(() -> runException("1000j"))
.isInstanceOf(IllegalArgumentException.class)
);

Choose a reason for hiding this comment

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

👍🏻

Comment on lines +15 to +19
private void validateBonusBall(Lotto lotto, int bonusBall) {
if (!lotto.isPossible(bonusBall)) {
throw new IllegalArgumentException(String.format("[ERROR] %d는 보너스 볼이 될 수 없습니다.", bonusBall));
}
}

Choose a reason for hiding this comment

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

저는 보너스 볼에 대한 검증을 로또에 요청했는데, 보너스 볼에 대한 검증은 보너스볼에서 하는게 더 책임이 명확해보이네요!

Comment on lines 9 to 18
FIRST(2_000_000_000, WinNumbers::isFirstRank),
SECOND(30_000_000, WinNumbers::isSecondRank),
THIRD(1_500_000, WinNumbers::isThirdRank),
FOURTH(50_000, WinNumbers::isFourthRank),
FIFTH(5_000, WinNumbers::isFifthRank),
OTHER(0, WinNumbers::isOutOfRank);


private final int prize;
private final RankValidator rankValidator;

Choose a reason for hiding this comment

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

인터페이스를 이렇게도 쓸 수 있네요! 👀

Comment on lines 39 to 43
return String.format("3개 일치 (%,d원) - %d개\n", FIFTH.getPrize(), ranks.get(FIFTH))
+ String.format("4개 일치 (%,d원) - %d개\n", FOURTH.getPrize(), ranks.get(FOURTH))
+ String.format("5개 일치 (%,d원) - %d개\n", THIRD.getPrize(), ranks.get(THIRD))
+ String.format("5개 일치, 보너스 볼 일치 (%,d원) - %d개\n", SECOND.getPrize(), ranks.get(SECOND))
+ String.format("6개 일치 (%,d원) - %d개\n", FIRST.getPrize(), ranks.get(FIRST));

Choose a reason for hiding this comment

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

각 등수 기준은 LottoResult에서 갖고 있으면 더 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

맞죠,,, 그거 되게 고민하다가 이렇게 대충 구현했네요. 조금 더 고민하고 push 해볼게요!

Comment on lines 9 to 14
FIRST(2_000_000_000, WinNumbers::isFirstRank),
SECOND(30_000_000, WinNumbers::isSecondRank),
THIRD(1_500_000, WinNumbers::isThirdRank),
FOURTH(50_000, WinNumbers::isFourthRank),
FIFTH(5_000, WinNumbers::isFifthRank),
OTHER(0, WinNumbers::isOutOfRank);

Choose a reason for hiding this comment

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

뒷북같지만..prize에 0이 많이 들어가서 언더바(_)로 가독성이 높아진 것 같아서 좋은 것 같습니다!

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

@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.

너무 깔끔해서 결과계산 관련한 부분 말고는 덧붙일 말이 없었네요.. 그래서인지 코멘트를 적게 썼습니다 ㅜㅜ ㅋㅋㅋ 수고 많으셨습니다~

  1. Stream을 정말 잘 활용하시네요!! 성우님 코드 볼때마다 stream 학습 욕구가 생기는 것 같아요 ㅋㅋ

로또 결과 계산 & 출력 관련 로직이 여러 클래스에 흩어져 있는 것 같습니다.
LottoRank, WinNumbers, RankValidator, Lotto가 로또 결과 계산, 수익 계산의 책임을 나누어 갖고 있네요

특히, 로또 일치 숫자 수는 Lotto 클래스에서 다루는 반면, 보너스숫자는 WinNumbers 클래스에서 다루고 있어 한눈에 찾기 어려웠던 것 같아요!
또 만약 게임 룰이 바뀌거나, SixthRank가 생긴다면 여러 클래스를 모두 손봐야 할 것 같구요

System.out.println(String.format("\n%d개를 구매했습니다.", lottoCount));

final List<Lotto> lottos = getAutoLottos(lottoCount);
lottos.forEach(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.

List에 forEach를 쓸 수 있군요...!!

}

public static LottoRanks from(List<LottoRank> ranks) {
EnumMap<LottoRank, Integer> lottoRankMap = new EnumMap<>(LottoRank.class);
Copy link

Choose a reason for hiding this comment

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

EnumMap이라는 게 있었군요!! 딱 제가 찾던 녀석입니다 ㅜㅜ

Copy link

@jeongyuneo jeongyuneo left a comment

Choose a reason for hiding this comment

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

몇 가지 코멘트 남깁니다!


@Override
public String toString() {
return String.format("%d개 일치%s (%,d원)", this.count, this.bonus ? ", 보너스 볼 일치" : "", this.prize);

Choose a reason for hiding this comment

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

3항 연산자를 쓰지 않는다.

위 요구사항을 만족하지 못하는 것 같습니다!

public String toString() {
return ranks.entrySet()
.stream()
.map(entry -> String.format("%s - %d개", entry.getKey().toString(), entry.getValue()))

Choose a reason for hiding this comment

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

Suggested change
.map(entry -> String.format("%s - %d개", entry.getKey().toString(), entry.getValue()))
.map(entry -> String.format("%s - %d개", entry.getKey(), entry.getValue()))

Comment on lines +34 to +37
return ranks.entrySet()
.stream()
.map(entry -> String.format("%s - %d개", entry.getKey().toString(), entry.getValue()))
.collect(Collectors.joining("\n"));

Choose a reason for hiding this comment

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

LottoRank의 모든 값들에 대해 출력하고 있어, 당첨되지 않은 내역도 같이 출력되는 것 같습니다!

@seong-wooo seong-wooo removed the help wanted Extra attention is needed label Apr 3, 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.

3 participants