Skip to content

[LBP] 윤준석 로또 미션 2-5단계 제출합니다. #89

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

Open
wants to merge 11 commits into
base: junseok0304
Choose a base branch
from
18 changes: 18 additions & 0 deletions src/main/java/config/LottoConstants.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package config;

public enum LottoConstants {
Copy link

Choose a reason for hiding this comment

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

Enum 활용 👍🏻

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다!

MIN_NUMBER(1),
MAX_NUMBER(45),
LOTTO_SIZE(6),
LOTTO_PRICE(1000);

private final int value;

LottoConstants(int value) {
this.value = value;
}

public int getValue() {
return value;
}
}
48 changes: 48 additions & 0 deletions src/main/java/config/LottoValidator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package config;

import java.util.*;

public class LottoValidator {
Copy link

Choose a reason for hiding this comment

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

LottoConstants를 정의했지만 Validator에서 사용하지 않은 이유가 있을까요?

Copy link

Choose a reason for hiding this comment

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

현재 Validator 클래스가 유틸리티 성격을 띠고 있지만, Lotto와 LottoMachine에서도 자체적으로 검증을 하고 있어서 중복되는 코드가 많아 보여요 🤔
도메인 계층에서 검증을 수행하고 있는데, Controller에서 한 번 더 검증하는 이유가 있을까요?

도메인에서 검증하는 방식과 Validator 클래스를 사용하는 방식 각각의 장단점을 비교해 보면 좋을 것 같아요. 그 후에 리팩토링 방향을 고민해보시면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

예외메시지 출력을 위해서였지만
중복되는 코드가 생기는 이슈가 있었네요,
validator에서 사전 입력 검증을 하도록 하고
도메인 내부에서 검증 로직을 메서드로 정리해서 재사용하는 방식도 괜찮을 것 같습니다!

public static void validatePurchaseAmount(int amount) {
if (amount <= 0) {
throw new IllegalArgumentException("구매 금액은 0보다 커야 합니다.");
}
if (amount % 1000 != 0) {
throw new IllegalArgumentException("구매 금액은 1000원 단위여야 합니다.");
}
}

public static void validateManualTicketCount(int count) {
if (count < 0) {
throw new IllegalArgumentException("수동 구매 개수는 0 이상이어야 합니다.");
}
}

public static List<Integer> validateAndParseLottoNumbers(String input) {
List<Integer> numbers = Arrays.stream(input.split(","))
.map(String::trim)
.map(Integer::parseInt)
.toList();
Comment on lines +22 to +25
Copy link

Choose a reason for hiding this comment

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

현재 메서드의 길이가 요구사항(10라인 제한)을 초과한 것 같아요. 검증 로직이 추가되면 더 길어질 가능성도 있어 보이네요 🤔

이 메서드에서는 입력을 나누는 역할과 검증하는 역할을 함께 하고 있는데, 혹시 입력을 나누는 로직이 다른 곳에서도 필요할 가능성이 있을까요? 만약 그렇다면 별도 메서드로 분리하면 어떤 장점이 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

입력값을 나누는 로직과 검증하는 로직을 분리하면 재사용성을 재고할 수 있고
테스트 코드 작성시에도 이점이 있을 것 같습니다!


if (numbers.size() != 6) {
throw new IllegalArgumentException("로또 번호는 6개여야 합니다.");
}

Set<Integer> uniqueNumbers = new HashSet<>(numbers);
if (uniqueNumbers.size() != 6) {
throw new IllegalArgumentException("로또 번호는 중복될 수 없습니다.");
}

if (numbers.stream().anyMatch(n -> n < 1 || n > 45)) {
throw new IllegalArgumentException("로또 번호는 1~45 사이여야 합니다.");
}

return new ArrayList<>(uniqueNumbers);
}

public static void validateBonusNumber(int bonus) {
if (bonus < 1 || bonus > 45) {
throw new IllegalArgumentException("보너스 번호는 1~45 사이여야 합니다.");
}
}
}
39 changes: 39 additions & 0 deletions src/main/java/config/WinningRank.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package config;

