[SCG] 이진우 로또 1~2단계 미션 제출합니다. #164
[SCG] 이진우 로또 1~2단계 미션 제출합니다. #164codeTravelerLee wants to merge 19 commits intonext-step:codetravelerleefrom
Conversation
kjyyjk
left a comment
There was a problem hiding this comment.
안녕하세요 진우님~
페어 프로그래밍은 낯선 방식이었을텐데도 페어분과 함께 잘구현해주셨네요ㅎㅎ
고생하셨습니다.
본문에 말씀주신 내용들은 당장 다루기보다는 리뷰 주고 받으며 차근차근 다뤄보면 좋을 것 같아요.
우선 코드에 코멘트 몇개 남겨드렸으니 확인하시고 다시 리뷰요청 주세요!
이후 3~4단계도 있으니 만약 테스트 코드 추가를 제외하고 다른 코멘트를 전부 다루셨다면,
다시 리뷰 요청주셔도 됩니다!
추가로 궁금한 점 있으면 언제든지 디코 스레드나 디엠으로 연락주세요.
당첨 로또번호 검증로직을 어디서 수행해야 할까? -> 클래스 내부라고 생각했습니다.
현재 로또 번호에 대한 검증코드가 하나도 없네요. (1-45 범위 외 정수)
사실 자바로 구현한 모든게 클래스 내부라고 할 수 있죠.
어떤 클래스 내부에 구현하면 좋을지 고민해보시고 구현해주세요.
| class LottoTest { | ||
| } No newline at end of file |
There was a problem hiding this comment.
로또 시스템이 오작동하면 대한민국이 불바다가 될 수도 있을 것 같아요🔥
매번 애플리케이션을 실행해서 모든 경우를 눈으로 확인하기는 힘들 것 같은데 테스트 코드를 작성해보는건 어떨까요~?
| private static final LottoController lotto = new LottoController(); | ||
| public static void main(String[] args) { | ||
| lotto.run(); | ||
| } |
There was a problem hiding this comment.
아래 예시와 같이 Application의 main 메서드에 구현했어도 될 것 같은데,
LottoController로 분리하신 이유는 무엇인가요? 🤔
Controller의 역할을 Application이 할 수는 없는걸까요?
public class Application {
public static void main(String[] args) {
InputView inputView = new InputView();
ResultView resultView = new ResultView();
ErrorView errorView = new ErrorView();
int purchaseAmount = inputView.getPurchaseAmount();
Lottos lottos = new Lottos(purchaseAmount);
List<Lotto> allLottos = lottos.getLottos();
resultView.printAllLottos(allLottos);
Lotto winningLotto = getValidWinningLotto();
resultView.printWinningLottoStatistics(purchaseAmount, winningLotto.getNumbers(), allLottos);
}
There was a problem hiding this comment.
application의 main메소드한테 프로그램의 "실행" 이라는 단일 책임만 주기 위해서 이렇게 했어요!
나중에 로또게임 뿐만 아니라, 사다리게임 등등 서비스의 기능이 추가된다면,
main안에 이 모든걸 넣으면 가독성, 유지보수성도 떨어질 뿐더러 main메소드가 여러 책임을 갖게 될 것 같아요.
컨트롤러를 별도로 만들어두면, 나중에 사다리게임이 추가됐을때 ladderGameController를 별도로 만들고, apllication은 이 여러 프로그램들을 "실행" 만 시켜주는거죠!
There was a problem hiding this comment.
내용과는 별개로,
main에서 컨트롤러로 생성자를 통해 의존성을 주입하도록 구조를 변경했어요!
import controller.LottoController;
import view.ErrorView;
import view.InputView;
import view.ResultView;
public class Application {
public static void main(String[] args) {
InputView inputView = new InputView();
ResultView resultView = new ResultView();
ErrorView errorView = new ErrorView();
LottoController lottoController = new LottoController(inputView, resultView, errorView);
lottoController.run();
}
}There was a problem hiding this comment.
혹시 mvc라고 해서 무작정 분리하지는 않았는지 드린 질문이었는데,
의도를 가지고 분리를 하신 것 같아 좋네요!
|
|
||
| for(MatchResult result: MatchResult.values()) { | ||
| if(singleEqualCount == result.getMatchCount()) { | ||
| resultMap.put(result, resultMap.get(result) + 1); |
There was a problem hiding this comment.
getOrDefault()에 대해 학습하고 적용해볼까요?
아래 코드가 필요 없을 수도 있겠네요.
for(MatchResult result: MatchResult.values()) {
resultMap.put(result, 0);
}
| for(Lotto singleLotto: allLottos) { | ||
| int singleEqualCount = 0; | ||
| for (Integer number : winningNumbers) { | ||
| if (singleLotto.getNumbers().contains(number)) { |
There was a problem hiding this comment.
WinningLotto가 Lotto.numbers 리스트를 꺼내와서 number가 들어있는 지를 직접 체크하네요!
당첨 번호가 포함되어있는지 확인하는 책임을 numbers를 들고 있는 Lotto에게 위임해보는건 어떨까요?
이러한 변경에는 어떤 장점이 있을까요?
There was a problem hiding this comment.
좋네요! 만약에 "당첨"을 규정하는 비즈니스 로직에 변경이 생기면 기존 코드에서는 Lotto와 관련 없는 WinningLotto 클래스를 수정해야 하는 웃긴 상황이 발생할 수 있었겠네요.
numbers가 Lotto의 필드이니,
Tell, Don't ask 원칙을 지키려면 당첨번호가 포함되어있는지 확인은 Lotto 클래스에서 하는것이 맞겠네요.
이렇게 하면 클래스간 결합도가 낮아지고, 각 클래스 내에선 응집도가 높아지겠네요.
수정된 코드는 아래와 같습니다.
Commit 9cc93af
WinningLotto.java 내 getMatchResult 메서드
public HashMap<MatchResult, Integer> getMatchResult(List<Lotto> allLottos, List<Integer> winningNumbers) {
HashMap<MatchResult, Integer> resultMap = new HashMap<>();
for (Lotto singleLotto : allLottos) {
int singleEqualCount = 0;
for (Integer number : winningNumbers) {
singleEqualCount += singleLotto.getWinningNumberMatchCount(number);
}
for (MatchResult result : MatchResult.values()) {
if (singleEqualCount == result.getMatchCount()) {
resultMap.put(result, resultMap.getOrDefault(result, 0) + 1);
}
}
}
return resultMap;
}Lotto.java 에 추가된 메소드
public int getWinningNumberMatchCount(Integer number) {
int matchCount = 0;
if (numbers.contains(number)) {
matchCount++;
}
return matchCount;
}There was a problem hiding this comment.
getWinningNumberMatchCount는 number가 포함되면 1, 아니면 0.
이 두 값만을 반환하겠네요.
저는 아래 두 방식들이 더 자연스러워보입니다!
- 모든 당첨 번호를 numbers 파라미터로 넘겨 당첨 개수 int를 반환하게 하거나
- number가 포함되는지 boolean으로 반환하여 true면 외부에서 +1하는 식
src/main/java/domain/Lottos.java
Outdated
| List<Integer> numbers = getSingleLotto(); | ||
| lottos.add(new Lotto(numbers)); |
There was a problem hiding this comment.
메서드명은 getSingleLotto인데 정작 반환하는 값은 로또가 아닌 로또 번호들이네요.
getSingleLotto 메서드가 Lotto를 반환하도록 변경해볼까요?
There was a problem hiding this comment.
오호! 그러네요. 일급컬렉션과 친해지려면 아직 멀었군요 하하.
수정완료! (commit: 7355f80)
Lottos.java
private List<Lotto> generateLottos(int lottoCount) {
List<Lotto> lottos = new ArrayList<>();
for (int i = 0; i < lottoCount; i++) {
Lotto lotto = getSingleLotto();
lottos.add(lotto);
}
return lottos;
}
private Lotto getSingleLotto() {
ArrayList<Integer> lottoNumbers = generateLottoNumbersArray();
Collections.shuffle(lottoNumbers);
List<Integer> subNumbers = lottoNumbers.subList(0, 6);
Collections.sort(subNumbers);
return new Lotto(subNumbers);
}| } | ||
|
|
||
| for(MatchResult result: MatchResult.values()) { | ||
| if(singleEqualCount == result.getMatchCount()) { |
There was a problem hiding this comment.
당첨 개수에 따른 MatchResult 계산 책임을 matchCount를 들고 있는 MatchResult에게 위임해보는 것은 어떨까요?
There was a problem hiding this comment.
matchResult = MatchResult.getByMatchCount(singleEqualCount);
와 같은 형태일 수 있겠네요.
There was a problem hiding this comment.
반영완료! WinningLotto#getMatchResult 가 제 코드에서 가장 복잡한 메소드중 하나였는데, 아주 간결해졌네요 👍👍
src/main/java/view/InputView.java
Outdated
|
|
||
| } catch (NumberFormatException e) { | ||
| System.out.println("숫자만 입력해주세요!"); | ||
| System.exit(-1); |
There was a problem hiding this comment.
ErrorView가 별도로 있음에도 InputView에서 에러 정보를 출력하고, 재시도 없이 앱을 종료시켜버리네요. 👀
여기서는 예외 발생/변환만하고, LottoController#getValidWinningLotto에서 예외를 처리해볼까요?
There was a problem hiding this comment.
그렇네요. 수정 완료!
commit : 7b9b209
Inputiview#getWinningNumbers
public List<Integer> getWinningNumbers() {
System.out.println("지난 주 당첨 번호를 입력해주세요.");
List<Integer> winningNumbers = new ArrayList<>();
String numbers = scanner.nextLine();
String[] numbersArr = numbers.split(",");
for (String number : numbersArr) {
winningNumbers.add(Integer.parseInt(number.trim()));
}
return winningNumbers;
}
}LottoController#getValidWinningLotto
private Lotto getValidWinningLotto() {
try {
List<Integer> numbers = inputView.getWinningNumbers();
return new Lotto(numbers);
} catch (NumberFormatException e) {
errorView.printErrorMessage("숫자만 입력해주세요!");
return getValidWinningLotto();
} catch (IllegalArgumentException e) {
errorView.printErrorMessage(e.getMessage());
return getValidWinningLotto();
}
}
src/main/java/view/InputView.java
Outdated
| } | ||
| } | ||
|
|
||
| public List<Integer> getWinningNumbers () { |
There was a problem hiding this comment.
InputView#getWinningNumbers 에 아래와 같이 음수값이 존재하면 예외를 발생시키도록 코드를 수정해봤어요.
String numbers = scanner.nextLine();
String[] numbersArr = numbers.split(",");
for (String number : numbersArr) {
if (Integer.parseInt(number) < 0) {
throw new IllegalArgumentException("당첨번호는 음수일 수 없어요!");
}
winningNumbers.add(Integer.parseInt(number.trim()));
}|
|
||
| private static final Scanner scanner = new Scanner(System.in); | ||
|
|
||
| public int getPurchaseAmount() { |
There was a problem hiding this comment.
당첨번호 입력 재시도는 LottoController에서,
구매금앱 입력 재시도는 InputView에서 하고 있네요.
의도하신게 아니라면 통일할 수 있을 것 같아요.
재시도 로직은 어디에 위치하면 좋을까요?
진우님이 생각하시는 InputView의 책임은 어디까지인가요?
There was a problem hiding this comment.
아래와 같이 컨트롤러에서 검증하도록 변경했어요. 26db84b
private int getValidPurchaseAmount() {
try {
return inputView.getPurchaseAmount();
} catch (NumberFormatException e) {
errorView.printErrorMessage("숫자만 입력해주세요!");
return getValidPurchaseAmount();
} catch (IllegalArgumentException e) {
errorView.printErrorMessage(e.getMessage());
return getValidPurchaseAmount();
}
}의도했던 것은 아니고, 처음엔 inputview 안에서 검증을 하도록 작성했다가,
inputView 가 출력만 담당하도록 검증 로직을 컨트롤러로 이관하는 과정에서,
당첨 번호를 입력받는 경우만 이관하고, 구입 금액 입력에 대한 검증 로직은 컨트롤러로의 이전을 빼먹은 상황이었네요.
저는 InputView는 정말 출력만 담당해야 한다고 생각하는데,
준영님은 어떻게 생각하시나요?!
There was a problem hiding this comment.
정답은 없지만 저도 InputView 밖에서 재시도하는게 좋아보입니다!
예외 발생 시 프로그램을 종료할 지, 다시 입력을 받을지 InputView가 알 필요는 없다고 생각하거든요.
| public enum MatchResult { | ||
| THREE(3, 5_000), | ||
| FOUR(4, 50_000), | ||
| FIVE(5, 1_500_000), | ||
| SIX(6, 2_000_000_000), | ||
| MISS(0,0); |
There was a problem hiding this comment.
로또 당첨 상금을 enum을 활용해 잘 구현해주셨네요!
금액에 _ 표기해준 디테일까지 👍
There was a problem hiding this comment.
나름 긴 고민 끝에 도입한 녀석이었습니다 ㅎㅎ 뿌듯합니다.
|
준영님께서 반영해주신 사안들 반영했어요! 우선 테스트코드 제외하고 다른 사항들 리뷰 부탁드릴게요! |
kjyyjk
left a comment
There was a problem hiding this comment.
리뷰 잘 반영해주셨네요. 💯
몇가지 코멘트 더 드렸는데 확인해보시고 다시 리뷰 요청주세요!
다음 리뷰 요청시에는 머지해드릴 예정이며, 바로 다음 차시 진행해주시면 됩니다~
| private static final LottoController lotto = new LottoController(); | ||
| public static void main(String[] args) { | ||
| lotto.run(); | ||
| } |
There was a problem hiding this comment.
혹시 mvc라고 해서 무작정 분리하지는 않았는지 드린 질문이었는데,
의도를 가지고 분리를 하신 것 같아 좋네요!
| for(Lotto singleLotto: allLottos) { | ||
| int singleEqualCount = 0; | ||
| for (Integer number : winningNumbers) { | ||
| if (singleLotto.getNumbers().contains(number)) { |
There was a problem hiding this comment.
getWinningNumberMatchCount는 number가 포함되면 1, 아니면 0.
이 두 값만을 반환하겠네요.
저는 아래 두 방식들이 더 자연스러워보입니다!
- 모든 당첨 번호를 numbers 파라미터로 넘겨 당첨 개수 int를 반환하게 하거나
- number가 포함되는지 boolean으로 반환하여 true면 외부에서 +1하는 식
|
|
||
| private static final Scanner scanner = new Scanner(System.in); | ||
|
|
||
| public int getPurchaseAmount() { |
There was a problem hiding this comment.
정답은 없지만 저도 InputView 밖에서 재시도하는게 좋아보입니다!
예외 발생 시 프로그램을 종료할 지, 다시 입력을 받을지 InputView가 알 필요는 없다고 생각하거든요.
| public Lotto(List<Integer> numbers) { | ||
| validateSize(numbers); | ||
| validateDuplicate(numbers); | ||
| this.numbers = numbers; | ||
| } |
There was a problem hiding this comment.
만약 numbers에 1~45 범위를 벗어나는 숫자가 포함되어있으면 어떻게 될까요?
이를 방지하려면 Lotto 생성자 내에서 각 number들을 검증해야겠네요.
There was a problem hiding this comment.
프로그래밍 요구사항: 모든 원시 값과 문자열을 포장한다
number 원시 값을 LottoNumber로 포장하여 로또 번호에 대한 책임을 위임하고,
Lotto는 6개의 고유한 로또 번호들에 대한 책임만 가지도록 변경해볼까요?
현재 로또 번호에 대한 검증 로직이 당첨 번호 입력쪽에도 있는데,
LottoNumber를 활용하면 한 곳에서만 관리할 수 있을 것 같아요.
| public void printWinningLottoStatistics (final int purchaseAmount, List<Integer> winningNumbers, List<Lotto> allLottos) { | ||
| HashMap<MatchResult, Integer> resultMap = winningLotto.getMatchResult(allLottos, winningNumbers); |
There was a problem hiding this comment.
구입한 Lotto들과 당첨 번호를 WinningLotto#getMathchResult가 비교하고 계산하네요.
해당 로직을 Lottos에 위임해보는건 어떨까요?
구입한 Lotto들에 대한 책임을 가진 Lottos가 당첨 번호를 기반으로 당첨 정보를 계산하는 것이 더 자연스럽다고 생각해요.
| } | ||
| } | ||
|
|
||
| public void printWinningLottoStatistics (final int purchaseAmount, List<Integer> winningNumbers, List<Lotto> allLottos) { |
There was a problem hiding this comment.
출력하는 중에 외부에서 List에 값을 추가하면 어떻게 될까요?
winningNumbers와 allLottos는 불변을 보장하지 않아도 괜찮을까요??
There was a problem hiding this comment.
반대로 ResultView#printWinningLottoStatistics 내에서
allLottos.add(new Lotto())와 같은 로직을 수행하면 구매한 로또 개수가 달라질 수도 있겠네요.
불변 컬렉션에 대해 공부하고
Lottos, Lotto가 불변 컬렉션을 반환하도록 변경해볼까요?
| public Lotto(List<Integer> numbers) { | ||
| validateSize(numbers); | ||
| validateDuplicate(numbers); | ||
| this.numbers = numbers; |
There was a problem hiding this comment.
Lotto를 생성한 이후, 외부에서 numbers에 접근하여 숫자를 추가하면 어떻게 될까요?
방어적 복사에 대해 학습하고 적용해보세요!
https://medium.com/sjk5766/%EB%B0%A9%EC%96%B4%EC%A0%81-%EB%B3%B5%EC%82%AC-defensive-copy-f85922b4891c
준영님 안녕하세요! SCG 이진우입니다!
백엔드스터디 3주차 과제를 제출합니다!
JAVA라는 녀석과 함께한지 이제 겨우 3주가 되어 아직 OOP 뿐만아니라 JAVA의 문법 자체도 많이 부족하지만,
양질의 스터디 자료들, 리뷰어님들의 리뷰, 지옥의 구글링을 통해 나름(?) 어색함을 극복해나가는 중입니다.
😎 신경쓴 점
🤮 어려웠던 점
리뷰 기다릴게요,☺️
감사합니다