-
Notifications
You must be signed in to change notification settings - Fork 66
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
[로또 미션] 김의천 미션 제출합니다. #71
base: wzrabbit
Are you sure you want to change the base?
Changes from all commits
0ba5162
68f9cf5
ee14062
09a57c1
0e87fe1
62dbaf4
6ba05cd
05825ed
2d57a43
2807189
04b4d8b
186610c
bd11784
4b84f5e
7e8db89
49db09c
de2e808
f763a05
5f16b83
8885997
88007cd
e7b1582
f214b47
f7fc7a7
1d9ec97
c4bea12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import controller.LottoGameController; | ||
|
||
public class LottoApplication { | ||
public static void main(String[] args) { | ||
LottoGameController lottoGameController = new LottoGameController(); | ||
lottoGameController.run(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
package controller; | ||
|
||
import model.*; | ||
import view.InputView; | ||
import view.OutputView; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.stream.IntStream; | ||
import java.util.stream.Collectors; | ||
|
||
public class LottoGameController { | ||
private final LottoGenerator lottoGenerator = new LottoGenerator(); | ||
private LottoPurchasePrice lottoPurchasePrice; | ||
private int lottoCount; | ||
private int manualLottoCount; | ||
private LottoList manualLottoList; | ||
private LottoList lottoList; | ||
private WinningLottoInfo winningLottoInfo; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 컨트롤러에 DI 와 관련된 부분이 없으니 뭔가 어색해보이네요ㅋㅋ 따로 주입하지 않은 이유가 있나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저 이 코멘트는 이해가 잘 안 되었는데, 혹시 가능하시다면 조금 더 자세히 설명해 주실 수 있나요? DI(Dependency Injection)이랑 컨트롤러의 연관성을 잘 떠올리지 못했던 것 같아요 😢 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아..! 별건아니고 생성자 넣어보자는 거였어요 제가 스프링 공부하면서는 컨트롤러에 약간 관성적으로 생성자넣어서 객체 주입했거든요ㅋㅋ |
||
public void run() { | ||
inputLottoPurchasePrice(); | ||
inputManualLottoCount(); | ||
inputManualLottoList(); | ||
makeLottoList(lottoCount); | ||
inputWinningLottoInfo(); | ||
printLottoPrizeResult(); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 검증에 대해 궁금한 것이 있어요.
에 대해 궁금해요! 컬렉션을 사용하고 있으니 이와 관련된 곳에서 유효한 값인지 확인해도 될 것 같은데 요토의 생각은 어떨까요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 우선 정말 좋은 코멘트 감사합니다. 코멘트를 계기로 고민해 볼 기회가 생긴 것 같습니다. 한 줄로 요약하면, 여기에서는 컬렉션을 만들어 쓰기에는 다소 과했다는 생각이 들었었기 때문입니다.
우선 이 입력값 검증의 경우 둘 이상의 요소가 연관되어 있는 상태입니다. 1) 수동 로또의 개수(입력받는 값), 2) 전체 로또의 개수(검증 시 사용하게 되는 값) 입니다. 값을 컬렉션으로 관리하고 싶을 경우 컬렉션을 만들고 그 컬렉션 내에 검증 로직을 두면 될 것 같습니다. 다만 저는 여기에서는 컬렉션을 또 만들어 (수동 로또의 개수 / 전체 로또의 개수)를 묶는 컬렉션을 만들거나, (수동 로또의 개수) 단일 값을 관리하는 컬렉션을 만들고 싶지 않았습니다. (수동 로또의 개수 / 전체 로또의 개수)를 묶는 컬렉션을 만들고 싶지 않은 이유
(수동 로또의 개수)만을 묶는 컬렉션을 만들고 싶지 않은 이유
그래서 컨트롤러에 로직을 두게 되었습니다. 두 값을 연관시켜 강하게 결합시킨 채로 관리하고 싶지 않고, 이미 존재하는 다른 컬렉션이 상당 부분 이 일을 대신해주고 있기 때문입니다. 그 외에도 컨트롤러에 로직을 둘 이유를 생각해 보았는데 두 모델의 도메인 성격이 꽤나 떨어진 경우에도 컨트롤러에 검증 로직을 둘 것 같습니다.
이상 제가 고민하고 생각해 본 것들의 결론입니다. 제 말이 무조건 맞다고 생각하지 않으며 코멘트를 적을 당시 조금이나마 알고 있는 지식과 생각을 바탕으로 적은 것이므로 의견이 갈릴만한 부분이 많은 것도 맞습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오,, 되게 좋은 인사이트네요,,검증 로직을 도메인 객체 내부에 두어 컬렉션이 항상 신뢰할 수 있는 상태임을 보장할 수도 있지만, 이번 경우처럼 두 값이 서로 연관되어 있고 이미 다른 컬렉션으로 관리되고 있다면 컨트롤러에서 검증하는 것도 충분히 합리적이라고 생각돼요. 자세하게 정리해주셔서 감사합니룽~ |
||
private void inputLottoPurchasePrice() { | ||
while (true) { | ||
try { | ||
lottoPurchasePrice = InputView.inputLottoPurchasePrice(); | ||
lottoCount = lottoPurchasePrice.getLottoCount(); | ||
break; | ||
} catch (Exception exception) { | ||
OutputView.printErrorMessage(exception.getMessage()); | ||
} | ||
} | ||
} | ||
|
||
private void inputManualLottoCount() { | ||
while (true) { | ||
try { | ||
int manualLottoCount = InputView.inputManualLottoCount(); | ||
if (manualLottoCount > lottoPurchasePrice.getLottoCount()) { | ||
throw new IllegalArgumentException("수동 로또의 개수가 전체 구매할 수 있는 로또 수인 " | ||
+ lottoPurchasePrice.getLottoCount() | ||
+ "개보다 많아서는 안 됩니다. 수동 로또의 개수를 줄여보세요."); | ||
} | ||
this.manualLottoCount = manualLottoCount; | ||
break; | ||
} catch (Exception exception) { | ||
OutputView.printErrorMessage(exception.getMessage()); | ||
} | ||
} | ||
} | ||
|
||
private void inputManualLottoList() { | ||
while (true) { | ||
try { | ||
manualLottoList = InputView.inputManualLottoList(manualLottoCount); | ||
break; | ||
} catch (Exception exception) { | ||
OutputView.printErrorMessage(exception.getMessage()); | ||
} | ||
} | ||
} | ||
|
||
private void makeLottoList(int lottoCount) { | ||
int autoLottoCount = lottoCount - this.manualLottoList.size(); | ||
List<Lotto> manualLottoList = this.manualLottoList.getValue(); | ||
List<Lotto> autoLottoList = IntStream.range(0, autoLottoCount) | ||
.mapToObj(i -> lottoGenerator.generate()) | ||
.collect(Collectors.toList()); | ||
List<Lotto> concatedLottoList = new ArrayList<>(); | ||
concatedLottoList.addAll(manualLottoList); | ||
concatedLottoList.addAll(autoLottoList); | ||
|
||
lottoList = new LottoList(concatedLottoList); | ||
OutputView.printLottoPurchases(lottoList, manualLottoCount, autoLottoCount); | ||
} | ||
|
||
private void inputWinningLottoInfo() { | ||
while (true) { | ||
try { | ||
final Lotto winningLotto = InputView.inputWinningLotto(); | ||
final BonusNumber bonusNumber = InputView.inputBonusNumber(); | ||
this.winningLottoInfo = new WinningLottoInfo(winningLotto, bonusNumber); | ||
break; | ||
} catch (Exception exception) { | ||
OutputView.printErrorMessage(exception.getMessage()); | ||
} | ||
} | ||
} | ||
|
||
private void printLottoPrizeResult() { | ||
try { | ||
OutputView.printLottoPrizeResult(lottoList.getLottoResultByWinningLottoInfo(winningLottoInfo)); | ||
} catch (Exception exception) { | ||
OutputView.printErrorMessage(exception.getMessage()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package model; | ||
|
||
public class BonusNumber { | ||
public final int bonusNumber; | ||
|
||
public BonusNumber(int bonusNumber) { | ||
validateBonusNumber(bonusNumber); | ||
this.bonusNumber = bonusNumber; | ||
} | ||
|
||
private void validateBonusNumber(int bonusNumber) { | ||
if (bonusNumber < LottoConstants.MIN_NUMBER.getValue() || bonusNumber > LottoConstants.MAX_NUMBER.getValue()) { | ||
throw new Error("보너스 넘버는 " + LottoConstants.MIN_NUMBER.getValue() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 일반적인 Error보다는 IllegalArgumentException 와 같은 에러를 사용하는게 좋을 것 같아요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 우왁, 이건 제 실수네요. 포괄적으로 적더라도 |
||
+ " 이상 " + LottoConstants.MAX_NUMBER.getValue() + " 이하여야 합니다."); | ||
} | ||
} | ||
|
||
public int getValue() { | ||
return bonusNumber; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package model; | ||
|
||
import java.util.*; | ||
|
||
public class Lotto { | ||
private final List<Integer> lotto; | ||
|
||
public Lotto(List<Integer> lotto) { | ||
validateLotto(lotto); | ||
this.lotto = getSortedLotto(lotto); | ||
} | ||
|
||
private void validateLotto(List<Integer> lotto) { | ||
if (lotto.size() != LottoConstants.NUMBER_COUNT.getValue()) { | ||
throw new IllegalArgumentException("로또 번호는 6개의 수로 이루어져야 합니다."); | ||
} | ||
|
||
lotto.forEach(lottoNumber -> { | ||
if (lottoNumber < LottoConstants.MIN_NUMBER.getValue() || lottoNumber > LottoConstants.MAX_NUMBER.getValue()) { | ||
throw new IllegalArgumentException("로또 번호는 " + LottoConstants.MIN_NUMBER.getValue() + | ||
" 이상 " + LottoConstants.MAX_NUMBER.getValue() + " 이하여야 합니다."); | ||
} | ||
}); | ||
|
||
Set<Integer> uniqueLottoNumbers = new HashSet<>(lotto); | ||
|
||
if (uniqueLottoNumbers.size() != LottoConstants.NUMBER_COUNT.getValue()) { | ||
throw new IllegalArgumentException("로또 번호는 중복되어서는 안 됩니다."); | ||
} | ||
} | ||
|
||
private List<Integer> getSortedLotto(List<Integer> lotto) { | ||
List<Integer> sortedLotto = new ArrayList<>(lotto); | ||
sortedLotto.sort(null); | ||
|
||
return sortedLotto; | ||
} | ||
|
||
public List<Integer> getValue() { | ||
return Collections.unmodifiableList(lotto); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return lotto.toString(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package model; | ||
|
||
public enum LottoConstants { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 제 생각엔 단순 상수를 enum으로 관리하는 건 약간 비효율적인게 아닐까? 라는 생각이 들어요. 그냥 정적 변수로 선언해도 괜찮을 것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 궁금한 점이 있어요. 사실 루루는 구조가 좀 복잡한 형태(특히 언급해주신 "연관된 데이터들을 한꺼번에 처리")의 상수 데이터라면 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
맞아요! 저는 딱 이생각으로 분리해요. 프론트 개발할 때 관련된 애들끼리 객체배열 만들어서 상수로 관리하잖아요 그런 느낌으로 사용합니다. 단일 값이면 그냥 final키워드로 선언하는게 타입적으로도, 사용할 때도 편하지만 이게 연관된 객체같은 느낌이면 하나하나 선언해서 쓰기 귀찮고 복잡하잖아요. 그리고 속성들끼리 연관적이라는걸 다른 로직으로 검증도 해줘야하구요 (예를들어 매치카운트가 6개면 1등이다 이런식으로의 연결) |
||
MIN_NUMBER(1), | ||
MAX_NUMBER(45), | ||
NUMBER_COUNT(6), | ||
PRICE(1_000); | ||
|
||
private final int value; | ||
|
||
LottoConstants(int value) { | ||
this.value = value; | ||
} | ||
|
||
public int getValue() { | ||
return value; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package model; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Random; | ||
|
||
public class LottoGenerator { | ||
final Random random = new Random(); | ||
|
||
public Lotto generate() { | ||
final List<Integer> lottoNumbers = new ArrayList<>(); | ||
|
||
while (lottoNumbers.size() < LottoConstants.NUMBER_COUNT.getValue()) { | ||
final int candidateLottoNumber = random.nextInt(LottoConstants.MIN_NUMBER.getValue(), | ||
LottoConstants.MAX_NUMBER.getValue()); | ||
|
||
if (!lottoNumbers.contains(candidateLottoNumber)) { | ||
lottoNumbers.add(candidateLottoNumber); | ||
} | ||
} | ||
|
||
return new Lotto(lottoNumbers); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package model; | ||
|
||
import java.util.ArrayList; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Set; | ||
|
||
public class LottoList { | ||
private final List<Lotto> lottoList; | ||
|
||
public LottoList(List<Lotto> lottoList) { | ||
this.lottoList = new ArrayList<>(lottoList); | ||
} | ||
|
||
public List<Lotto> getValue() { | ||
return lottoList; | ||
} | ||
|
||
public int size() { | ||
return lottoList.size(); | ||
} | ||
|
||
public LottoPrizeResult getLottoResultByWinningLottoInfo(WinningLottoInfo winningLottoInfo) { | ||
LottoPrizeResult lottoPrizeResult = new LottoPrizeResult(); | ||
|
||
lottoList.forEach(lotto -> { | ||
Set<Integer> lottoNumbers = new HashSet<>(lotto.getValue()); | ||
int matchNumberCount = (int) winningLottoInfo.getLottoValue().stream().filter(lottoNumbers::contains).count(); | ||
boolean hasBonusNumber = lottoNumbers.contains(winningLottoInfo.getBonusNumberValue()); | ||
|
||
lottoPrizeResult.addPrize(matchNumberCount, hasBonusNumber); | ||
}); | ||
|
||
return lottoPrizeResult; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package model; | ||
|
||
public enum LottoPrize { | ||
FIFTH(5_000, 3, false), | ||
FOURTH(50_000, 4, false), | ||
THIRD(1_500_000, 5, false), | ||
SECOND(30_000_000, 5, true), | ||
FIRST(2_000_000_000, 6, false); | ||
|
||
private final int money; | ||
private final int matchNumberCount; | ||
private final boolean hasBonusNumber; | ||
|
||
LottoPrize(int money, int matchNumberCount, boolean hasBonusNumber) { | ||
this.money = money; | ||
this.matchNumberCount = matchNumberCount; | ||
this.hasBonusNumber = hasBonusNumber; | ||
} | ||
|
||
public static LottoPrize getPrizeByMatchResult(int matchNumberCount, boolean hasBonusNumber) { | ||
for (LottoPrize lottoPrize: values()) { | ||
if (matchNumberCount != lottoPrize.matchNumberCount) { | ||
continue; | ||
} | ||
|
||
if (matchNumberCount == 5 && hasBonusNumber != lottoPrize.hasBonusNumber) { | ||
continue; | ||
} | ||
|
||
return lottoPrize; | ||
} | ||
|
||
return null; | ||
} | ||
|
||
public int getMoney() { | ||
return money; | ||
} | ||
|
||
public int getMatchNumberCount() { | ||
return matchNumberCount; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
package model; | ||
|
||
import java.util.*; | ||
|
||
public class LottoPrizeResult { | ||
private final Map<LottoPrize, Integer> prizeResult; | ||
int lottoCount; | ||
|
||
public LottoPrizeResult() { | ||
this.prizeResult = new EnumMap<LottoPrize, Integer>(LottoPrize.class); | ||
this.lottoCount = 0; | ||
|
||
for (LottoPrize lottoPrize : LottoPrize.values()) { | ||
prizeResult.put(lottoPrize, 0); | ||
} | ||
} | ||
|
||
public Map<LottoPrize, Integer> getValue() { | ||
return Collections.unmodifiableMap(prizeResult); | ||
} | ||
|
||
public void addPrize(int matchNumberCount, boolean hasBonusNumber) { | ||
LottoPrize currentLottoPrize = LottoPrize.getPrizeByMatchResult(matchNumberCount, hasBonusNumber); | ||
|
||
if (currentLottoPrize != null) { | ||
prizeResult.put(currentLottoPrize, prizeResult.get(currentLottoPrize) + 1); | ||
} | ||
|
||
lottoCount += 1; | ||
} | ||
|
||
public double getProfitRate() { | ||
int purchasePrice = lottoCount * LottoConstants.PRICE.getValue(); | ||
|
||
if (purchasePrice == 0) { | ||
return 0; | ||
} | ||
|
||
int totalPrizeMoney = | ||
prizeResult.entrySet().stream() | ||
.mapToInt(entry -> entry.getKey().getMoney() * entry.getValue()) | ||
.sum(); | ||
|
||
return (double) totalPrizeMoney / purchasePrice; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package model; | ||
|
||
public class LottoPurchasePrice { | ||
private final int lottoPurchasePrice; | ||
|
||
public LottoPurchasePrice(int lottoPrice) { | ||
validateLottoPrice(lottoPrice); | ||
this.lottoPurchasePrice = lottoPrice; | ||
} | ||
|
||
private void validateLottoPrice(int lottoPurchasePrice) { | ||
if (lottoPurchasePrice < 0) { | ||
throw new IllegalArgumentException("로또 총 구입금액은 음수가 될 수 없습니다."); | ||
} | ||
|
||
if (lottoPurchasePrice % LottoConstants.PRICE.getValue() != 0) { | ||
throw new IllegalArgumentException("로또 총 구입금액은 " + LottoConstants.PRICE.getValue() + | ||
"로 나누어 떨어져야 합니다."); | ||
} | ||
} | ||
|
||
public int getValue() { | ||
return lottoPurchasePrice; | ||
} | ||
|
||
public int getLottoCount() { | ||
return lottoPurchasePrice / LottoConstants.PRICE.getValue(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨트롤러가 너무 많은 상태를 가지고 있는 것 같은데 꼭 필수적인 상태 주입을 제외하고 줄여보는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨트롤러를 너무 잘게 나누다 보니까 메서드와 메서드 간에 공유해야 하는 값이 있어 사실상 전역과 비슷한 상태로 두게 되어 버렸군요. 상태가 너무 많다는 건 동의합니다 -- 사실상 한 번만 공유하고 끝나는 1회성 필드들은 이렇게 둘 필요는 없었던 것 같아요.
다음에 작업할 때에는 큰 단위의 메소드와 작은 단위의 메소드를 만들고, 큰 단위의 메소드에 지역 변수처럼 선언하는 식으로 변경해볼까 합니다, 혹시 또다른 묘안이 떠오른다면 알려주세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 것 같아요. 확실히 지금 다 갈아엎기엔 좀 대공사긴 하네요ㅋㅋㅋㅋㅋ