import java.util.Arrays;
import java.util.Optional;

public enum WinningRank {
MATCH_3(3, 5000, "3개 일치 (5000원)"),
MATCH_4(4, 50000, "4개 일치 (50000원)"),
MATCH_5(5, 1500000, "5개 일치 (1500000원)"),
MATCH_5_BONUS(5, 30000000, "5개 일치, 보너스 볼 일치(30000000원)"),
MATCH_6(6, 2000000000, "6개 일치 (2000000000원)");
Comment on lines +7 to +11
Copy link

Choose a reason for hiding this comment

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

Java에서는 큰 숫자를 다룰 때 언더바를 활용할 수 있어요. 언더바를 사용하면 가독성이 좋아질 것 같은데, 어떻게 생각하시나요?

    MATCH_3(3, 5_000, "3개 일치 (5,000원)"),
    MATCH_4(4, 50_000, "4개 일치 (50,000원)"),
    ...

Copy link
Author

Choose a reason for hiding this comment

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

오! 훨씬 나은 것 같습니다. 찾아보니 자바에서는 , 콤마가 예약어이므로
_를 사용해서 숫자에서 코드에 영향을 주지 않으면서
가독성을 높일 수 있는 방법이라고 하는군요


private final int matchCount;
private final int prize;
private final String description;

WinningRank(int matchCount, int prize, String description) {
this.matchCount = matchCount;
this.prize = prize;
this.description = description;
}

public int getPrize() {
return prize;
}

public String getDescription() {
return description;
}

public static Optional<WinningRank> findByMatchCount(int matchCount, boolean hasBonus) {
if (matchCount == 5 && hasBonus) {
Copy link

Choose a reason for hiding this comment

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

이와 같이 하드 코딩된 조건문을 사용하는 것보다, Enum이 스스로 결정하고 검증할 수 있도록 하는 건 어떨까요?
예를 들면, isMatching과 같은 메서드를 만들어서 자신과 주어진 값을 일치하는지 확인하는 방법으로요!

이 방법에 대한 준석님의 의견이 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

Enum에서 스스로 상태를 결정해준다는 것에 대해서 생각해보지 못했는데,
그런 방법도 있군요!
규칙을 한 곳에서 관리해주기에도 유리할 것 같습니다 감사합니다.

return Optional.of(MATCH_5_BONUS);
}
return Arrays.stream(values())
.filter(rank -> rank.matchCount == matchCount)
.findFirst();
}
}
94 changes: 94 additions & 0 deletions src/main/java/controller/LottoMachineController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package controller;

import model.Lotto;
import model.LottoMachine;
import model.LottoResultChecker;
import config.LottoValidator;
import view.InputView;
import view.ResultView;

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

