-
Notifications
You must be signed in to change notification settings - Fork 6
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
로또 2차 수동 미션 제출입니다. #8
base: main
Are you sure you want to change the base?
Conversation
this.lottos = Collections.unmodifiableList(new ArrayList<>(lottos)); | ||
this.lottoCount = lottoCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collections.unmodifiableList를 사용하셨군요!
무슨 기능을 하는 지 몰랐는데 이번 기회에 공부할 수 있어서 좋았습니다.
저는 이번에 Java를 배우기 시작하면서 이전에 몰랐던 새로운 요소들을 봤을 때, 그리고 썼을 때 항상 굳이? 라는 생각을 먼저 하고 시작하는데요, 혹시 여기서 unmodifiableList를 사용하신 이유가 궁금합니다 !
private static List<Lotto> createManualLottos(List<String> manualLotto, LottoCount count) { | ||
return manualLotto.stream() | ||
.map(Lotto::of) | ||
.collect(Collectors.toList()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메소드에서 count는 사용하고 있지 않네요.
private static void DisposeBall() { | ||
for (int ballNumber = MINIMUM_LOTTO_NUMBER; ballNumber <= MAXIMUM_LOTTO_NUMBER; ballNumber++) { | ||
MAPPING_BALL.put(ballNumber, new LottoNumber(ballNumber)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또 넘버들을 초기화하는데 Map을 사용하셨군요, 좋은 아이디어 같습니다!
오늘 세션에서처럼 이같은 경우 Map을 사용하면 중복을 제거하는 로직을 넣지 않아도 된다는 이점이 있을 것 같네요.
하지만 for문을 통해 횟수를 45회로 제한한다면 이와 같은 기능이 충분이 잘 동작할 수 있을까요?
만약 중간에 중복된 값이 들어간다면 44개의 공만 저장되나요?
45개의 숫자가 확실하게 들어온다고 가정한다면 Map을 쓸 이유가 있을까요?
THREE_MATCH(new MatchCount(3), false, new Money(5_000)), | ||
FOUR_MATCH(new MatchCount(4), false, new Money(50_000)), | ||
FIVE_MATCH(new MatchCount(5), false, new Money(1_500_000)), | ||
FIVE_MATCH_WITH_BONUS_BALL(new MatchCount(5), true, new Money(30_000_000)), | ||
SIX_MATCH(new MatchCount(6), false, new Money(2_000_000_000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_로 읽기쉽게 구분할 수 있었군요..!
좋은 것 배워갑니다 ! 👍
THREE_MATCH(new MatchCount(3), false, new Money(5_000)), | ||
FOUR_MATCH(new MatchCount(4), false, new Money(50_000)), | ||
FIVE_MATCH(new MatchCount(5), false, new Money(1_500_000)), | ||
FIVE_MATCH_WITH_BONUS_BALL(new MatchCount(5), true, new Money(30_000_000)), | ||
SIX_MATCH(new MatchCount(6), false, new Money(2_000_000_000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum값에서 1, 3번째 값은 새로운 객체를 생성해서 초기화 한다는 게 흥미롭네요!!
하지만 입력받는 값이나 전달받는 값이 아닌 상수를 정의한다는 점에서 굳이 이런 방식을 사용하신 이유가 있나요?
궁금합니다 👍
public float getProfitRate() { | ||
float totalPrize = lottoResult.calculateTotalPrize(); | ||
Money money = Money.getMoney(count); | ||
return totalPrize / money.getMoney(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 실수형을 사용할 때 최대한 float대신 double을 사용하고있습니다.
실수형의 default는 double이기 때문에요!
public boolean isProfit() { | ||
if (getProfitRate() < PROFIT_THRESHOLD) { | ||
return false; | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메소드를 도대체 왜 작성하셨지 하면서 천천히 생각해보니까 1이상일 때는 해당하는 메세지를 출력하면 안되는 것이었군요 ㅠㅠ 세세한 부분까지 캐치하신 거 대단하시네요 ~!
public Printer(){ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않으셨군요
수동 미션하느라 수고하셨습니다 :) 코드 리뷰하는 동안 여러 부분에서 배워갑니다, 감사합니다 ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 👍
리뷰 이외에도 테스트가 작성되지 않은 부분이 몇몇 있는것 같아요.
테스트 커버리지 한번 확인해주세요 🙂 (getter 같은 부분까지 테스트 할 필요는 없습니다.)
Printer printer = new Printer(); | ||
Receiver receiver = new Receiver(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Printer printer = new Printer(); | |
Receiver receiver = new Receiver(); | |
private final Printer printer = new Printer(); | |
private final Receiver receiver = new Receiver(); |
main이니 큰 문제는 없겠지만, 그래도 오용을 막기위해선 private final을 붙여주는게 안전할것 같네요 🙂
public class Lottos implements Iterable<Lotto> { | ||
|
||
private final List<Lotto> lottos; | ||
private final LottoCount lottoCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lottoCount를 인스턴스 변수로 갖고 있어야만 할까요?
인스턴스 변수의 개수는 가능한 최소한으로 유지하는게 좋습니다.
(이유에 대해서도 생각해볼까요? 🤔)
private static final int MINIMUM_MANUAL_LOTTO_COUNT = 0; | ||
private final int manualLottoCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static final int MINIMUM_MANUAL_LOTTO_COUNT = 0; | |
private final int manualLottoCount; | |
private static final int MINIMUM_MANUAL_LOTTO_COUNT = 0; | |
private final int manualLottoCount; |
🙂
} | ||
|
||
public boolean hasManualLottoCount() { | ||
return manualLottoCount != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0을 NO_COUNT 정도로 표현해줄수 있지 않을까요?
1차 미션 진행하다가 새로 해보자는 생각에 시도했다가 이제서야 완성이 되었네요.
늦어서 죄송합니다;(