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

[또링] 로또 미션 제출합니다! #162

Merged
merged 60 commits into from
Feb 26, 2020
Merged

Conversation

jnsorn
Copy link

@jnsorn jnsorn commented Feb 20, 2020

안녕하세요. 모르는 부분이 많은데, 많이 리뷰해주시면 열심히 수정해보겠습니다! 잘부탁ㄷ립니다~☺

 1. 정수만 입력 가능
 2. 음수와 0은 입력 불가능
 3. 1,000원 단위로만 입력 가능
 4. 최대 구입 가능 금액 100,000원 제한
 1. 생성자 대신 valueOf로 변경
 2. 테스트코드의 생성자를 valueOf로 수정
Copy link

@jeonghoon1107 jeonghoon1107 left a comment

Choose a reason for hiding this comment

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

안녕하세요~! 전체적으로 구현 잘하셨어요 💯
몇가지 피드백 남겼으니 확인해주세요 :)

int numberOfLotto = inputLottoMoney.calculateNumberOfLotto();
printPurchaseCompleteMessage(numberOfLotto);

List<Lotto> lottos = lottoController.purchaseLotto(numberOfLotto);

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.

말씀해주신대로 List를 가지고 있는 Lottos 일급 컬렉션을 생성하였습니다. 하지만 Lottos를 생성하여 얻는 이점은 List 타입 필드의 불변성을 보장한다는 것 밖에 깨닫지 못했습니다. 😂 혹시...! 이 점 말고 일급컬렉션 사용을 추천해주신 다른 이유가 있다면 듣고 싶습니다!😊

List<Lotto> lottos = lottoController.purchaseLotto(numberOfLotto);
printPurchasedLotto(lottos);

Lotto winningLotto = new Lotto(LottoParser.parser(inputWinningLottoNumber()));

Choose a reason for hiding this comment

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

winningLotto클래스를 만들어보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분 수정하였습니다 :)


Lotto winningLotto = new Lotto(LottoParser.parser(inputWinningLottoNumber()));
LottoNumber bonusLottoNumber = LottoNumber.valueOf(inputBonusLottoNumber());
Map<LottoRank, Integer> lottoRankCount =

Choose a reason for hiding this comment

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

Result를 담당하는 클래스를 따로 만들면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분 수정하였습니다!

}
}

private void validateDuplication(List<LottoNumber> lottoNumbers) {

Choose a reason for hiding this comment

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

Set을 이용하면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

Set을 이용하게 되면 사용자로 하여금 이 자료구조가 중복이 안된다는 것을 명시적으로 표현할 수 있고, 따로 중복 검사를 수행하지 않아도 되는 장점을 가질 수 있겠네요! 해당 부분 수정하겠습니다. 😊

public class LottoMoney {
private static final long ZERO = 0;
private static final long UNIT = 1000;
private static final long MAX_BOUND = 100000;

Choose a reason for hiding this comment

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

_를 이용하면 가독성을 높일 수 있어요!

추가적으로 max에 대한 요구사항이 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

따로 요구사항은 없지만 현실에서는 1인 1회차 10만원 한도라는 룰이 있어서 적용해보았습니다! 하지만 요구사항에 명시되지 않은 내용을 구현하는 것은 개발자의 욕심이라는 생각도 드네요... 해당 부분 수정하겠습니다 :)

enum내에서는 _로 구분을 하였는데 여기서는 깜빡한 것 같습니다😓 조언해주신대로 _를 붙여 가독성을 높이는 방향으로 수정하겠습니다!

Copy link

@kingbbode kingbbode left a comment

Choose a reason for hiding this comment

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

안녕하세요!
코드가 잘 작성되었네요 :)
1단계 머지하겠습니다!

Comment on lines +7 to +18
public class Lottos implements Iterable<Lotto> {
private List<Lotto> lottos;

public Lottos(List<Lotto> lottos) {
this.lottos = new ArrayList<>(lottos);
}

@Override
public Iterator<Lotto> iterator() {
return lottos.iterator();
}
}

Choose a reason for hiding this comment

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

일급컬렉션으로서 하는 역할이 없네요 :)
그렇다면 List<Lotto> 를 사용하는게 더 편하지 않을까요?
의미없는 객체 포장은 불필요해보여요.

@kingbbode kingbbode merged commit 6b86ff0 into woowacourse:jnsorn Feb 26, 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
Development

Successfully merging this pull request may close these issues.

4 participants