public class LottoMachineController {
public void run() {
Copy link

Choose a reason for hiding this comment

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

run() 메서드가 다양한 책임을 가지고 있어요.
흐름을 표현한 것은 좋지만, 메서드 분리가 되어있지 않으니 가독성이 떨어지는 것 같아요. 각 역할에 맞게 메서드를 분리하면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

말씀해주신대로, 런에서 모두 처리하는 것 보다 메서드를 분리해서 호출해서 사용하는편이 더 나은 것 같습니다!

int purchaseAmount;

while (true) {
try {
purchaseAmount = InputView.getPurchaseAmount();
LottoValidator.validatePurchaseAmount(purchaseAmount);
break;
} catch (IllegalArgumentException e) {
ResultView.printInvalidAmountMessage();
}
}
Comment on lines +18 to +26
Copy link

Choose a reason for hiding this comment

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

재입력을 받는 접근은 좋아요 👍🏻
하지만 요구사항에 indent depth1로 제한하고 있기 때문에, 재입력보다는 예외 처리를 하면 더 간결해질 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

do while문 사용에 익숙치 않아서 위와 같은 방법으로 작성한 것 같습니다!
지윤님 말씀대로, 예외 발생시 반복입력을 요청하도록 하는 방법도 괜찮은 것 같습니다.


int totalTicketCount = LottoMachine.getTicketCount(purchaseAmount);

int manualTicketCount;
while (true) {
try {
manualTicketCount = InputView.getManualTicketCount();
LottoValidator.validateManualTicketCount(manualTicketCount);
if (manualTicketCount > totalTicketCount) {
ResultView.printInvalidManualTicketMessage();
continue;
}
break;
} catch (IllegalArgumentException e) {
ResultView.printInvalidManualCountMessage();
}
}

int autoTicketCount = totalTicketCount - manualTicketCount;

List<List<Integer>> manualNumbers;
while (true) {
try {
List<String> inputNumbers = InputView.getManualLottoNumbers(manualTicketCount);
manualNumbers = inputNumbers.stream()
.map(LottoValidator::validateAndParseLottoNumbers)
.collect(Collectors.toList());
break;
} catch (IllegalArgumentException e) {
ResultView.printInvalidLottoNumbersMessage();
}
}

List<Lotto> tickets = LottoMachine.generateLottos(manualNumbers, purchaseAmount);
List<String> formattedTickets = tickets.stream().map(Lotto::toString).collect(Collectors.toList());

ResultView.printOrderTickets(manualTicketCount, autoTicketCount);
ResultView.printTickets(formattedTickets);

List<Integer> winningNumbers;
while (true) {
try {
String input = InputView.getWinningNumbers();
winningNumbers = LottoValidator.validateAndParseLottoNumbers(input);
break;
} catch (IllegalArgumentException e) {
ResultView.printInvalidWinningNumbersMessage();
}
}

int bonusNumber;
while (true) {
try {
bonusNumber = InputView.getBonusNumber();
LottoValidator.validateBonusNumber(bonusNumber);
break;
} catch (IllegalArgumentException e) {
ResultView.printInvalidBonusNumberMessage();
}
}

Map<String, Integer> results = LottoResultChecker.checkWinningResults(tickets, winningNumbers, bonusNumber);
int totalPrize = results.get("totalPrize");
double profitRate = LottoResultChecker.calculateProfitRate(totalPrize, purchaseAmount);

ResultView.printWinningStatistics(results, profitRate);
}
}
46 changes: 46 additions & 0 deletions src/main/java/model/LottoMachine.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package model;

import config.LottoConstants;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.stream.IntStream;

