민형 로또(수동) 구현 #12

Gobukgol · 2020-05-26T10:39:02Z

클래스	설명
LottoNumberCache	로또 번호들을 캐싱하는 LottoNumber 내부 정적 클래스
LottoNumber	로또 번호
NumberGenerator	LottoNumberGenerator가 사용할 정수 리스트를 만들어주는 인터페이스, 인자로 정수 리스트의 최솟값과 최댓값을 받음
ShuffledNumberGenerator	NumberGenerator를 구현하는 클래스로 생성된 리스트를 셔플까지해서 반환
LottoNumberGenerator	자동으로 인자로 받은 갯수만큼 LottoNumber를 반환하거나, 인자롤 받은 정수 리스트를 LottoNumber로 변환하여 반환.
LottoTicket	LottoNumber를 감싸는 일급컬렉션 정정 팩토리 메소드로 생성
LottoTickets	LottoTicket의 일급 컬렉션, LottoWinningTicket을 인자로 받아 가지고있는 Ticket 추첨해서 LottoResult로 반환
LottoMachine	로또 티켓을 발급
ManualNumbersDto	수동으로 입력한 정수 리스트를 가지고있음.
LottoWinningTicket	지난주 당첨번호와 보너스 번호를 가지는 클래스
WinningSheet	로또 상금 정보를 가지는 Enum, 맞은 갯수와 보너스 일치 여부로 맞는 상금 정보를 반환
LottoResult	로또 추첨 결과를 가지고 있는 클래스, 로또 당첨 결과를 Map으로 관리
LottoMoney	구매금액으로 자동과 수동 티켓수를 계산하고 수익률 계산

피드백 반영하고 최종 요구사항까지 구현하였습니다.
피드백 반영하면서 NumberGenerator 인터페이스의 이름을 고민하다가 실패하였습니다.

malibinYun · 2020-05-26T13:45:05Z

src/main/java/com/javabom/lotto/domain/vo/LottoMoney.java

+    private static final int LOTTO_PRICE = 1000;
+    private final int purchaseAmount;
+    private final int numberOfTicket;


돈이 로또가격과 티켓의 개수를 가지고있는게 어색해보여
이건 LottoMachine 쪽에 있는게 나은거같아

malibinYun · 2020-05-26T13:50:49Z

src/main/java/com/javabom/lotto/LottoApplication.java

+
+        OutputView.printLottoTickets(lottoTickets);
+
+        List<Integer> winningNumbers = InputView.inputLastWinningNumbers();


WinningNumbers는 일급컬렉션으로 묶는게 좋아보여
그럼 LottoNumberGenerator에 있는 public static LottoNumber findByNumber(int number) 메소드가 없어도 될거같아.

malibinYun · 2020-05-26T13:51:07Z

src/main/java/com/javabom/lotto/domain/WinningSheet.java

