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

[럿고] 로또 미션 제출합니다. #2

Merged
merged 33 commits into from
Jun 25, 2020

Conversation

ksy90101
Copy link

와... 레벨1을 열심히 안했나 보네요 ㅠㅠ
예상외로 시간이 많이 걸렸어요 ...ㅎㅎ
최대한 한다고 했는데, 부족한 부분이 있다면 마음껏 까주세요 ㅎㅎ
그럼 고생하세요!

}

public static LottoTicket createRandomSixBalls() {
final Random random = new Random();
Copy link

Choose a reason for hiding this comment

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

Copy link

@kimevanjunseok kimevanjunseok left a comment

Choose a reason for hiding this comment

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

안녕하세요!!!럿고!!
리뷰가 많이 느져서 죄송합니다...
럿고 코드를 보면서 배울 점이 많이 있었습니다.
개인의 주관이 있는 피드백이니 깊게 생각 안하셔도 될 것 같습니다.😊😊

}
}

public static List<Integer> numberListConverterBy(final String value) {
Copy link

@kimevanjunseok kimevanjunseok Jun 24, 2020

Choose a reason for hiding this comment

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

README 실행결과보면 "1, 2, 3, 4, 5, 6" 이렇게 띄어쓰기 주던데... 저렇게 주면 숫자취급도 못 받네요...
그리고 정렬도 해주던데....
괜찮아요!! 요구사항에 그런 말은 없었으니까요!!!😂😂

Copy link
Author

Choose a reason for hiding this comment

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

ㅎㅎㅎ 정렬은 생각을 못했네요... ㅎㅎ 그리고 trim을 빼먹었네요... ㅎㅎ

Money money = createMoney();
CreationCount creationCount = createCreationCount(money);

return new User(money, creationCount, Stream.concat(

Choose a reason for hiding this comment

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

이렇게 Stream 하는거 처음봤어요!!!😮

return new CreationCount(money, Converter.numberConverterBy(inputManualCount));
}

private List<LottoTicket> createAutomaticLottoTickets(final int automaticCount) {

Choose a reason for hiding this comment

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

createAutomaticLottoTicketscreateManualLottoTickets메서드 둘다 이렇게 하셨던데 for문을 안 쓴 이유가 있나요? 성능상 for문이 좋을 것같아서요!!!(별차이 없겠지만....)
단순한 호기심입니다!!!

Copy link
Author

Choose a reason for hiding this comment

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

스트림 공부를 해보고 싶......
성능은 jason의 말이 생각나네요
우리의 컴퓨터는 생각 이상의 성능을 가지고 있다! 였나요...?ㅎㅎ

Comment on lines 19 to 21
private BallFactory() {

}

Choose a reason for hiding this comment

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

Suggested change
private BallFactory() {
}
private BallFactory() {
}

헤헷😊

Copy link
Author

Choose a reason for hiding this comment

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

매의눈 @_@

public static LottoTicket createRandomSixBalls() {
final Random random = new Random();

return random.ints(1, 46).

Choose a reason for hiding this comment

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

앗 상수로 안 빼도 되는건가요!?!?!?

}

public static LottoTicket createManualSixBalls(List<Integer> ballNumbers) {
validateBallNumbersSizeNotSix(ballNumbers);

Choose a reason for hiding this comment

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

이건 개인적인 생각입니다!!!
Factory가 검증을 하는 것이 어색하게 느껴집니다!!!😊

Copy link
Author

Choose a reason for hiding this comment

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

맞네요 로또 티켓으로 가야겠어요 ㅎㅎ

Copy link

@YerinCho YerinCho left a comment

Choose a reason for hiding this comment

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

럿고 안녕하세요~!
그동안 까먹고(?) 몰랐던 걸 알아가는 코드였어요!
제 식견이 부족한 이유로 만족스러운 피드백이 될진 모르겠지만,,,
의견 남기고 갑니다 :)


private final static int MIN_LOTTO_TICKET_COUNT = 0;

private final Logger logger = Logger.getLogger("logger");

Choose a reason for hiding this comment

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

👍


return random.ints(1, 46).
distinct()
.limit(6)

Choose a reason for hiding this comment

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

선언된 상수를 써도 될 것 같아요!

}

private void validateOverSix(List<Ball> balls) {
if (balls.size() != 6) {

Choose a reason for hiding this comment

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

6이 이곳 저곳에서 계속 쓰이는데 LottoTicket이 위치한 패키지를 변경하고 Ball에 선언되어 있는 상수를 같이 사용하는건 어떨까요 ??
이 객체도 어차피 List<Ball> 의 일급 컬렉션이니까요!


public CreationCount(final Money money, final int manualCount) {
Objects.requireNonNull(money);
this.totalCount = money.getMoney() / TOTAL_COUNT_PER;

Choose a reason for hiding this comment

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

Money 객체에서 get해서 사용하지 않고 Money에 메시지를 던질 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

총 카운트를 구하는 것은 CreationCount의 역할이라고 생각해서 get을 해서 로직을 구현했습니다!
오렌지의 생각이 궁금하네요~

Choose a reason for hiding this comment

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

저는 계산하는 로직은 money값이 가장 중요한 역할을 한다고 생각해서 전부 Money객체가 하도록 설계했어요!
그리고 로또 한 장의 가격이 바뀌게 되면 TOTAL_COUNT_PER 와 MONEY_UNIT 모두 바뀌어야 하니까 Money가 이 로직을 담당하게 되면 수정에 대한 범위도 작아지지 않을까 생각이 듭니다!

this.automaticCount = totalCount - manualCount;
}

private void validateGreaterManualCountThanTotalCount(int totalCount, int manualCount) {

Choose a reason for hiding this comment

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

Suggested change
private void validateGreaterManualCountThanTotalCount(int totalCount, int manualCount) {
private void validateManualCountAndTotalCount(int totalCount, int manualCount) {

로도 충분하지 않을까요...? 결과적으로는 두 값의 유효성을 확인하는 메소드니까요!

Copy link
Author

Choose a reason for hiding this comment

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

명확하게 명시하고 싶었습니다. 이 유효성 검사는 총 횟수보다 수동 횟수가 더 큰 경우라서요.. ㅎ

return true;
}

return lottoTicket.getBalls()

Choose a reason for hiding this comment

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

규칙 9: 게터/세터/프로퍼티를 쓰지 않는다

lottoTicket에 winningBalls를 넘겨주면 해결이 가능할 것 같아요


@DisplayName("숫자가 아닌 다른 값이 들어왔을 경우 예외처리")
@ParameterizedTest
@ValueSource(strings = {"가", "a", "A", "-"})

Choose a reason for hiding this comment

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

빈 값 테스트케이스도 있으면 좋을 것 같아요~


@DisplayName("1 ~ 45의 로또 볼이 생성되는지 확인하는 테스트")
@ParameterizedTest
@MethodSource("rangeProvider")

Choose a reason for hiding this comment

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

잊고 있던 사용법을 이렇게 다시 알아갑니다 👍


import lotto.console.domain.ticket.LottoTicket;

public class WinningBallsTest {

Choose a reason for hiding this comment

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

보너스볼에 대한 중복체크 테스트가 없네요!

Copy link
Author

Choose a reason for hiding this comment

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

헉... 까먹었네요 ㅠㅠ


public static Ball of(final int ballNumber) {
return balls.stream()
.filter(ball -> ball.getNumber() == ballNumber)

Choose a reason for hiding this comment

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

규칙 9: 게터/세터/프로퍼티를 쓰지 않는다

@ksy90101
Copy link
Author

여러분들 덕분에 코드가 깔끔해졌네요~ 감사합니다!

Copy link

@YerinCho YerinCho left a comment

Choose a reason for hiding this comment

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

피드백 반영한 거 확인 했습니다 :)
질문에 대한 의견 남겼습니다~

}

private void validateOverSix(final List<Ball> balls) {
if (balls.size() != 6) {

Choose a reason for hiding this comment

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

상수가 놀아요!

Copy link

@kimevanjunseok kimevanjunseok left a comment

Choose a reason for hiding this comment

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

럿고 역시 잘하시네요!!!
한번 또 보다가 물어보고 싶은 내용 남겼습니다!!
고생하셨습니다!!!


public CreationCount(final Money money, final int manualCount) {
Objects.requireNonNull(money);
this.totalCount = money.getMoney() / TOTAL_COUNT_PER;

Choose a reason for hiding this comment

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

totalCount가 필요한가요?? 검증할 때만 사용하는 것 같아요!! (럿고가 마음에 안 들어서 그런거 아니예요...😂😂)

Copy link
Author

Choose a reason for hiding this comment

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

횟수를 의미하는 도메인으로 총 횟수도 가지고 있어야겠다고 생각했어요! 사실, 검증용이긴 하네요 ㅎㅎ
좀 더 고민해볼께요!

@ksy90101 ksy90101 merged commit 81c48ba into Woohan-TDD:tdd-rutgo Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants