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단계 - 로또(수동)] 오찌(오지훈) 미션 제출합니다. #438

Merged
merged 20 commits into from
Mar 4, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c2a725a
refactor: 부적절한 메서드 및 변수 명 변경
Ohzzi Feb 28, 2022
18b9248
refactor: 로또 숫자 풀의 자료구조를 List에서 Map으로 변경
Ohzzi Feb 28, 2022
78a8ecf
refactor: LottoFactory에서 LottoNumber의 pool을 사용하도록 수정
Ohzzi Feb 28, 2022
6901251
refactor: validation 로직 위치 변경
Ohzzi Feb 28, 2022
fd1dd79
refactor: 통계 출력 시 for 대신 foreach 사용
Ohzzi Feb 28, 2022
af74366
fix: 수익률 계산 오류 수정
Ohzzi Feb 28, 2022
90fe99c
refactor: WinningStatistics 초기화 시 stream.foreach 대신 일반 for-each 사용하도록 변경
Ohzzi Feb 28, 2022
77537f7
feat: 수동 구매 장 수, 자동 구매 장 수를 담는 로또 주문 추가
Ohzzi Mar 1, 2022
fba05f3
feat: 수동 로또 번호를 담는 수동 주문 구현
Ohzzi Mar 1, 2022
2dd2b25
test: 로또 주문 테스트 추가
Ohzzi Mar 1, 2022
d40e678
fix: 수동 로또 구매 수가 전체 구매 수를 넘었을 때 프로그램이 종료되는 오류 수정
Ohzzi Mar 1, 2022
8ec4b36
refactor: view로 도메인을 바로 넘기지 않고 DTO를 넘기도록 수정
Ohzzi Mar 1, 2022
5b9054a
fix: 수동 구매 장 수가 없을 경우 불필요한 메시지 출력하지 않도록 수정
Ohzzi Mar 1, 2022
19dffa8
refactor: DTO 생성시에 원래 객체를 받아서 생성하도록 수정
Ohzzi Mar 3, 2022
209840f
refactor: WinningStatistics가 생성시에 수익률까지 모두 계산하도록 수정
Ohzzi Mar 3, 2022
9ad9ef7
refactor: 테스트에서 에러 메시지 상수를 가져와서 사용하지 않고 문자열 직접 테스트
Ohzzi Mar 3, 2022
44f0a5e
refactor: 이중 리스트를 DTO의 리스트로 변경
Ohzzi Mar 3, 2022
3feafd9
refactor: WinningStatistics 생성자를 정적 팩터리 메서드로 변경
Ohzzi Mar 3, 2022
9475824
refactor: getter에서 unmodifiable 자료구조를 반환하도록 수정
Ohzzi Mar 3, 2022
dd3506a
refactor: 클래스 명과 필드 명 적절하게 수정
Ohzzi Mar 4, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 87 additions & 24 deletions src/main/java/controller/LottoController.java
Original file line number Diff line number Diff line change
@@ -1,64 +1,127 @@
package controller;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import model.Lotto;
import model.LottoFactory;
import model.LottoOrder;
import model.LottoPurchasingMoney;
import model.LottoRank;
import model.WinningLotto;
import model.WinningStatistics;
import model.dto.LottoDto;
import model.dto.WinningStatisticsDto;
import view.InputView;
import view.OutputView;

public class LottoController {
public void run() {
Copy link
Author

Choose a reason for hiding this comment

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

코드를 작성하고 보니 controller가 너무 난잡한게 아닌가 하는 생각이 들어요!
service 레이어를 분리해볼까 라고 생각은 했지만 아직 controller와 service의 분리 기준도 잘 모르겠고요!
생각해보면 controller에서 여러 메서드를 public으로 열어 놓고 main에서 여러 메서드를 호출하면서 진행해야 하는 게 아닐까 하는 생각도 하고 있습니다. 닉은 어떻게 생각하시나요??

Copy link

Choose a reason for hiding this comment

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

음 저는 main은 클린하게 유지되어야 한다고 생각해요. (애플리케이션을 실행시키는 역할만)
말씀하신대로 controller에서 여러 메서드를 public으로 열어 놓고 main에서 여러 메서드를 호출하면서 진행하게 되면 오히려 Model과 View를 통해 애플리케이션의 흐름을 제어하는 Controller의 책임이 main으로 분산된다고 생각하거든요.

난잡하다고 느끼신 부분은 많은 private 메서드 때문일지도 모르겠네요. 그럴때마다 객체가 너무 많은 일을 하고 있지는 않은지 고민해보시면 좋아요.
아래에 따로 코멘트 남길게요 :)

LottoPurchasingMoney inputLottoPurchasingMoney = inputMoney();
LottoPurchasingMoney lottoPurchasingMoney = inputPurchasingMoney();

List<Lotto> lotteries = purchaseLottoTickets(inputLottoPurchasingMoney);
OutputView.printPurchasedTickets(lotteries);
LottoOrder lottoOrder = inputLottoOrder(lottoPurchasingMoney);

WinningLotto winningLotto = inputWinningNumbers();
List<Lotto> lotteries = purchaseLotteries(lottoOrder);
printPurchasedLotteries(lottoOrder, lotteries);

WinningLotto winningLotto = inputWinningLotto();

WinningStatistics winningStatistics = calculateStatistics(lotteries, winningLotto);

OutputView.printStatistics(winningStatistics, inputLottoPurchasingMoney);
printWinningStatistics(winningStatistics, lottoPurchasingMoney);
}