+public enum WinningSheet {
+    FAIL(0, 0),
+    FOURTH(3, 5000),
+    THIRD(4, 50000),
+    SECOND(5, 1500000),
+    FIRST(6, 2000000000);
+


로또 보너스 번호가 없는거같아!

malibinYun · 2020-05-26T13:52:09Z

src/main/java/com/javabom/lotto/LottoApplication.java

+
+        OutputView.printNumberOfTicket(lottoMoney);
+
+        List<LottoTicket> lottoTickets = LottoMachine.purchaseLottoTicket(lottoMoney);


List은 일급컬렉션으로 묶는게 좋아보여!

pci2676 · 2020-05-27T10:22:22Z

src/main/java/com/javabom/lotto/domain/number/LottoNumberGenerator.java

+    static {
+        lottoNumbers = IntStream.rangeClosed(LOTTO_NUMBER_UNDER_BOUND, LOTTO_NUMBER_UPPER_BOUND)
+                .mapToObj(LottoNumber::new)
+                .collect(Collectors.toList());
+    }


싱글턴!!
내 개인적인 생각인데 Interger 클래스 까보면 내부에 static class 로 IntergerCache가 있단말이야?
그래서 LottoNumber도 LottoCache같은 거를 내부에 static class로 들고 있고
로또 넘버를 생성하는 거를 부르면 LottoCache에서 미리 생성된 싱글턴들을 반환해 주는건 어떨까??
LottoNumber의 생성자가 열려있어서 싱글턴하게 사용되지 않을수 있을거 같은 생각이 들어서!
LottoNumber의 생성자는 private으로 닫아두고 해당 private 생성자를 이용해서 static class에 cache를 생성해두고
of 같은 정적 팩토리 메서드로 캐싱된 값을 가져올수 있게 하는거지!

pci2676 · 2020-05-27T10:51:13Z

src/main/java/com/javabom/lotto/domain/LottoMachine.java

+    public static List<LottoTicket> purchaseLottoTicket(LottoMoney lottoMoney) {
+        List<LottoTicket> lottoTickets = new ArrayList<>();
+
+        for (int i = 0; i < lottoMoney.getNumberOfTicket(); i++) {
+            List<LottoNumber> lottoNumbers = LottoNumberGenerator.generateLottoNumbers();
+            lottoTickets.add(new LottoTicket(lottoNumbers));
+        }
+
+        return lottoTickets;
+    }


LottoTicket이 노출될 필요가 있나? 라는 생각이 들었어!
LottoNumberGenrator가 굳이 List<LottoNumber>를 줘야하나? 라는 생각에서
LottoTicket을 반환해 줘도 문제 없지 않을까? 라는 생각이 들어

pci2676 · 2020-05-27T10:53:07Z

src/main/java/com/javabom/lotto/domain/number/LottoNumberGenerator.java

+    public static List<LottoNumber> generateLottoNumbers() {
+        Collections.shuffle(lottoNumbers);


Collections.shuffle(lottoNumbers); 라는 부분은 우리가 제어할수 없는 부분이잖아?
제어할 수 없는 부분에 대한 의존성은 분리해 놓는게 좋지 않을까?

pci2676 · 2020-05-27T11:03:52Z

src/main/java/com/javabom/lotto/domain/LottoResult.java

+    private Map<WinningSheet, Long> makeWinningStatistics(List<Integer> drawResults) {
+        Map<WinningSheet, Long> winningStatistics = new LinkedHashMap<>();
+
+        for (WinningSheet winningSheet : WinningSheet.values()) {
+            long count = drawResults.stream()
+                    .filter(drawResult -> winningSheet.getMatchCount() == drawResult)
+                    .count();
+
+            winningStatistics.put(winningSheet, count);
+        }
+
+        return winningStatistics;
+    }


이 부분은 winningSheet에게 책임을 더 넘겨보는건 어떨까?

long count = drawResults.stream() .filter(drawResult -> winningSheet.getMatchCount() == drawResult) .count();

특히 이 부분..!!

pci2676 · 2020-05-27T11:05:02Z

src/main/java/com/javabom/lotto/domain/LottoResult.java

+                .reduce(Integer::sum)
+                .orElse(0);
+
+        double rateOfReturn = profit / lottoMoney.getPurchaseAmount();


이 부분도 LottoMoney가 해줄수 있지 않을까?

pci2676 · 2020-05-27T11:09:45Z

src/main/java/com/javabom/lotto/domain/DrawingMachine.java

+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class DrawingMachine {


우승 번호의 일급컬렉션 역할이라 위 이름이 그다지 어울리지 않는것 같아!

pci2676 · 2020-05-27T11:13:53Z

src/main/java/com/javabom/lotto/domain/DrawingMachine.java

+    private void validateDuplicateWinningNumbers(List<Integer> winningNumbers) {
+        Set<Integer> nonDuplicateNumbers = new HashSet<>(winningNumbers);
+
+        if (nonDuplicateNumbers.size() != WINNING_NUMBER_SIZE) {


nonDuplicateNumbers.size() 가 7이어도 이 메서드는 실패하는 건데 중복은 아닌거잖아?
validateWinningNumbersSize 메서드가 먼저 호출되서 지금은 검증이 되겠지만
엄연히 validateDuplicateWinningNumbers 메서드만 호출했을때는 잘못된 로직아닐까?

pci2676 · 2020-05-27T11:14:50Z

src/main/java/com/javabom/lotto/domain/LottoResult.java

+import java.util.List;
+import java.util.Map;
+
+public class LottoResult {


이 객체에서 객체간의 의존성을 더 잘게 쪼개고 책임을 분리해보는게 좋을것 같아

refactor(LottoMachine): 수동 번호와 자동 개수를 받도록 변경

pci2676

오~~~ 전체적으로 깔끔하게 구현된 것 같아

pci2676 · 2020-06-04T11:05:38Z

src/main/java/com/javabom/lotto/domain/vo/LottoMoney.java

+    public LottoMoney(int purchaseAmount) {
+        validatePurchaseAmount(purchaseAmount);
+        this.numberOfAutoTicket = purchaseAmount / LOTTO_PRICE;
+    }


사용되지 않는 생성자네!

pci2676 · 2020-06-04T11:15:37Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+    public static LottoTicket ofFixed(List<Integer> numbers) {
+        validateSize(numbers);
+        return new LottoTicket(LottoNumberGenerator.generateFixedNumber(numbers));
+    }
+
+    public static LottoTicket ofAuto() {
+        return new LottoTicket(LottoNumberGenerator.generateRandomNumber(LOTTO_NUMBERS_SIZE));
+    }


LottoNumberGenerator의 의존성이 LottoTicket 내부에 숨겨져있어서 테스트하기가 힘들것 같아
fixed 티켓은 테스트할수 있지만 auto 티켓은 이 상태로는 테스트하기가 힘들것 같아보여

pci2676 · 2020-06-04T11:17:41Z

src/main/java/com/javabom/lotto/domain/ticket/LottoWinningTicket.java

+    public LottoTicket getWinningTicket() {
+        return winningTicket;
+    }
+
+    public LottoNumber getBonusNumber() {
+        return bonusNumber;
+    }


사용되지 않는 메서드넹

pci2676 · 2020-06-04T11:20:30Z

src/main/java/com/javabom/lotto/domain/WinningSheet.java

+        if (isThirdAndMatchBonus(findSheet, matchBonus)) {
+            return SECOND;
+        }


Suggested change

if (isThirdAndMatchBonus(findSheet, matchBonus)) {

return SECOND;

}

if (findSheet.isSecond(matchBonus)) {

return SECOND;

}

이건 어때?

pci2676 · 2020-06-04T11:37:58Z

src/main/java/com/javabom/lotto/domain/vo/LottoMoney.java

+    public double calculateRateOfReturn(int profit) {
+        int purchaseAmount = (numberOfAutoTicket + numberOfManualTicket) * LOTTO_PRICE;
+
+        return profit / purchaseAmount;
+    }


int 결과 계산이 double이 되어버렸어

malibinYun · 2020-06-05T13:23:04Z

src/main/java/com/javabom/lotto/domain/LottoBill.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class LottoBill {


난 갠적으로 LottoTickets가 더 이름이 나은거같아보여

malibinYun · 2020-06-05T13:24:41Z

src/main/java/com/javabom/lotto/domain/LottoBill.java

+        this.lottoTickets = lottoTickets;
+    }
+
+    public LottoResult drawAllLotto(LottoWinningTicket lottoWinningTicket) {


draw의 뜻이 끌어당기다, 그리다 이런뜻으로 알고있는데 다른 뜻은 모르겠어서
어떤 일을 하는지 이름만 보고는 잘 모를거같아

…) -> matchAllLotto() 로 이름 변경

refactor(LottoNumberGenerator): generateRandomNumber가 인자로 NumberGenerator 를 받아서 호출하도록 변경 refactor(LottoTicket): ofAuto() 메소드 인자로 NumberGenerator 구현체를 받도록 변경

malibinYun · 2020-06-11T10:54:59Z

src/main/java/com/javabom/lotto/domain/number/NumberGenerator.java

+import java.util.List;
+
+public interface NumberGenerator {
+    List<Integer> generate(int min, int max);


음 나라면

Suggested change

List<Integer> generate(int min, int max);

List<Integer> generateRangeOf(int min, int max);

라고 했을거같아

malibinYun · 2020-06-11T11:04:56Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+    public static LottoTicket ofFixed(List<Integer> numbers) {
+        validateSize(numbers);
+        return new LottoTicket(LottoNumberGenerator.generateFixedNumber(numbers));
+    }
+
+    public static LottoTicket ofAuto(NumberGenerator numberGenerator) {
+        return new LottoTicket(LottoNumberGenerator.generateRandomNumber(LOTTO_NUMBERS_SIZE, numberGenerator));
+    }
+


티켓 자체가 수동or자동 티켓을 발급해주는게 난조금 이상하게 보여
티켓이 티켓을 생성하는 책임을 가지고있다는 느낌이랄까?
흠.. 솔직히 잘 모르겠다 나는 LottoTicket을 만들어주는 애는 다른 클래스에 책임을 넘겼어.

음 이건 정적 펙토리 메서드입니다.
객체의 생성은 해당 객체의 생성자로만 이루어지는데, 생성자의 이름변경을 못하니 클래스메서드로 생성함수의 이름으로 명확하게 표현한걸로 보이네요,
이런 구현은 좋아보입니다.

malibinYun · 2020-06-11T11:08:11Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+        Set<Integer> duplicateNumbers = lottoNumbers.stream()
+                .map(LottoNumber::getNumber)
+                .collect(Collectors.toSet());


결과를 Set으로 안받구 그냥 integer로 받아도 좋을듯!

Suggested change

Set<Integer> duplicateNumbers = lottoNumbers.stream()

.map(LottoNumber::getNumber)

.collect(Collectors.toSet());

int distinctNumberSize = lottoNumbers.stream()

.map(LottoNumber::getNumber)

.collect(Collectors.toSet())

.size()

pci2676 · 2020-06-12T12:23:29Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+
+    public int findMatchCount(LottoTicket winningTicket) {
+        return (int) IntStream.range(0, LOTTO_NUMBERS_SIZE)
+                .filter(index -> winningTicket.findByIndex(index).equals(this.findByIndex(index)))


디미터 법칙을 위반하고 있는것 같아!

pci2676 · 2020-06-12T12:51:17Z

src/main/java/com/javabom/lotto/domain/vo/LottoMoney.java

+public class LottoMoney {
+    private static final int LOTTO_PRICE = 1000;
+    private int numberOfAutoTicket;
+    private int numberOfManualTicket;


흠 Money는 딱 돈에 대한 값만을 들고 있을것 같은 VO의 네이밍인데 산 갯수 두개를 들고 있는게 뭔가 어색해보여 이름을 바꿔줘도 될것같아.
아니면 객체를 나눠서 돈은 돈대로 갯수는 갯수대로 따로 관리해도 좋을것 같아
그리고 VO니까 final을 붙여줘도 좋을것 같아

chldbtjd2272 · 2020-06-12T14:26:10Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+        }
+    }
+
+    public int findMatchCount(LottoTicket winningTicket) {


for문을 index에 의존해서 돌리는 방법이 아니라
애초에 LottNumber와 index를 감싸는 객체를 만들어 클래스를 만들어서 비교하면 더 깔끔하게 로직을 짤 수 있을거같아.

class PickedLottoNumber{ private int order; private LottoNumber lottoNumber; ..... }

chldbtjd2272 · 2020-06-12T19:47:46Z

src/main/java/com/javabom/lotto/LottoApplication.java

    public static void main(String[] args) {
+        int purchaseAmount = InputView.inputPurchaseAmount();
+
+        int numberOfManualTicket = InputView.inputNumberOfManualTicket();


InputView에서 LottoMoney객체를 한번에 받아 가져오는게 좋을듯, 기본 타입을 굳이 다 노출시킬필요도 없고 도메인 객체를 input에서 받아도 상관없어보여

LottoMoney lottoMoney=InputView.inputLottoInfo();

chldbtjd2272 · 2020-06-12T19:48:47Z

src/main/java/com/javabom/lotto/LottoApplication.java

+
+        int bonusNumber = InputView.inputBonusNumber();
+
+        LottoWinningTicket lottoWinningTicket = new LottoWinningTicket(winningNumbers, bonusNumber);


이것도 위와 마찬가지로 View에서 바로 받아와도 문제없어

chldbtjd2272 · 2020-06-12T19:55:51Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+    public static LottoTicket ofFixed(List<Integer> numbers) {
+        validateSize(numbers);
+        return new LottoTicket(LottoNumberGenerator.generateFixedNumber(numbers));
+    }
+
+    public static LottoTicket ofAuto(NumberGenerator numberGenerator) {
+        return new LottoTicket(LottoNumberGenerator.generateRandomNumber(LOTTO_NUMBERS_SIZE, numberGenerator));
+    }
+


음 이건 정적 펙토리 메서드입니다.
객체의 생성은 해당 객체의 생성자로만 이루어지는데, 생성자의 이름변경을 못하니 클래스메서드로 생성함수의 이름으로 명확하게 표현한걸로 보이네요,
이런 구현은 좋아보입니다.

chldbtjd2272 · 2020-06-12T20:09:43Z

src/main/java/com/javabom/lotto/domain/ticket/LottoWinningTicket.java

+import java.util.List;
+
+public class LottoWinningTicket {
+    private final LottoTicket winningTicket;


LottoTicket과 winningTicket은 같은 역할을 하는 객체일까?
LottoTicket의 공통적으로 필요한 부분만 추출해 하나의 클래스로 추출하고,
각각 필요한 기능은 각각의 클래스가 가지는 포함관계로 풀어보는것도 좋을거같아

chldbtjd2272 · 2020-06-12T20:18:03Z

src/main/java/com/javabom/lotto/domain/LottoResult.java

+    }
+
+    private Map<WinningSheet, Integer> makeStatistics(List<WinningSheet> winningSheets) {
+        Map<WinningSheet, Integer> statistics = new EnumMap<>(WinningSheet.class);


collect의 groupingBy를 쓰는게 더 깔끔할거같아

로또(자동) 구현

a06a583

malibinYun reviewed May 26, 2020

View reviewed changes

pci2676 requested changes May 27, 2020

View reviewed changes

Gobukgol added 5 commits June 4, 2020 16:23

로또(2등) 구현

fc52a65

feat(LottoMoney): 수동 개수 필드 추가

8d34b91

refactor(LottoMoneyTest): 테스트 로직 변경

3580cdb

refactor(InputView): 수동 개수 입력받도록 변경, InputView에 LottoMoney 의존성 제거

30702ff

feat(ManualNumbersDto): 수동으로 입력받은 번호를 Integer로 parsing해서 List로 만들어주는 Dto

484b98e

refactor(LottoMachine): 수동 번호와 자동 개수를 받도록 변경

pci2676 approved these changes Jun 4, 2020

View reviewed changes

Gobukgol added 2 commits June 5, 2020 14:01

refactor(): 사용되지 않는 생성자 및 getter 제거

c4e9ca0

refactor(LottoMoney): return 값 계산 double로 하게 변경

c8149df

malibinYun reviewed Jun 5, 2020

View reviewed changes

Gobukgol added 5 commits June 11, 2020 16:04

refactor(LottoBill): LottoBill -> LottoTickets 로 이름 변경, drawAllLotto(…

492d400

…) -> matchAllLotto() 로 이름 변경

refactor(LottoNumberGenerator, LottoTicket): 자료구조 Set 에서 List로 변경

5e3a58f

refactor(LottoTicketTest): ofAuto 테스트 추가

173b3a7

refactor(LottoTicket): 순서까지 맞아야 count 하도록 변경

41093fc

Gobukgol changed the title ~~민형 로또(자동) 구현~~ 민형 로또(수동) 구현 Jun 11, 2020

malibinYun reviewed Jun 11, 2020

View reviewed changes

pci2676 approved these changes Jun 12, 2020

View reviewed changes

chldbtjd2272 reviewed Jun 12, 2020

View reviewed changes


		OutputView.printLottoTickets(lottoTickets);

		List<Integer> winningNumbers = InputView.inputLastWinningNumbers();


		OutputView.printNumberOfTicket(lottoMoney);

		List<LottoTicket> lottoTickets = LottoMachine.purchaseLottoTicket(lottoMoney);

		public static List<LottoNumber> generateLottoNumbers() {
		Collections.shuffle(lottoNumbers);

	List<Integer> generate(int min, int max);
	List<Integer> generateRangeOf(int min, int max);


		int bonusNumber = InputView.inputBonusNumber();

		LottoWinningTicket lottoWinningTicket = new LottoWinningTicket(winningNumbers, bonusNumber);

민형 로또(수동) 구현 #12

Are you sure you want to change the base?

민형 로또(수동) 구현 #12

Uh oh!

Conversation

Gobukgol commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pci2676 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chldbtjd2272 Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Gobukgol commented May 26, 2020 •

edited

Loading

chldbtjd2272 Jun 12, 2020 •

edited

Loading