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

미션 제출합니다. #1

Open
wants to merge 45 commits into
base: songpang
Choose a base branch
from
Open

Conversation

songpang
Copy link
Member

@songpang songpang commented Apr 1, 2021

미션 제출합니다!
좋은 하루 되세요.
감사합니다 :)

Jummi10 added 21 commits March 31, 2021 00:35
- 일치하는 숫자의 개수에 따른 당첨금을 가지고 있는 enum
- PrizeMoney -> WinningStatistics 으로 이름 변경
- 일치 개수와 그에 따른 상금을 가지고 있는 클래스 WinningStatistics -> PrizeMoney 로 변경
-
- WinningStatistics 가 당첨 통계만 가지고 있도록 수정
Copy link
Member

@pkalsh pkalsh left a comment

Choose a reason for hiding this comment

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

미션 수행하느라 고생하셨습니다!
최대한 원시 타입과 문자열을 포장하는 요구사항이 있었는데 VO를 사용했을때와 사용하지 않았을때 어떤 차이가 있고, 사용할때 장점을 어떻게 하면 살릴 수 있는지 공부해보면 좋을 것 같습니다. 😄

[참고1] 빈약한 도메인 모델
[참고2] VO란 무엇일까?


import domain.value.validator.PurchasePriceValidator;