private WinningStatistics calculateStatistics(List<Lotto> lotteries, WinningLotto winningLotto) {
WinningStatistics winningStatistics = new WinningStatistics();

lotteries.forEach(lotto -> {
LottoRank lottoRank = LottoRank.getRank(
winningLotto.countMatching(lotto),
winningLotto.containBonusBall(lotto)
);
winningStatistics.put(lottoRank);
});

return winningStatistics;
private void printPurchasedLotteries(LottoOrder lottoOrder, List<Lotto> lotteries) {
List<LottoDto> lotteriesDto = lotteries.stream()
.map(lotto -> new LottoDto(lotto.getLottoNumbers()))
.collect(Collectors.toUnmodifiableList());
OutputView.printPurchasedLotteries(lottoOrder, lotteriesDto);
}

private LottoPurchasingMoney inputMoney() {
private LottoPurchasingMoney inputPurchasingMoney() {
try {
return LottoPurchasingMoney.valueOf(InputView.inputMoney());
} catch (IOException | IllegalArgumentException e) {
OutputView.printErrorMessage(e.getMessage());
return inputMoney();
return inputPurchasingMoney();
}
}

private LottoOrder inputLottoOrder(LottoPurchasingMoney lottoPurchasingMoney) {
try {
int manualLottoCount = InputView.inputManualLottoCount();
return new LottoOrder(lottoPurchasingMoney, manualLottoCount);
} catch (IOException | IllegalArgumentException e) {
OutputView.printErrorMessage(e.getMessage());
return inputLottoOrder(lottoPurchasingMoney);
}
}

private List<Lotto> purchaseLotteries(LottoOrder lottoOrder) {
return Stream.of(purchaseManualLotteries(lottoOrder), purchaseAutoLotteries(lottoOrder))
.flatMap(Collection::stream)
.collect(Collectors.toUnmodifiableList());
}

private List<List<Integer>> inputManualLottoNumbers(LottoOrder lottoOrder) {
try {
return InputView.inputManualLottoNumbers(lottoOrder.getManualLottoCount());
} catch (IOException e) {
OutputView.printErrorMessage(e.getMessage());
return inputManualLottoNumbers(lottoOrder);
}
}

private List<Lotto> purchaseManualLotteries(LottoOrder lottoOrder) {
LottoFactory lottoFactory = LottoFactory.getInstance();
Copy link
Author

Choose a reason for hiding this comment

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

현재는 LottoFactory를 싱글톤으로 만들어서 유지하고 있는데, 사실상 유틸 클래스와 다름이 없다는 생각도 듭니다!
싱글톤 vs static 유틸 클래스 어느쪽으로 만드는 것이 더 좋은 선택일까요?

Copy link

Choose a reason for hiding this comment

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

좋은 질문이네요.
자바에서 기본적으로 static은 안티 패턴입니다.
객체지향 패러다임의 장점을 사용할 수 없거든요.
게다가 LottoFactory는 도메인 모델이지 유틸 클래스가 아닙니다ㅎㅎ

그래서 설계하실 때는 최대한 정적 클래스를 지양하시고, 도메인 로직과 관련이 없지만 가공 등의 로직이 필요한 경우만 유틸 (정적) 클래스로 만드시는 게 좋습니다. (ex. String <-> LocalDate 변환을 돕는 LocalDateUtils)

try {
List<List<Integer>> manualLottoNumbers = inputManualLottoNumbers(lottoOrder);
return manualLottoNumbers.stream()
.map(lottoFactory::generateManual)
.collect(Collectors.toUnmodifiableList());
} catch (IllegalArgumentException e) {
OutputView.printErrorMessage(e.getMessage());
return purchaseManualLotteries(lottoOrder);
}
}

private List<Lotto> purchaseAutoLotteries(LottoOrder lottoOrder) {
LottoFactory lottoFactory = LottoFactory.getInstance();
List<Lotto> autoLotteries = new ArrayList<>();
for (int i = 0; i < lottoOrder.getAutoLottoCount(); i++) {
autoLotteries.add(lottoFactory.generateAuto());
}
return autoLotteries;
}

private WinningLotto inputWinningNumbers() {
private WinningLotto inputWinningLotto() {
try {
return WinningLotto.of(InputView.inputWinningNumbers(), InputView.inputBonusBall());
} catch (IOException | IllegalArgumentException e) {
OutputView.printErrorMessage(e.getMessage());
return inputWinningNumbers();
return inputWinningLotto();
}
}

private List<Lotto> purchaseLottoTickets(LottoPurchasingMoney inputLottoPurchasingMoney) {
LottoFactory ticketFactory = LottoFactory.getInstance();
return ticketFactory.generateLotteries(inputLottoPurchasingMoney);
private WinningStatistics calculateStatistics(List<Lotto> lotteries, WinningLotto winningLotto) {
WinningStatistics winningStatistics = new WinningStatistics();

lotteries.forEach(lotto -> {
LottoRank lottoRank = LottoRank.getRank(
winningLotto.countMatching(lotto),
winningLotto.containBonusBall(lotto)
);
winningStatistics.put(lottoRank);
});
Copy link
Author

Choose a reason for hiding this comment

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

당첨 통계를 map에 저장하는 로직이 controller에 있어서 부자연스러울까요...?

Copy link

Choose a reason for hiding this comment

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

네네 안그래도 이 부분에서 위에 남긴 코멘트 때문에 이야기하려고 했는데 ㅎㅎ
통계 객체에서 충분히 해줄 수 있는 일이 아닐까요? :)


return winningStatistics;
}

private void printWinningStatistics(WinningStatistics winningStatistics, LottoPurchasingMoney lottoPurchasingMoney) {
OutputView.printStatistics(new WinningStatisticsDto(winningStatistics.getWinningCounts(),
winningStatistics.getEarningsRate(lottoPurchasingMoney)));
}
}
10 changes: 5 additions & 5 deletions src/main/java/model/Lotto.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ public class Lotto {
private final List<LottoNumber> lottoNumbers;

private Lotto(List<LottoNumber> lottoNumbers) {
lottoNumbers = List.copyOf(lottoNumbers);
validate(lottoNumbers);
this.lottoNumbers = lottoNumbers;
}

public static Lotto from(List<LottoNumber> lottoNumbers) {
lottoNumbers = List.copyOf(lottoNumbers);
validate(lottoNumbers);
return new Lotto(lottoNumbers);
}

Expand All @@ -34,18 +34,18 @@ public boolean contains(LottoNumber lottoNumber) {
return lottoNumbers.contains(lottoNumber);
}

private static void validate(List<LottoNumber> lottoNumbers) {
private void validate(List<LottoNumber> lottoNumbers) {
validateSize(lottoNumbers);
validateIsNotDuplicated(lottoNumbers);
}

private static void validateSize(List<LottoNumber> lottoNumbers) {
private void validateSize(List<LottoNumber> lottoNumbers) {
if (lottoNumbers.size() != LOTTO_NUMBER_COUNT) {
throw new IllegalArgumentException(INVALID_LOTTO_NUMBER_COUNT);
}
}

private static void validateIsNotDuplicated(List<LottoNumber> lottoNumbers) {
private void validateIsNotDuplicated(List<LottoNumber> lottoNumbers) {
long distinctSize = lottoNumbers.stream()
.distinct().count();
if (distinctSize != lottoNumbers.size()) {
Expand Down
28 changes: 7 additions & 21 deletions src/main/java/model/LottoFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,40 +4,26 @@
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class LottoFactory {
private static final LottoFactory LOTTO_FACTORY = new LottoFactory();

private static final int LOTTO_PRICE = 1000;

private final List<LottoNumber> lottoNumberPool;

private LottoFactory() {
lottoNumberPool = createLottoNumbers();

}

public static LottoFactory getInstance() {
return LOTTO_FACTORY;
}

private List<LottoNumber> createLottoNumbers() {
return IntStream.rangeClosed(1, 45)
.mapToObj(LottoNumber::valueOf)
.collect(Collectors.toList());
}

public List<Lotto> generateLotteries(LottoPurchasingMoney money) {
return IntStream.range(0, getAvailableLottoCount(money))
.mapToObj(i -> generateAuto())
.collect(Collectors.toUnmodifiableList());
}

private int getAvailableLottoCount(LottoPurchasingMoney money) {
return money.getAmount() / LOTTO_PRICE;
public Lotto generateManual(List<Integer> manualNumbers) {
return Lotto.from(manualNumbers.stream()
.map(LottoNumber::valueOf)
.collect(Collectors.toUnmodifiableList()));
}

private Lotto generateAuto() {
public Lotto generateAuto() {
List<LottoNumber> lottoNumberPool = new ArrayList<>(LottoNumber.LOTTO_NUMBER_POOL.values());
Collections.shuffle(lottoNumberPool);
return Lotto.from(new ArrayList<>(lottoNumberPool.subList(0, 6)));
}
Expand Down
11 changes: 6 additions & 5 deletions src/main/java/model/LottoNumber.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package model;

import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

Expand All @@ -10,12 +11,12 @@ public class LottoNumber {
private static final int MAX_LOTTO_NUMBER = 45;
static final String INVALID_LOTTO_NUMBER_RANGE = "[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.";

private static final List<LottoNumber> LOTTO_NUMBER_POOL;
static final Map<Integer, LottoNumber> LOTTO_NUMBER_POOL;

static {
LOTTO_NUMBER_POOL = IntStream.rangeClosed(MIN_LOTTO_NUMBER, MAX_LOTTO_NUMBER)
.mapToObj(LottoNumber::new)
.collect(Collectors.toUnmodifiableList());
.boxed()
.collect(Collectors.toUnmodifiableMap(Function.identity(), LottoNumber::new));
}

private final int number;
Expand All @@ -27,7 +28,7 @@ private LottoNumber(int number) {

public static LottoNumber valueOf(int number) {
validateRange(number);
Copy link

Choose a reason for hiding this comment

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

안그래도 이 부분은 생성자에서 validation을 해야하나 정적 메서드에서 해야 하나 고민했던 부분이었는데 해결이 됐네요!! :)
그런데 질문이 있습니다! LottoNumber 같은 경우에는 생성자를 처음에 pool을 만들때만 쓰고 있어서 여기에 validation이 들어가면 이후 프로덕션에서 사용할 수가 없는데, 이런 경우에는 정적 팩토리 메서드에 들어가도 될까요?

아 네네 그런 경우는 어쩔 수 없겠네요 :)

return LOTTO_NUMBER_POOL.get(number - 1);
return LOTTO_NUMBER_POOL.get(number);
}

private static void validateRange(int number) {
Expand Down
28 changes: 28 additions & 0 deletions src/main/java/model/LottoOrder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package model;

public class LottoOrder {
static final String TOO_MANY_MANUAL = "[ERROR] 수동 로또 구매 수는 전체 구매 수를 넘을 수 없습니다.";
Copy link
Author

Choose a reason for hiding this comment

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

수동 구매 개수와 자동 구매 개수를 저장할 방법을 골똘히 고민하다가, 우아한객체지향 세미나에서 주문, 배달과 같은 모든 상태를 객체로 만들어서 관리했던 것이 생각나서 저도 구매 정보를 LottoOrder로 만들어서 저장했습니다!

  1. 이렇게 구매 정보를 클래스로 분리하는 방법이 좋을까요?
  2. 이 클래스는 따로 비즈니스 로직이 없는게 아닌가 해서 DTO로 만들지 않았는데, 그래도 view로 넘길 때 DTO로 만들어 주는게 좋을까요?

Copy link

Choose a reason for hiding this comment

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

  1. 네네 잘하셨습니다 👍
  2. 음 저는 이 객체도 DTO라고 생각해요. 해당 객체를 가지고 Controller와 View 레이어에서 정보를 주고받으니까요 :)


private final int manualLottoCount;
private final int autoLottoCount;

public LottoOrder(LottoPurchasingMoney lottoPurchasingMoney, int manualLottoCount) {
validateManualCount(lottoPurchasingMoney, manualLottoCount);
this.manualLottoCount = manualLottoCount;
this.autoLottoCount = lottoPurchasingMoney.getPurchasableCount() - manualLottoCount;
}

public int getManualLottoCount() {
return manualLottoCount;
}

public int getAutoLottoCount() {
return autoLottoCount;
}

private void validateManualCount(LottoPurchasingMoney lottoPurchasingMoney, int manualLottoCount) {
if (lottoPurchasingMoney.getPurchasableCount() < manualLottoCount) {
throw new IllegalArgumentException(TOO_MANY_MANUAL);
}
}
}
10 changes: 7 additions & 3 deletions src/main/java/model/LottoPurchasingMoney.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,23 @@ public class LottoPurchasingMoney {
private final int amount;

private LottoPurchasingMoney(int amount) {
this.amount = amount;
validateEnough(amount);
this.amount = amount / LOTTO_PRICE * LOTTO_PRICE;
}

public static LottoPurchasingMoney valueOf(int amount) {
validateEnough(amount);
return new LottoPurchasingMoney(amount);
}

public int getAmount() {
return amount;
}

private static void validateEnough(int amount) {
public int getPurchasableCount() {
return amount / LOTTO_PRICE;
}

private void validateEnough(int amount) {
if (amount < LOTTO_PRICE) {
throw new IllegalArgumentException(NOT_ENOUGH_MONEY);
}
Expand Down
27 changes: 16 additions & 11 deletions src/main/java/model/WinningStatistics.java
Original file line number Diff line number Diff line change
@@ -1,33 +1,38 @@
package model;

import java.util.Arrays;
import java.util.EnumMap;
import java.util.Map;

public class WinningStatistics {
private final EnumMap<LottoRank, Integer> winningStatistics;
private final EnumMap<LottoRank, Integer> winningCounts;

public WinningStatistics() {
winningStatistics = new EnumMap<>(LottoRank.class);
Arrays.stream(LottoRank.values())
.forEach(lottoRank -> winningStatistics.put(lottoRank, 0));
winningCounts = new EnumMap<>(LottoRank.class);
Copy link

Choose a reason for hiding this comment

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

요런건 필드에서 바로 초기화하는게 더 깔끔할 것 같아요 :)
(생성자가 여러 개로 늘어날 경우를 대비해서)

for (LottoRank each : LottoRank.values()) {
winningCounts.put(each, 0);
}
}

public void put(LottoRank lottoRank) {
winningStatistics.put(lottoRank, winningStatistics.get(lottoRank) + 1);
winningCounts.put(lottoRank, winningCounts.get(lottoRank) + 1);
}

public int get(LottoRank lottoRank) {
return winningStatistics.get(lottoRank);
return winningCounts.get(lottoRank);
}

public double getEarningsRate(int money) {
public Map<LottoRank, Integer> getWinningCounts() {
return winningCounts;
}

public double getEarningsRate(LottoPurchasingMoney lottoPurchasingMoney) {
long totalPrize = getTotalPrize();
return totalPrize / (double)money;
return totalPrize / (double)lottoPurchasingMoney.getAmount();
}

long getTotalPrize() {
return winningStatistics.keySet().stream()
.mapToLong(lottoRank -> lottoRank.getPrizeMoney() * winningStatistics.get(lottoRank))
return winningCounts.keySet().stream()
.mapToLong(lottoRank -> lottoRank.getPrizeMoney() * winningCounts.get(lottoRank))
.sum();
}
}
16 changes: 16 additions & 0 deletions src/main/java/model/dto/LottoDto.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package model.dto;

import java.util.List;
import model.LottoNumber;

public class LottoDto {
private final List<LottoNumber> lottoNumbers;

public LottoDto(List<LottoNumber> lottoNumbers) {
Copy link
Author

Choose a reason for hiding this comment

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

이 부분에 DTO 관련 질문이 하나 더 있습니다! 제가 코드를 작성하고 보니까 controller에서 DTO에 필요한 값을 다 꺼내다가 생성자로 넣어주었는데요, 그냥 DTO 생성자에 원본 객체를 넣어주고 생성자 안에서 getter로 꺼내 쓰는 쪽이 조금 더 좋은 코드라고 보시나요?

Copy link

Choose a reason for hiding this comment

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

네네 (파라미터가 많은 경우) 코드도 깔끔해지고 어떤 객체의 데이터를 꺼내서 만든 DTO야, 라는 것이 생성자에서 드러나면 해당 DTO를 사용하는 클라이언트 입장에서도 이해하기 쉬울 것 같아요 :)

this.lottoNumbers = List.copyOf(lottoNumbers);
}

public List<LottoNumber> getLottoNumbers() {
return lottoNumbers;
}
}
Loading