public class LottoMachine {
private static final int LOTTO_PRICE = LottoConstants.LOTTO_PRICE.getValue();
Copy link

Choose a reason for hiding this comment

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

이미 LottoConstants에서 값을 불러올 수 있는데, 상수를 한 번 더 감싼 이유가 있을까요?

이렇게 하면 상수의 계층이 하나 더 늘어나서 오히려 가독성이 떨어질 수도 있을 것 같아요. 준석님은 어떻게 생각하세요?

Copy link
Author

Choose a reason for hiding this comment

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

그렇네요ㅜㅜ LottoConstants에서 값을 불러와서 사용하는 방식이 나을 것 같습니다!
가독성 측면에서도 이미 정의된 상수를 사용하는 편이 나을 것 같습니다,

private static final List<Integer> LOTTO_NUMBER_POOL =
IntStream.rangeClosed(LottoConstants.MIN_NUMBER.getValue(), LottoConstants.MAX_NUMBER.getValue())
.boxed()
.toList();

public static int getTicketCount(int purchaseAmount) {
Copy link

Choose a reason for hiding this comment

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

getTicketCount 메서드가 단순히 값을 가져오는 게 아니라 로또 개수를 계산하는 역할을 하네요. 더 명확한 네이밍을 사용하면 좋을 거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

calculateTicketCount 라는 이름이 괜찮을 것 같습니다!

if (purchaseAmount % LOTTO_PRICE != 0) {
throw new IllegalArgumentException("구매 금액은 1000원 단위여야 합니다.");
}
return purchaseAmount / LOTTO_PRICE;
}

private static Lotto generateAutoLotto() {
List<Integer> shuffledNumbers = new ArrayList<>(LOTTO_NUMBER_POOL);
Collections.shuffle(shuffledNumbers);
return new Lotto(shuffledNumbers.subList(0, LottoConstants.LOTTO_SIZE.getValue()));
}

public static List<Lotto> generateLottos(List<List<Integer>> manualNumbers, int purchaseAmount) {
Copy link

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.

말씀해주신대로, 수동 로또 생성과 자동 로또 생성을 분리해서 SRP를 준수하고
generateLotto에서는 수동로또생성 메서드와 자동로또생성 메서드를
호출하는 방식으로 바꾸면 좋을 것 같습니다! 감사합니다.

int totalTicketCount = getTicketCount(purchaseAmount);
int manualCount = manualNumbers.size();
int autoCount = totalTicketCount - manualCount;

if (manualCount > totalTicketCount) {
throw new IllegalArgumentException("구매 가능한 로또 개수를 초과했습니다.");
}

List<Lotto> lottos = new ArrayList<>();
manualNumbers.forEach(numbers -> lottos.add(new Lotto(numbers)));
for (int i = 0; i < autoCount; i++) {
lottos.add(generateAutoLotto());
}

return lottos;
}
}
51 changes: 51 additions & 0 deletions src/main/java/model/LottoResultChecker.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package model;

import config.WinningRank;
import java.util.*;

public class LottoResultChecker {

public static Map<String, Integer> checkWinningResults(List<Lotto> purchasedLottos, List<Integer> winningNumbers, int bonusNumber) {
Map<String, Integer> result = initializeResultMap();
int totalPrize = 0;
Set<Integer> winningSet = new HashSet<>(winningNumbers);

for (Lotto lotto : purchasedLottos) {
totalPrize += updateResults(result, lotto, winningSet, bonusNumber);
}

result.put("totalPrize", totalPrize);
return result;
}

private static Map<String, Integer> initializeResultMap() {
Map<String, Integer> result = new LinkedHashMap<>();
for (WinningRank rank : WinningRank.values()) {
result.put(rank.getDescription(), 0);
}
return result;
}

private static int updateResults(Map<String, Integer> result, Lotto lotto, Set<Integer> winningSet, int bonusNumber) {
Copy link

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.

메서드가 여러개의 역할을 하는 부분에 대해서 조금 더 주의깊게 생각해봐야할 것 같습니다, 감사합니다.

int matchCount = (int) lotto.getNumbers().stream().filter(winningSet::contains).count();
Copy link

Choose a reason for hiding this comment

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

stream 사용 👍🏻 체이닝 방식을 쓰면 더 가독성 높아질 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

스트림 체이닝에 대해서 공부해보겠습니다!
코드길이 단축과 중간변수를 선언하지 않아도 되어서 가독성과 유지보수에 이점이 있다고 하는군요,

boolean hasBonus = lotto.getNumbers().contains(bonusNumber);

Optional<WinningRank> rank = WinningRank.findByMatchCount(matchCount, hasBonus);
if (rank.isEmpty()) {
return 0;
}

WinningRank winningRank = rank.get();
String key = winningRank.getDescription();
result.put(key, result.getOrDefault(key, 0) + 1);

return winningRank.getPrize();
}

public static double calculateProfitRate(int totalPrize, int purchaseAmount) {
if (purchaseAmount == 0) {
return 0.0;
}
return (double) totalPrize / purchaseAmount;
}
}
1 change: 1 addition & 0 deletions src/main/java/view/ResultView.java
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package view;

import java.util.List;

public class ResultView {
Expand Down