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

Step3 구현 완료 (Step2 마지막 피드백 반영) #142

Merged
merged 18 commits into from
Mar 29, 2019

Conversation

CheolHoJung
Copy link

안녕하세요! 마지막 피드백 반영과 Step3 구현에 대한 피드백 요청합니다.

step3을 구현하기전에 리팩토링을 조금 진행했습니다.

  • 통계, 당첨순위 등의 클래스에서 Money, TicketCount를 멤버변수로 사용하도록 변경
    • 이 부분에서 Money와 TicketCount에 덧셈, 뺄셈, 곱셈, 나눗셈이 추가됐는데 통계를 구하는 부분의 복잡도만 증가하는 것 같아 굳이 필요할까 싶습니다.
  • 수익률에 대한 래퍼 클래스를 추가

로또 기계에서 랜덤한 상황을 테스트하기 위해 NumbersGenerator를 사용하는데, 이런 경우 일급 콜렉션의 규칙을 위반하는 걸까요? 수동 추첨 기능이 추가되면서 랜덤 번호를 제공하는 기능은 정적 클래스로 제공해도 되지않을까요?

Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰어 박현식입니다. :)
전체적으로 구현 너무 잘하셨어요 👍
피드백 몇가지 추가했구요, 말씀해주신 의견에 대해서 몇가지 제안도 추가했습니다.
확인해보시고 궁금하신 점 있으시면 DM 주세요. :)

public class LotteryNumber {
public class LotteryNumber implements Comparable<LotteryNumber> {

private static Map<Integer, LotteryNumber> cache = new HashMap<>();
Copy link

Choose a reason for hiding this comment

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

캐시 👍

src/main/java/lottery/domain/LotteryWinningStatistics.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/LotteryWinningStatistics.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/Money.java Outdated Show resolved Hide resolved
public final double rate;

public RevenueRate(Money revenue, Money myMoney) {
if (myMoney.amount == 0) {
Copy link

Choose a reason for hiding this comment

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

값객체의 내부값을 꺼내서 비교하는것보다는 값객체끼리 서로 비교하는게 더 좋지 않을까요?
예로 myMoney == Money.ZERO 혹은 Money.ZERO.equlas(myMoney)가 가능하겠네요.
아니면 적어도 직접 꺼내 비교하는 것 보다는 객체에 메시지를 보내서(매서드를 사용해서) 일치하는지 확인하는게 더 좋을 것 같습니다.

src/main/java/lottery/machine/LotteryMachine.java Outdated Show resolved Hide resolved
- 생성자 private으로 변경, valueOf와 of 정적 팩토리 메소드 추가
- next-step#142 (comment)
- Money와 TicketCount 사이의 변환은 LotteryMachine에서 처리
- 당첨 순위와 당첨된 티켓의 개수에 따라 당첨금액을 반환하는 부분은
LotteryRank에서처리
- 사칙연산의 파라미터를 원시타입이 아닌 객체로 변경
- final 멤버변수 직접 접근이 아닌 getter 이용
- next-step#142 (comment)
- LotteryMachine -> 로또 구매만 담당하는 LotteryVendingMachine로 변경
- 티켓의 일급콜렉션으로 추첨을 담당하는 LotteryRaffleMachine 추가
- next-step#142 (comment)
- next-step#142 (comment)
@CheolHoJung
Copy link
Author

피드백 적용했습니다. 크게 바뀐 부분은 마지막에 조언해주신 부분이네요.
구매기계와 추첨기계로 나눠봤습니다. 감사합니다 :)

Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰어 박현식입니다.
값객체 활용 상당히 잘하신 것 같아요 👍
3단계 머지했구요. 4단계 진행하시면 될 것 같습니다. :)

@phs1116 phs1116 merged commit 6d74898 into next-step:CheolHoJung Mar 29, 2019
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.

None yet

2 participants