Skip to content

Conversation

@NDjust
Copy link

@NDjust NDjust commented May 22, 2020

이번 주 로또 미션 제출합니다!

@@ -0,0 +1,35 @@
package com.javabom.lotto.domain;

public class LottoInfo {
Copy link
Member

Choose a reason for hiding this comment

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

클린 코드 서적에 "불용어 (noise word)를 추가하는 방식은 적절하지 못하다" 라는 말이있어요.
Data, Info, The 같은 단어들을 불용어라고 부르고있어요.
의미가 불분명해지고 의도를 알 수 없기때문이라고 생각합니다.

Copy link
Author

Choose a reason for hiding this comment

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

아하 감사합니다!

Comment on lines 8 to 11
public LottoInfo(String gameMoney) {
validGameMoney(gameMoney);
this.gameMoney = Integer.parseInt(gameMoney);
}
Copy link
Member

Choose a reason for hiding this comment

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

String을 생성사로 받기보단 Int로 받는게 좋아보여요
Input View 쪽에서 정수로 파싱후 가져오는게 나은거같아요!

Copy link
Author

Choose a reason for hiding this comment

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

그럼 InputView에서 gameMoney에 대한 유효성 검사를 해야하는데, View 로직에서 유효성 검사를 하는게 맞지 않는 거 같아서 저렇게 처리 했는데 다른 방향이 더 나을까요??

Copy link
Member

@malibinYun malibinYun May 22, 2020

Choose a reason for hiding this comment

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

제생각에는 단순히 숫자인 문자열인지만 판단하면 된다 생각했고.
nextInt나 parseint를 해올때 숫자가 아닌 문자열이면 Exception이 애초에 발생하니 괜찮다고 생각했습니다. 이게 100% 맞는 방법인지는 저도 잘 모르겠어요

import java.util.ArrayList;
import java.util.List;

public class LottoNumber {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class LottoNumber {
public class LottoNumbers {

뒤에 s를 붙이는 편이 더 좋아보입니다. 처음 봤을 때 이 객체가 List를 들고있다는 생각을 하지 못했어요.

Comment on lines 10 to 12
public LottoNumber(NumberShuffler shuffler) {
lottoNumber = shuffler.getNumbers();
}
Copy link
Member

Choose a reason for hiding this comment

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

NumberShuffler를 받고 거기서 getNumbers를 하고있는데,
아예 생성자에서 NumberShuffler를 받기보다 List를 받는게더 좋아보아요!

private final List<Integer> lottoNum;

public LottoNumberShuffler() {
lottoNum = Arrays.stream(IntStream.range(1, 46).toArray())
Copy link
Member

Choose a reason for hiding this comment

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

1, 46 같은 정수값들이 다른사람들이 보았을 때 무슨 의미인지 알 수 없으니
상수로 선언을 하여서 의미를 부여하는것이 좋다고 생각합니다.
밑에 0,6도 마찬가지!

Copy link

Choose a reason for hiding this comment

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

rangeClosed 를 사용해서 46말고 45라는 더 의미있는 상수를 쓸수도 있을거같아요.

import java.util.*;
import java.util.stream.Collectors;

public class WinningNumber {
Copy link
Member

Choose a reason for hiding this comment

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

이 클래스가 너무 많은 일을 하고있다고 생각해요.
기능들을 좀더 분리할수 있을거예요!

Comment on lines 28 to 33
.map(m -> {
validNumberFormat(m);
validLottoNumber(m);
return Integer.parseInt(m);
})
.collect(Collectors.toList());
Copy link
Member

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.

@nightmare73
아항 stream도 인덴트 하나로 보나요?..

Copy link
Member

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.

아하 네넵 분리해보겠습니다! 가독성을 위해서도 분리하는 게 좋을 듯 싶네요!

Comment on lines 77 to 79
private boolean isEmptyNumbers(String[] numbers) {
return Arrays.stream(numbers).map(String::trim).allMatch(s -> s.equals(""));
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private boolean isEmptyNumbers(String[] numbers) {
return Arrays.stream(numbers).map(String::trim).allMatch(s -> s.equals(""));
}
private boolean isEmptyNumbers(String[] numbers) {
return Arrays.stream(numbers)
.map(String::trim)
.allMatch(String::isEmpty);
}

Comment on lines 13 to 24
@ParameterizedTest
@DisplayName("정수가 값과 음수 값이 들어올 시 예외처리.")
@CsvSource(value = {"error,-1"})
public void validIntegerValue(String notInteger, String negativeValue) {
assertThatThrownBy(() -> new LottoInfo(notInteger))
.isInstanceOf(NumberFormatException.class)
.hasMessage("정수가 아닌 값이 입력 되었습니다.");

assertThatThrownBy(() -> new LottoInfo(negativeValue))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("구입금액이 음수 값으로 입력되었습니다.");
}
Copy link
Member

Choose a reason for hiding this comment

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

한 메소드가 두개의 테스트를 하고있네요
분리 가능하다고 봅니다

Copy link

Choose a reason for hiding this comment

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

에러메시지를 파라미터로 받고 @ParameterizedTest 를 잘 활용하면 assert 하나로 두개 다 테스트할 수 있을 것 같아요.

// given
LottoNumberShuffler lottoNumberShuffler = new LottoNumberShuffler();

assertEquals(6, lottoNumberShuffler.getNumbers().size());
Copy link
Member

Choose a reason for hiding this comment

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

assertJ 를 사용해서 작성하는게 더 가독성이 좋아요!

Copy link
Contributor

@jyami-kim jyami-kim left a comment

Choose a reason for hiding this comment

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

늦어서 많이는 못봤습니다ㅠ

this.gameMoney = Integer.parseInt(gameMoney);
}

private void validGameMoney(String gameMoney) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CleanCode에 보면 추상화 수준을 맞춰서 한단계씩 낮추라는 말이 있습니다.
메서드 작성을 목차 작성하듯이 한 단계씩 내려가라는 말인데, 이런 관점에서
valid를 이렇게 체크한건 추상화 수준을 고려한것 같아서 보기 좋아요 b


public List<LottoNumber> getNumbers() {
return new ArrayList<>(numbers);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

new ArrayList로 만드는 것보다.
Collections.unmodifiableList(list); 를 사용하는걸 추천드립ㄴ디ㅏ.
numbers 필드를 기껏 final로 만들어서 불변으로 만들었는데, new ArrayList를 통해 밖으로 빼내는 작업을 할 경우, 외부에서 ArrayList의 메서드를 사용해서 추가 삭제등의 변경 가능성이 있습니다.

Copy link
Author

Choose a reason for hiding this comment

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

아하 감사합니다!

import java.util.ArrayList;
import java.util.List;

public class LottoNumberFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting
commit할 때, intellij 기능을 사용해주세요, code formatting, import optimize 등의 작업을 해줍니다.
지금은 상관없지만, 나중에 팀작업할 때 formatting으로 인한 코드 변경이 있으면 Pr 보기가 불편해요.

Copy link
Contributor

Choose a reason for hiding this comment

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

어떤게 실제 변경인지 헷갈리고, formatting으로 인한 diff가 많아지면 리뷰가 피곤해져서요

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 List<LottoNumber> numbers;

public LottoNumbers(int gameMoney, NumberShuffler shuffler) {
numbers = LottoNumberFactory.getLottoNumbers(gameMoney / LottoInfo.lottoPrice, shuffler);
Copy link
Contributor

Choose a reason for hiding this comment

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

gameMoney/LottoInfo.lottoPrice
로컬변수를 이용해서 서술적으로 변수명을 작성해주세요. 이게 무슨의미인지 알아볼 수 있게
안에 들어가서 lottoCount 라는걸 알지 않아도 바깥에서 알 수 있게!

Copy link

Choose a reason for hiding this comment

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

LottoNumbers 라는 이름과 맞지 않는 로직이지 않나 싶어요.
LottoNumbers객체가 gameMoney를 받아 몇장인지 계산하는 역할을 가지고 있는 것이 자연스러운가?라는 의문이 듭니다.
제 생각일 뿐이에여ㅎㅅㅎ

numbers = LottoNumberFactory.getLottoNumbers(gameMoney / LottoInfo.lottoPrice, shuffler);
}

public List<LottoRank> findLottoRanks(List<Integer> winningNumber, int bonusNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

로또 번호에 대한 List를 LottoNumber 클래스로 관리한 것 처럼, winningNumber도 List가 아닌 WinningLottoNumber 등의 자료구조형 클래스로 관리해주면 좋을 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

그렇게 할경우 bonusNumber도 한 클래스에서 관리가 가능하겠죠

Copy link
Contributor

Choose a reason for hiding this comment

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

엥 WinningNumber 클래스가 있는데 그걸 받지 않고 List 와 int bonusNumber를 따로 받네요..? 만약 다르면 어떻게 되는거죠ㅎㅎㅎ...

Copy link
Author

Choose a reason for hiding this comment

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

좀 더 객체를 세분화해서 구현해야겠네요. 감사합니다 ㅎㅎ

Comment on lines 17 to 21
.map(m -> {
int sameCountByWinnerNumber = m.getSameCountByWinnerNumber(winningNumber);
boolean hasBonusNumber = m.hasBonusNumber(bonusNumber);
return LottoRank.findLottoRank(sameCountByWinnerNumber, hasBonusNumber);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

람다에 3줄이상 쓰지말라고 유명한 프로그래머분이 일침하셨는데, 딱 3줄이네요ㅋㅋㅋㅋㅋ
일침의 중간에 이씀,,

LottoNumbers lottoNumbers = new LottoNumbers(lottoInfo.getGameMoney(), new LottoNumberShuffler());
OutputView.printLottoNumbers(lottoNumbers.getNumbers());
WinningNumber winningNumber = new WinningNumber(InputView.askWinningNumber(), InputView.askBonusNumber());
WinningStatistics winningStatistics = new WinningStatistics(
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 가독성이 너무 안좋아요.. 생성자에 대한 파라미터스가 너무 많은건아닌지, 불필요하게 get으로 꺼내서 계산하고있는건 없는지
후자라면 구조를 개선하세요.

}

private void validWinningNumberCount(List<Integer> winningNumber) {
if (winningNumber.size() != 6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

여기나오는 6, 45 0 등의 상수가 LottoNumberShuffler 와 같은 상수값을 지닙니다.
저는 나단님이 LottoNumberShuffler를 인터페이스 구현체로 선택한 이유가
NumberShuffler를 파생한 구현체들이 0, 45 가 아닌 0, 70 등의 로또 넘버가 늘어나는 경우의 수를 고려했나? 라고 의도를 파악했었는데요.
여기의 WinningNumber 클래스에서 이 상수들이 0, 45, 6 등으로 그냥 사용을 해버린다면 LottoNumberShuffler 를 인터페이스 구현체로 선택한 의도가 조금 퇴색되는것 같습니다.
구조를 개선합시다.

Comment on lines 5 to 6
private final int gameMoney;
public static final int lottoPrice = 1000;
Copy link

@csbsjy csbsjy May 26, 2020

Choose a reason for hiding this comment

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

public static final int LOTTO_PRICE = 1000;

static final 이 붙은 변수를 "상수"라고해요, LOTTO_PRICE 와 같이 대문자, _ 조합으로 사용하고 클래스의 최상단에 위치하는 것이 컨벤션입니다 !

LottoInfo 객체의 역할이 뭔가요? 객체의 이름과 상태, 그리고 메서드를 봐도 어떤 역할을 하는 객체인지 판단을 하기가 어려워요. 로또게임에 대한 정보인 로또의 가격과 사용자에게 입력을 받는 게임머니가 함께 존재하는 것도 약간은 어색하게 느껴지네요.

private final List<Integer> lottoNum;

public LottoNumberShuffler() {
lottoNum = Arrays.stream(IntStream.range(1, 46).toArray())
Copy link

Choose a reason for hiding this comment

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

rangeClosed 를 사용해서 46말고 45라는 더 의미있는 상수를 쓸수도 있을거같아요.

List<LottoNumber> lottoNumbers = new ArrayList<>();

for (int i = 0; i < lottoCount; i++) {
lottoNumbers.add(new LottoNumber(shuffler));
Copy link

Choose a reason for hiding this comment

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

shuffler에 대한 의존을 LottoNumber까지 넘길필요가 있을까라는 생각이듭니다. 위에 LottoNumber 생성자에대한 혁님 코멘트와 같은 의견입니다 :)

private final List<LottoNumber> numbers;

public LottoNumbers(int gameMoney, NumberShuffler shuffler) {
numbers = LottoNumberFactory.getLottoNumbers(gameMoney / LottoInfo.lottoPrice, shuffler);
Copy link

Choose a reason for hiding this comment

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

LottoNumbers 라는 이름과 맞지 않는 로직이지 않나 싶어요.
LottoNumbers객체가 gameMoney를 받아 몇장인지 계산하는 역할을 가지고 있는 것이 자연스러운가?라는 의문이 듭니다.
제 생각일 뿐이에여ㅎㅅㅎ

THIRD_PLACE(1500000, 5),
SECOND_PLACE(30000000, 5),
FIRST_PLACE(2000000000, 6 ),
FAIL(0, -1);
Copy link

Choose a reason for hiding this comment

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

sameCount가 -1 인 이유가 있을까요? FAIL은 하나도 맞지 않을 때(sameCount가 0일때) 인게 자연스럽지 않을까요?


private List<Integer> splitWinningNumber(String winningNumber) {
validSplitNumber(winningNumber);
return Arrays.stream(winningNumber.split(","))
Copy link

Choose a reason for hiding this comment

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

WinningNumber는 도메인객체로 보이는데 인풋으로 들어오는 ,로 분리까지 하고있네요, view 와 관련된 로직이 도메인까지 들어온 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

피드백 감사합니다 ㅎㅎ

위 내용에 대해 궁금한 점이 viwe와 관련된 로직에서 InputView가 입력받은 값에 대해서 도메인에 맞게 검사와 자료형까지 변환을 진행 하는게 맞는지가 궁금합니다! 저는 View는 그냥 입력값 그 자체를 받아오고 도메인에서 해당 값을 처리하는 식으로 했거든요. 본 어플리케이션은 항상 View를 통해 값을 받아와서 역할을 수행한다고 생각해서 저렇게 작성 했습니다!

근데 재연님 말씀대로 VIew에서 받은 값이 잘못되면 View에서 터지는게 맞는 거 같기도 해서 헷갈리네요ㅜㅜ

Copy link

Choose a reason for hiding this comment

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

, 는 콘솔입력이라는 View 에 많이 종속되어있는 입력의 Delimiter라고 생각해요. 그래서 도메인에 최대한 노출이 되지 않는 것이 좋을 것 같고, 나단님이 무슨 말씀하시는지는 알거같아요. 만약 View는 무조건 입력만! 그 리고 다른 처리는 다른객체에! 라는 생각을 가지고 계신다면, 도메인으로 가기전에 Dto 객체를 만들수도 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

오오 감사합니다! 정리가 되네요 ㅎㅎ

this.results = results;
}

private int calculateRevenue() {
Copy link

Choose a reason for hiding this comment

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

int 는 상금을 반환하기에는 조금 아슬아슬한 자료형아닐까요?

.count();
}

public HashMap<LottoRank, Integer> findEachRankCount() {
Copy link

Choose a reason for hiding this comment

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

제 생각에는 이 메서드가 굳이 없어도 될 것 같아요. 사실상 getRankCount의 반복으로 구성되고 0 출력을 위한 메서드인데, 어차피 출력로직에서 이 반환 Map 에 get을 통해 얻어올 것이라면 그냥 getRankCount 메서드를 사용해도 되지 않을까요? HashMap 자료구조로 반환해줘야하는 이유가 있을까요?


@Test
@DisplayName("입력된 GameMoney 잘 가지고 있는지 확인.")
public void getGameMoneyTest() {
Copy link

Choose a reason for hiding this comment

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

크게 의미있는 테스트로 보이진 않아요.

@Test
@DisplayName("당첨 번호와 같은 로또 번호의 개수를 제대로 반환하는지 테스트.")
void getSameCountByWinnerNumberTest() {
List<Integer> lottoNumber = Arrays.stream(new int[]{1, 2, 3, 4, 5, 6})
Copy link

Choose a reason for hiding this comment

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

List<Integer> lottoNumbers = Arrays.asList(1,2,3,4,5,6);

으로 대체가능합니다


@DisplayName("당첨 결과를 제대로 찾는지 확인.")
@ParameterizedTest
@CsvSource(value = {"1,2,3,13,10,12 | 25 | 0", "1,2,3,4,10,12 | 25 | 1", "1,2,3,4,5,12 | 25 | 2", "1,2,3,4,5,12 | 6 | 3"
Copy link

Choose a reason for hiding this comment

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

CsvSource가 과해지면 가독성이 많이 떨어져요. MethodSource 등을 활용하는 것도 좋을 것 같아요.

Comment on lines 13 to 24
@ParameterizedTest
@DisplayName("정수가 값과 음수 값이 들어올 시 예외처리.")
@CsvSource(value = {"error,-1"})
public void validIntegerValue(String notInteger, String negativeValue) {
assertThatThrownBy(() -> new LottoInfo(notInteger))
.isInstanceOf(NumberFormatException.class)
.hasMessage("정수가 아닌 값이 입력 되었습니다.");

assertThatThrownBy(() -> new LottoInfo(negativeValue))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("구입금액이 음수 값으로 입력되었습니다.");
}
Copy link

Choose a reason for hiding this comment

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

에러메시지를 파라미터로 받고 @ParameterizedTest 를 잘 활용하면 assert 하나로 두개 다 테스트할 수 있을 것 같아요.



private void validDuplicatedNumber(List<Integer> winningNumber) {
Set<Integer> set = new HashSet<>(winningNumber);
Copy link

Choose a reason for hiding this comment

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

로또숫자는 "중복"이 존재하지 않아야한다는 특성을 가지고 있어요. 그래서 여기서도 Set을 선택하셨을거예요. 이렇게 할 것이라면 처음부터 LottoNumber를 List가 아닌 Set으로 Integer를 가지고 있는 것은 어떨까요? 그리고 보너스넘버와 기존 로또 WinningNumber도 중복되지 않아야하는데 그부분에 대한 검증도 빠져있는 것 같아요

.mapToInt(Integer::parseInt)
.boxed()
.collect(Collectors.toList());
List<LottoRank> lottoRanks = lottoNumbers.findLottoRanks(winningNumberList, Integer.parseInt(bonusNumber));
Copy link

Choose a reason for hiding this comment

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

결국 테스트하고싶은건 findLottoRanks로 당첨결과를 제대로 찾아내는지 같은데 .. 이 테스트코드를 보고 파악하기가 쉽지 않네요. 기존구조에 개선이 필요한지, 테스트코드에 불필요한 로직이 많이 들어가있는지 고쳐보는게 좋을것같아요!

Copy link
Author

Choose a reason for hiding this comment

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

구조 대폭 수정중입니다 ㅎㅎ 감사합니다


@Test
@DisplayName("당첨 번호를 잘 반환하는지 확인.")
public void getWinningNumberTest() {
Copy link

Choose a reason for hiding this comment

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

이런 테스트도 사실 의미없는 테스트로보여요

import java.util.Arrays;

public enum LottoRank {
FIFTH_PLACE(5000, 3, "3개 일치 (5000원)"),
Copy link

Choose a reason for hiding this comment

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

여기도 view 에 대한 내용이 도메인으로 들어온거같아요. 출력하고 싶은 내용이 바뀔 때마다 도메인을 건드리는 것이 괜찮을까요?

Copy link
Author

Choose a reason for hiding this comment

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

아아 그러네요! 반영하겠습니다!


private long calculateRevenue() {
long revenue = 0;
for (LottoRank result : results.getResults()) {
Copy link

Choose a reason for hiding this comment

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

LottoResult의 값을 꺼내지 않고 메시지를 전달하는 방식은 어떨까요? getter를 최대한 지양해보는 것도 좋을 것 같아요. 그리고 이 메서드는 calculateProfitRatio 아래에 위치 하는 것이 더 좋을 듯 합니다.

public int calculateProfitRatio(UserMoney userMoney) {
long revenue = calculateRevenue();
int money = userMoney.getMoney();
return (int) ((revenue / (double) money) * 100);
Copy link

Choose a reason for hiding this comment

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

UserMoney로 한정지어서 사용자의 돈만 vo 로 관리를 해주고 있는데 이유가 있나요? Money라는 vo로 빼고 사용해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

초반에 설계한게, 사용자가 돈을 투입하고, 사용자가 투입한 금액을 토대로 통계를 내준다고 생각하고 작성해서 사용자의 돈만 관리하도록 만들었습니다!

Copy link

@csbsjy csbsjy May 28, 2020

Choose a reason for hiding this comment

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

사용자가 투입한 금앨을 토대로 통계를 내준다고 생각하고 설계해줬다면 그 통계도 UserMoney에서 계산을 해야하는게 아닌가 생각해욥. 그리고 이건 저도 피드백받은건데 ㅎㅎ 투입한 금액이 곧 로또티켓을 구매한 금액일까요? 만약 5500원을 입력했다면 이익률을 계산할 때 5500원으로 나누는게 맞을까요 5000원으로 나누는게맞을까요? UserMoney를 투입금액 스토어로 사용하는게 정말 의미가 있는지 잘 모르겠어용!

Copy link
Author

Choose a reason for hiding this comment

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

아하 그러네요..! 이익률 계산에서 완전 틀리네요! 감사합니다! 반영하겠습니다

public class LottoShop {
public final static int NUMBER_BEGIN = 1;
public final static int NUMBER_END = 45;
public final static int MAX_NUMBER_COUNT = 6;
Copy link

Choose a reason for hiding this comment

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

컨벤션을 지켜주세요! `

Suggested change
public final static int MAX_NUMBER_COUNT = 6;
public static final int MAX_NUMBER_COUNT = 6;

int lottoCount = userMoney.getMoney() / TICKET_PRICE;
List<LottoTicket> lottoNumbers = new ArrayList<>();

for (int i = 0; i < lottoCount; i++) {
Copy link

Choose a reason for hiding this comment

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

while 문으로 "금액이 다 떨어질 때까지 로또를 산다"라는 내용이 바로 보일 수 있게 작성할 수도 있을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

네 반영하겠습니다!


import com.javabom.lotto.domain.dto.UserMoneyDto;

public class UserMoney {
Copy link

Choose a reason for hiding this comment

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

지금 이 객체만 봤을 떄는 사실 왜 만든 객체인지 잘 모르겠어요! 생성자와 getter만 존재하고있는데, 이 객체의 존재이유는 무엇인가요?

Copy link
Author

Choose a reason for hiding this comment

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

음 투입한 돈의 금액을 저장하는 역할로만 두고 만들었는데, 금액이 다 떨어질 때까지 로또를 사는 코드를 추가해서, 좀 더 의미 있는 객체로 만들어야겠네요!

@pci2676 pci2676 requested a review from BangKiHyun May 30, 2020 03:25
@pci2676
Copy link
Member

pci2676 commented May 30, 2020

image

나단님 간단하게 도메인 의존성만 다이어그램으로 확인해 봤는데요
선이 겹치는 부분은 디미터 법칙을 위반하는지 의심해보시고 고쳐보시면 좋을것 같고
화살표 방향을 잘보시고 의존성이 양방향으로 발생해서 의존성 사이클이 발생하는지 확인하시고 사이클을 제거하시는 방향으로 수정해 보시면 좋을 것 같아요

Comment on lines 27 to 31
private void validBuyLottery(String money) {
if (Integer.parseInt(money) < LottoShop.TICKET_PRICE) {
throw new IllegalArgumentException("로또를 구입할 금액이 부족합니다.");
}
}

Choose a reason for hiding this comment

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

로또 구입 금액 부족에 대한 유효성 검증을 UserMoney나 LottoShop에서 해주는게 좀더 나을거 같아요!

Comment on lines 9 to 12
public class LottoShop {
public final static int NUMBER_BEGIN = 1;
public final static int NUMBER_END = 45;
public final static int MAX_NUMBER_COUNT = 6;

Choose a reason for hiding this comment

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

로또 번호 생성에 대한 유효성 검증 값들이 LottoShop이 아닌 LottoNumberValidator가 갖고있으면 의존성 싸이클 문제가 해결되지 않을까 생각합니다!

Copy link
Author

Choose a reason for hiding this comment

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

반영해서 의존성 싸이클 고쳤습니다. ㅎㅎ
갑사합니다!

}
}

public void spendMoney(int price) {
Copy link
Contributor

Choose a reason for hiding this comment

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

코드는 위에서 아래로 목차읽히듯이 읽혀야해
그래서 spendMoney를 읽고, 그안에 있는 checkSpendMoney를 읽는게 순서이니까 두개의 위치를 바꿔주면 될 것 같아여

}

private void checkSpendMoney(int price) {
if (this.getRemainMoney() - price < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 그냥 remainMoney < price가 더 가독성이 좋을듯

LottoShop lottoShop = new LottoShop(new LottoNumberShuffler());
LottoUser lottoUser = new LottoUser(new Money(InputView.askInputMoney()), InputView.askManualNumberCount());

LottoTickets manualTickets = lottoUser.buyTicketsByManualInLottoShop(lottoShop, InputView.askManualLottoNumbers(lottoUser.getManualLottoCount()));
Copy link
Contributor

Choose a reason for hiding this comment

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

intellij에 보면 오른쪽에 세로로 그어저있는 라인이 있어

코드의 크기는 가로로 너무 길어서도 안돼고,
세로로 너무 길어서도 안돼

가로로 긴건 이동하면서 읽기 불편하고
세로로 긴건 한 메서드 안의 코드 줄수가 많다 + 한 클래스안의 코드 줄수가 많다를 의미해

List<Set<LottoNumber>> manualLottoNumbers;

public ManualLottoNumbersDto(List<String> manualLottoNumbers) {
this.manualLottoNumbers = manualLottoNumbers.stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

이 스트림 현재 코드에서 3줄로 줄이는 방법이 있음 :) 찾아보세요ㅋㅋㅋ
image
인텔리리 제이 옆에 나오는
저 Stream => List => Stream => Stream
이렇게 바뀌는 흐름이 힌트얌ㅎㅅㅎ

.collect(Collectors.toList()));
}

public LottoTickets joinTickets(LottoTickets lottoTickets) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 생성자잖아. 근데 joinTickets만 보면 LottoTickets라는 객체를 생성한다는걸 몰라
객체를 생성하는건 정적팩터리메서드 패턴이나 생성자를 보면 문법이라서, 관례라서 객체를 생성한다는걸 처음 보는 사람도 알 수 있는데,
이렇게 인스턴스 메서드로 생성자를 만들어버리면, 이 메서드의 내부를 모르고 쓰는사람은 객체가 생성된다는걸 모를꺼야 (뭔가 매핑이 된다거나 그렇게만 생각할 수도있어)

고로 위 코드에서는차라리
LottoTickets의 정적팩터리 메서드를 만들어주는게 좋아.

Comment on lines 20 to 22
LottoTickets manualTickets = lottoUser.buyTicketsByManualInLottoShop(lottoShop, InputView.askManualLottoNumbers(lottoUser.getManualLottoCount()));
LottoTickets automaticTickets = lottoUser.buyTicketsByAutomaticInLottoShop(lottoShop);
LottoTickets allTickets = manualTickets.joinTickets(automaticTickets);
Copy link
Contributor

Choose a reason for hiding this comment

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

여기 세줄 객체의 역할과 책임을 한번 생각해봐야할 것같아.
lottoUser가 LottoTickets를 생성하는게 맞을까? manualTickets가 LottoTickets를 생성하는게 맞을까?

이런의문을 품게된 이유는 아래에 manualTickets의 joinTickets 메서드 설명에 있어.


public class WinningTicket {

private final Set<LottoNumber> winningNumbers;
Copy link
Contributor

Choose a reason for hiding this comment

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

나라면 이부분 LottoTickets로 만들 것 같아.
그 후에 compare작업을 LottoTicket끼리 하도록 즉,

class LottoTicket
public int compareNumber(LottoTicket lottoTicket){
 //...
}

}

public LottoResult getLottoResult(WinningTicket winningTicket) {
return new LottoResult(tickets.stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

마찬가지야 여기서 LottoResult가 생성될 것이라는걸 나는 예측할수가 없었어

import java.util.List;
import java.util.Set;

public class LottoNumbersValidator {
Copy link
Member

Choose a reason for hiding this comment

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

이 Validator 클래스는 쓸데없는 의존성을 가지게한다고 생각합니다.
중복번호 체크나, 로또번호의 개수의 validation은 각각 다른 객체의 책임으로 분리가 가능합니다. 예를들면, 중복번호 체크는 WinnigTiecket에, 로또번호의 개수체크는 LottoTicket에.
각각의 객체가 가져야할 책임을 엉뚱한애가 여러책임을 한꺼번에 가지고있는 클래스라고 생각됩니다.


public class ManualLottoNumbersDto {

List<Set<LottoNumber>> manualLottoNumbers;
Copy link
Member

Choose a reason for hiding this comment

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

Collection 내에 Collection을 가지는건 좋은 모양새는 아니라고 생각됩니다.
List<>내의 객체를 다른 클래스로 래핑해서 다루는게 더 좋은 모양새라고 생각해요!


import com.javabom.lotto.domain.shop.LottoShop;

public class MoneyDto {
Copy link
Member

Choose a reason for hiding this comment

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

저는 DTO는 정말 말 그대로 데이터를 운반하기만 하는 객체라 생각해서
내부에 validation 체크가 있는게 맞는건지 잘 모르겠습니다..
저는 없는게 맞다고 생각해서 ㅠㅠ 웹을 짜본적이없어서 제눈에는 이상해보여요!

import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class LottoNumberShuffler implements LottoNumberGenerator {
Copy link
Member

Choose a reason for hiding this comment

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

해당 클래스의 이름과 실제 수행하는 일과 다르다고 생각합니다.
셔플러하면 뭔가 섞어주기만 할거같은데 내부 내용을 보면 섞인 숫자들을 뱉어내는 작업을 하고있어요. 뭔가 숫자를 뱉을거라는 생각은 들지않았어요.
Shuffler 보다 더 좋은 네임이 있을거라 생각합니다.

@NDjust NDjust changed the title 로또 첫번째 제출 [나단] 로또 게임 Jun 11, 2020
@NDjust
Copy link
Author

NDjust commented Jun 11, 2020

클래스 역할과 책임

클래스 역할과 책임
Money 로또 진행에 필요한 돈을 관리하는 불변 객체.
티켓을 구매, 티켓 구매 가능 여부 판단.
LottoShop 로또 티켓을 발급해주는 객체.
수동 티켓과 자동 로또 티켓 발급.
LottoNumberGenerator 로또 번호를 랜덤으로 발급해주는 객체.
로또 상점에서 발급되는 티켓을 생성해줌.
입력된 숫자를 로또 번호 객체로 변환.
LottoNumber 로또 번호를 관리하는 객체.
로또번호인 1~45 사이 값만 생성할 수 있도록 강제함.
LottoTicket 실제 추첨에 사용될 수 있는 로또 티켓을 관리하는 객체.
로또 게임에 유효한 티켓만 발급될 수 있도록 강제.
로또 티켓을 당첨 번호와 비교하며 당첨 결과를 알려줌.
WinningTicket 당첨 번호(보너스 번호 포함)를 관리하는 객체.
LottoTickets 실제 추첨에 사용될 수 있는 로또 티켓들을 관리하는 객체.
모든 티켓을 당첨 번호와 비교하며 당첨 결과를 알려줌.
LottoRank 당첨 순위에 대한 정보를 담고 있는 열거 객체.
LottoResult 최종 로또 추첨 결과를 가지고 있는 객체.
WinningStatistics 추첨 결과에 따른 통계량을 구해주는 객체.

클래스 다이어그램.

lotto uml

private List<List<LottoNumber>> manualNumbers;

public InputDto(String money, String manualCount) {
validInputValue(money, manualCount);
Copy link

Choose a reason for hiding this comment

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

Integer.parseInt의 호출이 두번씩 발생하고있어요

private void validMoney() {
int ticketPrice = manualCount * LottoShop.TICKET_PRICE.getValue();
int inputMoney = money.getValue();
if (ticketPrice > inputMoney || !money.canSpendMoney(LottoShop.TICKET_PRICE)) {
Copy link

Choose a reason for hiding this comment

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

money.canSpendMoney(LottoShop.TICKET_PRICE)
이 식하나만으로 검증이 부족할까요?

}
}

public void setManualNumbers(List<List<String>> manualNumbers) {
Copy link

Choose a reason for hiding this comment

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

setter를 피해주세요! 객체 생성 이후 Setter가 불려야만하는 객체라면 함께 묶여있어서는 안될 두 상태가 묶여있는것은 아닐까요?

return manualNumbers.stream()
.map(this::parseIntNumbers)
.map(LottoNumbersGenerator::convertToLottoNumbers)
.peek(LottoTicket::validNumbers)
Copy link

Choose a reason for hiding this comment

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

LottoTicket의 validNumbers를 InputDto에서 하는이유는 뭘까요? map연산으로 List으로 반환하지 않은 이유는 무엇인가요?

}

private void validMoney() {
int ticketPrice = manualCount * LottoShop.TICKET_PRICE.getValue();
Copy link

Choose a reason for hiding this comment

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

이왕 만들어둔 Money를 활용해보는 것은 어떨까요? vo 객체는 불변이기 때문에 굉장히 안전한데, 그런 객체를 많이 활용하지 못하는것같아요!

this.sameCount = sameCount;
}

public static LottoRank findBySameCount(int sameCount) {
Copy link

Choose a reason for hiding this comment

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

private!

public static LottoRank findBySameCount(int sameCount) {
return Arrays.stream(LottoRank.values())
.filter(rankInfo -> rankInfo.sameCount == sameCount)
.findAny()
Copy link

Choose a reason for hiding this comment

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

findAny를 사용하게되면 병렬로 수행 시 먼저 찾는 것을 반환한다고 하더라구요. 지금같은 경우는 순차스트림이기 때문에 먼저있는 THIRD_PLACE를 반환하는 것을 "우리는"알지만, 만약 이 이넘이 "순서에 의존한다"는것을 모르는 다른 개발자가 SECOND_PACE와 THIRD_PLACE 상수의 순서를 바꾸면 어떻게될까요? 그 사용자는 3등임에도 2등으로 출력이될거예요(ㄱㅇㄷ) 저한테 코멘트남긴 것에 대한 답변도 여기에서 나올 수 있겠네요!!


for (int i = 0; i < lottoNumbers.size(); i++) {
boolean isSame = lottoNumbers.get(i).equals(winningNumbers.get(i));
sameCount += (isSame ? 1 : 0);
Copy link

Choose a reason for hiding this comment

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

오 굉장히 파악하기 어려워요

import static org.junit.jupiter.api.Assertions.*;

class LottoResultTest {

Copy link

Choose a reason for hiding this comment

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

@DisplayName 생략하지말아주세욥 !
//given
//when
//then
으로 나눠서 작성하는 연습을 하셔도 좋을것같아요! (BDD 검색)

import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.junit.jupiter.api.Assertions.assertEquals;
Copy link

Choose a reason for hiding this comment

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

assertj-core의 메서드를 활용해주세요. 훨ㄲ씬 가독성이좋아요.


@Test
@DisplayName("입력된 숫자값을 로또번호로 변환하는지 확인.")
public void convertLottoNumberTest() {
Copy link

Choose a reason for hiding this comment

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

위의 테스트와 이 테스트가 어울리지 않은 것 같아요. 즉, LottoNumberGenerator 객체는 응집도가 떨어지는 객체이지 않은가 싶어요. 로또넘버를 생성하는 역할과 입력을 로또번호로 Converting해주는 역할을 함께 수행하는것.
그리고 앞의 기능은 LottoShop에서 뒤의 기능은 Dto에서 불리고있는데, Converting 역할이 안어울리지않나 생각해요.
Dto 의 경우 도메인과 View 사이에 존재하는 객체라고 생각하는데 Dto의 경계가 굉장히 모호한느낌이들어요.

Comment on lines +21 to +28
public int calculateProfitRatio() {
long revenue = calculateRevenue();
return (int) ((revenue / (double) calculateCostMoney()) * 100);
}

private long calculateCostMoney() {
return results.size() * LottoShop.TICKET_PRICE.getValue();
}
Copy link
Member

Choose a reason for hiding this comment

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

WinningStatistics니까 생성자에서 아예 수익률을 받는건 어떨가 생각해요. 그러면 LottoShop에 대한 쓸데없는 의존성을 없앨 수 있다고 생각해요. (상수접근을 위해만 의존성이 필요하니)
그리고 get() 메소드는 최대한 지양하는 방향으로 코드를 작성해보는것도 좋아보여요

Copy link
Author

Choose a reason for hiding this comment

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

그렇게 되면, WinningStatistics객체 존재 자체가 의미가 크게 없어서요!

현재는 수익률만 계산하지만, 추후에 로또 추첨 결과로 다양한 통계값을 낼 수도 있는 객체가 필요하다고 봐서 따로 빼놓은 객체라서요! (수익률을 생성자로 받아버리면 Real 깡통객체네용)

get 메소드는 지향하는 방향으로 수정해보겠습니다! 감사함돠

Comment on lines +20 to +24
public List<LottoNumber> generate() {
Collections.shuffle(lottoNum);
return new ArrayList<LottoNumber>(lottoNum.subList(0, LottoTicket.LOTTO_NUMBER_COUNT)) {
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public List<LottoNumber> generate() {
Collections.shuffle(lottoNum);
return new ArrayList<LottoNumber>(lottoNum.subList(0, LottoTicket.LOTTO_NUMBER_COUNT)) {
};
}
public List<LottoNumber> generate() {
Collections.shuffle(lottoNum);
return new ArrayList<>(lottoNum.subList(0, LottoTicket.LOTTO_NUMBER_COUNT));
}

<> 생략이 가능하구 {}는 필요없습니다.

Copy link
Member

Choose a reason for hiding this comment

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

또, 랜덤 관련해서 테스트가 불가능해보입니다!

Comment on lines +48 to +50
private LottoTicket giveLottoTicket(List<LottoNumber> lottoNumbers) {
return new LottoTicket(lottoNumbers);
}
Copy link
Member

Choose a reason for hiding this comment

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

이 메소드는 불필요해보여요. 이 메소드를 쓰는곳에서 new LottoTicket(~~~) 을 부르는게 훨씬 나아보입니다.

Comment on lines +22 to +27

public static List<List<String>> getManualLottoNumbers(int count) {
System.out.println();
System.out.println("수동으로 구매할 번호를 입력해 주세요.");
List<List<String>> lottoNumbers = new ArrayList<>();

Copy link
Member

Choose a reason for hiding this comment

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

List<List<~~>> 컬렉션내 컬렉션을 위치시키는건 무슨 데이터인지 직관성이 떨어진다고 생각합니다.
짠사람은 뭔지 알겠지만, 다른사람이 볼때 2차원 배열이라고 생각할 수 도있다고 생각해서 컬렉션을 한번 더 감싸는게 더 좋다고 생각해요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants