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

[로또 main2] 박찬호 제출합니다. #6

Open
wants to merge 18 commits into
base: main2
Choose a base branch
from

Conversation

great-park
Copy link
Member

안녕하세요!
컨트롤러에서 게임의 진행 상황을 코드로써 표현하기 위해서 노력했습니다. 그리고 최대한 역할을 분리시켰는데, 아직은 미숙한 것 같아요. 많은 피드백 부탁 드립니다!

본래의 설계하기를, Input에서 형식적인 검증을 수행하고, 각 객체에서 의미적인 검증을 수행하려고 했습니다. 현재 코드에서는 검증의 책임이 체계없이 파편화 되어 있어서 오히려 안 좋은 설계가 된 것 같네요. 차라리 모든 검증을 각 객체에게 맡겼으면 더 좋았을 것 같습니다. validation은 누구에게 맡겨야 할 지 각자 의견 달아주시면 감사하겠습니다!

Copy link

@ray-yhc ray-yhc left a comment

Choose a reason for hiding this comment

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

  • 컨트롤러, 뷰, 서비스로직으로 나누어 구현하신 것이 좋은 구조인 것 같네요! 참고해갑니다!
  • application에서 LottoMachine과 LottoFactory를 주입하셨네요! 의존성 주입의 장점을 살려서 LottoMachine과 LottoFactory를 인터페이스로 구현하면 더 좋은 구조가 될 것 같아요!
  • 만약 로또 상금 규칙이 변경된다면, Ranking과 Output을 모두 변경해야 해서 번거로울 것 같아요!

수고하셨습니다~~

Comment on lines +31 to +34
printInsertManualLottoNumbersRequest();
for (int i = 0; i < manualLottoCount; i++) {
List<Integer> numbers = insertLottoNumbers();
Lotto manualLotto = lottoFactory.createManualLotto(numbers);
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
Member Author

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.

다른 객체들의 역할들을 LottoMachine이 대신하고 있는 것 같아요

Lotto 생성은 Lotto 혹은 Lottos가,
WinningNumbers 생성은 WinningNumber가
결과 계산은 LottoResult가 직접 하도록 하면 어떨까요?

현실 속의 객체와 소프트웨어 객체 사이의 가장 큰 차이점은 무엇일까? 그것은 현실 속에서는 수동적인 존재가 소프트웨어 객체로 구현될 때는 능동적으로 변한다는 것이다.
...
스스로 판매 금액을 계산해서 종이에 기입하는 현실 속의 상품을 상상해 보라. 여러분이 객체지향 세계를 구축할 때 현실에서 가져온 객체들은 현실 속에서는 할 수 없는 어떤 일이라도 할 수 있는 전지전능한 존재가 된다.
<객체지향의 사실과 오해 p.55>

Copy link
Member Author

Choose a reason for hiding this comment

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

아 그렇네요
객체가 능동적으로 자신과 관련된 동작을 책임지도록 하는게 더 낫겠네요 감사합니다!

Comment on lines +56 to +57
totalWinningNumbers.addAll(normalWinningNumbers);
totalWinningNumbers.add(bonusWinningNumber);
Copy link

Choose a reason for hiding this comment

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

bonusWinningNumber도 totalWinningNumbers에 포함되어 다음과 같이 에러가 나네요!

image

Copy link
Member Author

Choose a reason for hiding this comment

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

NumberType을 만들어서 WinningNumber 필드로 넣긴 했는데, 활용을 잘 못했네요
수정해보겠습니다!

Comment on lines +9 to +11
LottoFactory lottoFactory = new LottoFactory();
LottoMachine lottoMachine = new LottoMachine(lottoFactory);
LottoController lottoController = new LottoController(lottoMachine);
Copy link

Choose a reason for hiding this comment

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

오 의존성 주입을 구현하신 건가요??!?

Comment on lines +36 to +41
private static Ranking getSecondOrThird(boolean isBonusContain) {
if (isBonusContain){
return Ranking.SECOND;
}
return Ranking.THIRD;
}
Copy link

Choose a reason for hiding this comment

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

만약 2,3등의 수상 조건이 변경된다면??

}

public LottoResult computeLottoResult(Lottos lottos, List<WinningNumber> winningNumbers) {
Map<Ranking, Integer> winningInfo = new HashMap<>();
Copy link

Choose a reason for hiding this comment

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

key 가 Enum이라면 EnumMap도 추천합니다!

Copy link
Member

@seong-wooo seong-wooo left a comment

Choose a reason for hiding this comment

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

테스트하기 좋은 구조로 변경하려면 어떻게 해야할까요?
일단 이 부분만 중점으로 해서 리뷰 남기겠습니다!

Comment on lines +16 to +19
this.numbers = numbers.stream()
.sorted()
.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.

numbers 내부에 존재하는 값들이 모두 1 ~ 45라는 룰을 만족하나요?

Comment on lines +69 to +86
private static int validateNumber(String input) {
return checkRange(checkNumber(input));
}

private static int checkNumber(String input) {
if (!input.chars().allMatch(Character::isDigit)) {
throw new IllegalArgumentException(INPUT_IS_NOT_NUMBER);
}

return parseInt(input);
}

private static int checkRange(int input) {
if (input < LOTTO_MIN_NUMBER || input > LOTTO_MAX_NUMBER) {
throw new IllegalArgumentException(LOTTO_NUMBER_OUT_OF_RANGE);
}
return input;
}
Copy link
Member

Choose a reason for hiding this comment

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

상당히 복잡한데요?

checkNumber나 checkRange는 검증의 역할만 하는 것이 좋을 것 같아요.

Comment on lines +21 to +26
public Lotto() {
this.numbers = Randoms.pickUniqueNumbersInRange(MINIMUM_LOTTO_NUMBER, MAXIMUM_LOTTO_NUMBER, LOTTO_TOTAL_COUNT)
.stream()
.sorted()
.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.

Lotto lotto = new Lotto()를 실행했다고 생각해볼게요.
lotto 내부에 어떤 값이 들어있는지 테스트할 수 있나요?
Random으로 값을 생성하는 로직을 직접적으로 의존하고 있어서 테스트할 수 없습니다.

며칠 전에 이동욱 개발자님이 하신 말씀이 생각나네요.
제어할 수 없는 영역에는 의존하지말라!

Comment on lines +12 to +14
List<Lotto> lottos = new ArrayList<>();
lottos.addAll(manualLottos);
lottos.addAll(autoLottos);

Choose a reason for hiding this comment

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

List<Lotto> lottos = Stream.concat(manualLottos.stream(), autoLottos.stream())
                .collect(Collectors.toList());

이런 방식으로도 두 개의 리스트를 합칠 수 있습니다!

Comment on lines +8 to +9
private Integer manualLottoCount;
private Integer autoLottoCount;

Choose a reason for hiding this comment

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

자료형으로 int가 아닌 Integer를 사용하신 이유가 있으신가요?

Comment on lines +15 to +16
private static final int LOTTO_MIN_NUMBER = 1;
private static final int LOTTO_MAX_NUMBER = 45;

Choose a reason for hiding this comment

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

스크린샷 2023-04-11 오후 3 26 12

야구 게임 미션 당시 성우님이 달아주셨던 코멘트인데, 찬호님 코드에도 해당되는 내용 같아서 첨부해드립니다.
로또 숫자의 최솟값과 최댓값에 대한 캡슐화가 필요해보입니다!

Comment on lines +73 to +79
private static int checkNumber(String input) {
if (!input.chars().allMatch(Character::isDigit)) {
throw new IllegalArgumentException(INPUT_IS_NOT_NUMBER);
}

return parseInt(input);
}

Choose a reason for hiding this comment

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

검증 로직에서 검증 외에 파싱까지 수행하는게 어색한 것 같습니다!

Comment on lines +19 to +28
try {
LottoMoney money = new LottoMoney(insertLottoMoney());
Lottos lottos = lottoMachine.createLottos(money, insertManualLottoCount());
printLottos(lottos);
List<WinningNumber> winningNumbers = lottoMachine.createWinningNumbers(insertNormalWinningNumbers(), insertBonusWinningNumber());
LottoResult lottoResult = lottoMachine.computeLottoResult(lottos, winningNumbers);
printLottoResult(lottoResult, money);
} catch (IllegalArgumentException error) {
printErrorMessage(error.getMessage());
}

Choose a reason for hiding this comment

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

사용자가 잘못된 값을 입력할 경우 IllegalArgumentException를 발생시키고, "[ERROR]"로 시작하는 에러 메시지를 출력 후 종료한다.

try-catch 때문에 해당 요구사항을 만족하지 못하는 것 같아요! try-catch는 삭제해도 될 것 같습니다!

Comment on lines +29 to +39
private List<Lotto> createManualLottos(int manualLottoCount) {
List<Lotto> manualLottos = new ArrayList<>();
printInsertManualLottoNumbersRequest();
for (int i = 0; i < manualLottoCount; i++) {
List<Integer> numbers = insertLottoNumbers();
Lotto manualLotto = lottoFactory.createManualLotto(numbers);
manualLottos.add(manualLotto);
}

return manualLottos;
}

Choose a reason for hiding this comment

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

private List<Lotto> createManualLottos(int manualLottoCount) {
    printInsertManualLottoNumbersRequest();
    return Stream.generate(lottoFactory::createAutoLotto)
            .limit(manualLottoCount)
            .collect(Collectors.toList())
}

이렇게 Stream을 사용하면 가독성이 훨씬 좋아질 것 같아요!


private final Long prizeMoney;
private final int equalCount;
private final boolean containBonus;

Choose a reason for hiding this comment

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

해당 변수는 선언 후 사용되고 있지 않네요!

Comment on lines +4 to +6
private final String MONEY_MUST_OVER_1000 = "구매 금액은 1000원 이상을 입력해주세요.";
private final String MONEY_UNIT_IS_1000 = "구매 금액은 1000원 단위로 입력해주세요.";
private final int LOTTO_PRICE_UNIT = 1000;

Choose a reason for hiding this comment

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

Suggested change
private final String MONEY_MUST_OVER_1000 = "구매 금액은 1000원 이상을 입력해주세요.";
private final String MONEY_UNIT_IS_1000 = "구매 금액은 1000원 단위로 입력해주세요.";
private final int LOTTO_PRICE_UNIT = 1000;
private static final String MONEY_MUST_OVER_1000 = "구매 금액은 1000원 이상을 입력해주세요.";
private static final String MONEY_UNIT_IS_1000 = "구매 금액은 1000원 단위로 입력해주세요.";
private static final int LOTTO_PRICE_UNIT = 1000;

상수 선언은 보통 static final을 함께 사용합니다!

Comment on lines +62 to +66
String FIFTH_STATS = String.format("3개 일치 (%s원) - %s개", numberFormat.format(FIFTH.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(FIFTH, 0));
String FORTH_STATS = String.format("4개 일치 (%s원) - %s개", numberFormat.format(FORTH.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(FORTH, 0));
String THIRD_STATS = String.format("5개 일치 (%s원) - %s개", numberFormat.format(THIRD.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(THIRD, 0));
String SECOND_STATS = String.format("5개 일치, 보너스 볼 일치 (%s원) - %s개", numberFormat.format(SECOND.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(SECOND, 0));
String FIRST_STATS = String.format("6개 일치 (%s원) - %s개", numberFormat.format(FIRST.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(FIRST, 0));

Choose a reason for hiding this comment

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

로또 결과를 출력 양식으로 반환하는 로직은 LottoResult 안에 있으면 더 좋을 것 같아요!

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.

4 participants