public class PurchasePrice {
Copy link
Member

Choose a reason for hiding this comment

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

구매 금액을 VO로 포장한게 인상적이네요.
다만 도메인으로써 할 수 있는 행동이 getter 밖에 없다는 것이 아쉬워요,

코드 전반적으로 Validator로 구현한 검증 기능들을 각 도메인 객체들의 고유한 행동으로 구현하면
응집도가 향상되어 이 클래스만 보더라도 구매 금액에 관한 정보를 더 많이 얻을 수 있을 것 같아요

public int printPurchasedCount(PurchasePrice purchasePrice) {
int purchasedCount = purchasePrice.getPurchasePrice() / PRICE_OF_ONE_LOTTERY_TICKET;
System.out.println(purchasedCount + OutputMessage.PURCHASED_COUNT.getMessage());
return purchasedCount;
}
Copy link
Member

Choose a reason for hiding this comment

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

로또 한 장의 가격은 비즈니스 로직이라 생각합니다.
UI 로직만을 담아야하는 Printer가 구매 금액을 받아 구매한 로또 수를 반환하는건
비즈니스 로직과 UI 로직이 혼재되어 있다고 보여지네요!

Comment on lines +24 to +23
lotteries.getLotteries()
.stream()
.map(lottery -> lottery.getNumbers().getNumbers())
.forEach(System.out::println);
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
Member

Choose a reason for hiding this comment

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

리스트 자체가 toString이 대괄호 안에 element들을 콤마로 구분하여 구현되어 있네요
저는 따로 joining해서 구현했는데 그냥 출력해도 되는거였네요..

Comment on lines 10 to 11
private static final String COMMA = ",";
private static final String BLANK = " ";
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 34
public void validateRangeOfWinningNumbers(List<Integer> splitWinningNumbers) {
if (splitWinningNumbers.stream()
.anyMatch(
winningNumber -> winningNumber < MINIMUM_WINNING_NUMBER || winningNumber > MAXIMUM_WINNING_NUMBER)) {
throw new IllegalArgumentException(ExceptionMessage.MUST_INPUT_NUMBERS_IN_VALID_RANGE.getMessage());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

if문 안에 스트림 구문이 있으니 이걸 인덴트 오버라고 생각해야 되나 의문이 드네요.

또한 if문 안에 스트림 구문으로 인해 가독성이 조금 떨어지는데 메소드 추출을 이용해서 다음과 같이 변경해도 좋을 것 같아요.

Suggested change
public void validateRangeOfWinningNumbers(List<Integer> splitWinningNumbers) {
if (splitWinningNumbers.stream()
.anyMatch(
winningNumber -> winningNumber < MINIMUM_WINNING_NUMBER || winningNumber > MAXIMUM_WINNING_NUMBER)) {
throw new IllegalArgumentException(ExceptionMessage.MUST_INPUT_NUMBERS_IN_VALID_RANGE.getMessage());
}
}
public void validateRangeOfWinningNumbers(List<Integer> splitWinningNumbers) {
if (splitWinningNumbers.stream().noneMatch(this::isInRange)) {
throw new IllegalArgumentException(ExceptionMessage.MUST_INPUT_NUMBERS_IN_VALID_RANGE.getMessage());
}
}
private boolean isInRange(Integer winningNumber) {
return winningNumber >= MINIMUM_WINNING_NUMBER && winningNumber <= MAXIMUM_WINNING_NUMBER
}


import ui.message.ExceptionMessage;

public class WinningNumbersValidator {
Copy link
Member

@pkalsh pkalsh Apr 3, 2021

Choose a reason for hiding this comment

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

꼼꼼한 예외 케이스 검증은 👍 👍 지만

PurchasePrice에서 말씀 드린 대로 이 검증 메소드들도 WinningNumbers 안에 존재한다면 도메인이 더 풍부해지지 않을까요?

Comment on lines 15 to 19
private void initiateNumbers() {
for (int i = 1; i <= 45; i++) {
targetNumbers.add(i);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

WinningNumbersValidator에 존재하는 1, 45라는 로또 번호의 범위 로직이 두 클래스에 나누어 드러나고 있습니다.
LotteryNumber라는 VO를 만들고 이 안에서 범위를 검증하는 식으로 설계하신 후 shuffle과 같은 행동도 몰아버리면 풍부한 도메인이 될 수 있을것 같네요. 😄

Comment on lines 11 to 16
public void compareNumbers(WinningNumbers winningNumbers, BonusNumber bonusNumber, Lotteries lotteries, int purchasedCount) {
for (int i = 0; i < purchasedCount; i++) {
ComparisonResult comparisonResult = compareOneTicketNumbers(winningNumbers, bonusNumber, lotteries.get(i));
winningStatistics.checkRanking(comparisonResult);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

메소드가 너무 많은 인자를 받고 있네요.
사실 purchasedCount 같은 경우는 Lotteries에 포함되어 있는 데이터기 때문에 굳이 보낼 필요가 없다고 생각해요.

코드 전반적으로 도메인 객체 자체가 할 수 있는 일이 많지 않고 대부분 getter 만을 갖고 있는데 도메인 객체가 관리하는 상태를 기준으로 객체가 할 수 있는 행위를 정의하면 좋을 것 같아요.

예를들어, 이 코드에서는 각 로또티켓마다 당첨 번호와 비교하는 행위를 위해서 purchasedCount만큼 도는 for문을 사용했는데 차라리 purchasedCount 크기만큼의 길이를 가진 리스트를 포함한 Lotteries에게 메시지를 보내면 어떨까요?
자율적인 객체를 만들기 위해 고민해봅시다!

[참고]


import java.util.List;

public class WinningStatistics {
Copy link
Member

Choose a reason for hiding this comment

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

열거형 상수인 PrizeMoney를 단지 Map<Integer, Integer> 정도의 인터페이스로 사용하고 있는듯 하여 아쉽네요.
PrizeMoney type의 객체 자체를 활용해서 당첨 횟수를 셀 수 있는 방법을 생각해봅시다!

[참고]

Copy link
Member

@pkalsh pkalsh left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어 죄송합니다...
1단계 코드보다 훨씬 깔끔해진 느낌을 받았네요!
고생하셨습니다 👍 👍


import ui.message.ExceptionMessage;

public class BonusNumber extends LotteryNumberRange {
Copy link
Member

Choose a reason for hiding this comment

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

BonusNumber가 LotteryNumberRange를 상속한 이유는 무엇인가요??

로또 번호와 관련한 도메인을 따로 패키지를 만들만큼 많은 클래스가 나왔는데 하나의 객체에서 관리할 수 있지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

LotteryNumberRange에 제약사항들을 넣어서 관련한 클래스들에 영향을 주기 위해서 였어요. 좋지않은 방법일까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

생각해보니까 LSP를 위반하는 설계였네요,,, 수정해보겠슴니당 ㅠㅠ

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

public class AutoNumbersGenerator {
Copy link
Member

Choose a reason for hiding this comment

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

ManualNumbersGenerator와 AutoNumbersGenerator를 따로 정의한 이유는 무엇인가요?
수동으로 생성한 숫자와 자동으로 생성한 숫자는 다른 특성을 가지고 있나요?

제 개인적인 의견으로는 자동이든 수동이든

  • 중복된 로또 번호를 가질 수 없고,
  • 6개의 숫자가 생성 돼야 하는 특징을 공유하고 있어서

이와 관련한 속성을 한 객체에서 관리하되 생성하는 로직을 외부에서 변경할 수 있도록 제공하는 편이 낫다고 생각합니다.

다른 의견 있으시다면 남겨주세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

음,, 관리하는 객체의 관점에서 사실 자동/수동에서 차이가 있다고 생각했었는데요. 사실 수동/자동의 번호 공통적인 특징에서 볼 때는 동민님 의견의 맞다고 생각합니다.
하지만 생성과정에서 볼 때 사용자 입력을 받아서 검증하는 로직이 필요한 수동번호와
그럴 필요가 없고 1-45번호를 생성하여 셔플해야 하는 자동번호가 같은 객체에서 관리되어야 하나라는 생각이 들었어요.

만약에 한 객체에서 관리하게 된다면 어떤 경우에는 불필요한 로직이 추가되는 것이 아닐까요?


import ui.message.ExceptionMessage;

public class LotteryNumberRange {
Copy link
Member

Choose a reason for hiding this comment

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

public static 상수를 제공함과 동시에 검증하는 비즈니스 로직을 담고 있습니다.
지금은 validateRange를 util성 메서드로 제공함으로써 절차 지향적이라고 생각됩니다.

한 객체 내에서 상수를 private화하고 로또 번호 검증과 생성을 동시에 할 수 있지 않을까요?


import ui.message.ExceptionMessage;

public class ManualNumbersGenerator extends LotteryNumberRange {
Copy link
Member

Choose a reason for hiding this comment

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

public static 상수를 제공함과 동시에 검증하는 비즈니스 로직을 담고 있습니다.
이를 상속으로 해결하여 Generator가 동시에 사용할 수 있는 메서드로 제공하고 있네요.

하지만 위에서 말씀 드린 대로 자동 생성과 수동 생성을 다른 객체로 분리하지 않고 관심사를 모을 수 있지 않을까요?

혹은 Generator라는 추상 부모 클래스가 로또 번호를 생성하는 abstract 메서드를 정의하고
이를 ManualNumberGenerator와 AutoNumberGenerator가 상속한다면 다형성도 활용할 수 있을 것 같네요!

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 +17 to +18
public WinningStatistics compareWithWinningNumbersAndBonusNumber(ManualNumbersGenerator winningNumbers,
BonusNumber bonusNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

ManualNumberGenerator는 이름만 봐서 로또 번호를 생성하여 반환하는 기능을 담당하는 것으로 보여지는데, 이 메서드에서 쓰이기에는 일급 컬렉션으로도 쓰이고 있는 것 같네요.

저는 로또 번호 6개를 포장하는 다른 도메인을 생각해보는게 좋다고 생각합니다.
❓ 어떻게 생각하시나요?


import domain.lotteryStore.numbers.Numbers;

public class Lottery {
Copy link
Member

Choose a reason for hiding this comment

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

Lottery는 getter만 가지고 있는 자료구조 객체로 남기에는 아쉽네요 😢

Copy link
Member

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 👍
리뷰 확인해주세요~


import ui.message.ExceptionMessage;

public class BonusNumber extends LotteryNumberRange {
Copy link
Member

Choose a reason for hiding this comment

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

BonusNumber is a LotteryNumberRange가 성립하나요?

LSP를 위반하고 있는건 아닐까요? 🤔

}

private int getCountOfMatchingWinningNumbers(Lottery lottery, ManualNumbersGenerator winningNumbers) {
return lottery.getNumbers().countMatchingNumbers(winningNumbers);
Copy link
Member

Choose a reason for hiding this comment

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

디미터 법칙을 지켜볼까요?

디미터 법칙을 따르다보면 lottery가 여러 행동들을 가질수 있을 것 같아요.

}

private boolean hasBonusNumber(Lottery lottery, BonusNumber bonusNumber) {
return lottery.getNumbers().contains(bonusNumber.getBonusNumber());
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 18 to 20
private final ManualNumbersGenerator winningNumbers = new ManualNumbersGenerator("1, 2, 3, 4, 5, 6");
private final BonusNumber bonusNumber = new BonusNumber(7, winningNumbers);
private final Lotteries lotteries = new Lotteries(Arrays.asList(
Copy link
Member

Choose a reason for hiding this comment

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

@BeforeEach로 초기화하지 않은 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

정신이 없어서 Before을 안썼나봐요 ... ;(

Comment on lines +18 to +20

@ParameterizedTest
@ValueSource(ints = {-1, 0, 46, 47, 52, 55})
Copy link
Member

Choose a reason for hiding this comment

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

0과 46만으로도 충분한 테스트가 이루어지고 있는것 같아요.
혹은 -1 (음수도 안된다) 정도는 추가해볼 수 있을것 같네요!

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
Member

Choose a reason for hiding this comment

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

@ValueSource(ints = {-1, 0, 46, 47, 52, 55}) -> @ValueSource(ints = {-1, 0, 46})
이 되어도 충분할 것 같다는 뜻이었습니다 🙂

Copy link
Member

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.
아직 이전 리뷰가 반영이 안된 부분이 있는것 같아요 😥 (LSP 관련)
확인 부탁드리겠습니다 🙏

동민님 리뷰 중, Generator를 추상화하여 다형성을 활용해보는 것도 좋은 경험이 될것이라 생각합니다..! 여유가 있으시다면 시도해보는 것도 좋을것 같습니다 🙂

Comment on lines +19 to +35
private static ManualNumbersGenerator winningNumbers;
private static BonusNumber bonusNumber;
private static Lotteries lotteries;

@BeforeAll
public static void setup() {
winningNumbers = new ManualNumbersGenerator("1, 2, 3, 4, 5, 6");
bonusNumber = new BonusNumber(7, winningNumbers);
lotteries = new Lotteries(Arrays.asList(
new Lottery(new Numbers(Arrays.asList(8, 2, 23, 41, 4, 5))),
new Lottery(new Numbers(Arrays.asList(3, 5, 29, 6, 2, 38))),
new Lottery(new Numbers(Arrays.asList(4, 31, 5, 40, 2, 1))),
new Lottery(new Numbers(Arrays.asList(4, 1, 3, 45, 5, 2))),
new Lottery(new Numbers(Arrays.asList(7, 1, 2, 3, 4, 5))),
new Lottery(new Numbers(Arrays.asList(1, 2, 3, 4, 5, 6)))
));
}
Copy link
Member

Choose a reason for hiding this comment

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

테스트의 변수들을 @BeforeAll 로 할당하고 있네요!
@BeforeAll로 초기화하면 어떤 문제가 있을까요? 🤔

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
Member

Choose a reason for hiding this comment

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

제가 잘못 확인한것 같습니다 ㅜㅜ
화이팅입니다!

Comment on lines +12 to +17
private LotteryStore lotteryStore;

@BeforeEach
void setUp() {
lotteryStore = new LotteryStore();
}
Copy link
Member

Choose a reason for hiding this comment

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

LotteryStore는 정적 클래스가 되었으니 위 과정이 필요없을것 같습니다. 🙂

INPUT_LAST_WEEK_WINNING_NUMBERS("\n지난 주 당첨 번호를 입력해 주세요."),
INPUT_BONUS_BALL("보너스 볼을 입력해 주세요.");

private String message;
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 String message;
private final String message;

final이 붙는게 좋아보이네요 🙂
다른 enum Message에도 해당됩니다!

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