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

[로또] 이준섭 미션 제출합니다. #2112

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

junseoplee
Copy link

No description provided.

Copy link

@poi1649 poi1649 left a comment

Choose a reason for hiding this comment

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

꽤 요구사항이 많은 미션이라 오래 걸릴 수 있었는데, 시간맞춰 제출하시느라 고생 많았습니다.
저번과 비교해서 확실히 코드가 깔끔해졌다는 느낌을 많이 받았네요 👍 enum과 일급 컬렉션까지 만들어볼 줄은 몰랐는데..ㅋㅋㅋ 매우 훌륭합니다.

다만, 요구사항에 있는
기능을 구현하기 전 docs/README.md에 구현할 기능 목록을 정리해 추가한다. 라는 문장이 만족되지 않은 것 같아, 이번에는 리뷰 반영 + 기능 목록 정리를 한 번 해주시고, 다시 요청 주시면 좋을 것 같습니다! 이번도 화이팅입니다~


System.out.println("총 수익률은 " + profitRate + "%입니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

파일의 마지막줄엔 개행이 필요하고, 만약 개행이 없으면 깃허브, IDE 등에서 경고가 발생합니다. 왜 그럴까요!?
스크린샷 2024-01-13 오후 12 26 08

Copy link
Author

Choose a reason for hiding this comment

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

IEEE에서 정의한 POSIX 표준의 일부로, 각 라인은 개행 문자로 끝나야 합니다.
gcc 컴파일러는 소스코드를 한 라인씩 읽으며 파일이 끝났더라도 개행문자가 없으면 한 줄이 끝나지 않은 것으로 인식해서 정상적으로 동작하지 않을 수 있습니다.
따라서 잠재적인 오류를 방지하고자 경고를 발생시킵니다.

Comment on lines 138 to 141
DecimalFormat df = new DecimalFormat("#.##");
profitRate = Double.parseDouble(df.format(profitRate)); // 소수 둘째 자리에서 반올림

System.out.println("총 수익률은 " + profitRate + "%입니다.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
DecimalFormat df = new DecimalFormat("#.##");
profitRate = Double.parseDouble(df.format(profitRate)); // 소수 둘째 자리에서 반올림
System.out.println("총 수익률은 " + profitRate + "%입니다.");
System.out.println("총 수익률은 " + String.format("%.1f", profitRate) + "%입니다.");

처럼 써줄 수도 있어요! DecimalFormat과 어떤 차이가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

소수를 반올림하는 방법은 제가 알기로 4가지 정도가 있습니다.
첫 번째는 제가 사용한 DecimalFormat 클래스를 사용한 방법이고(#은 0을 생략합니다) 반환값은 String입니다.
두 번째는 BigDecimal 클래스 setScale() 메서드를 사용합니다. (첫 번째 매개변수는 자릿수, 두 번째는 올림, 내림 설정)
세 번째는 suggest 해주신 String 클래스의 format() 메서드입니다. (%.자리f)
네 번째는 Math 클래스의 round() 메서드

DecimalFormat 클래스와 String.format() 의 차이
DecimalFormat 클래스의 경우 인스턴스 생성이 강제되지만 재사용이 가능합니다.
String.format() 은 정적인 사용이 가능합니다.

DecimalFormat은 더 단순하고 직접적인 포맷팅 방식을 사용해서 더 빠릅니다.

import lotto.numbers.Lotto;
import lotto.numbers.UserInputNumbers;

public class LottoManager {
Copy link

Choose a reason for hiding this comment

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

객체지향의 원칙 중 하나의 클래스는 하나의 책임만 가져야 한다. 는 원칙이 있습니다.
현재 이 클래스는 입력을 받는 로직, 출력을 담당하는 로직, 로또 번호를 만드는 로직 등등.. 클래스가 너무 많은 책임을 갖고 있는 것 같아요!
클래스를 분리해보면 좋겠네요.
추가로 객체 지향의 원칙에 대해서도 정리해보면 좋을 것 같아요

Copy link
Author

@junseoplee junseoplee Jan 16, 2024

Choose a reason for hiding this comment

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

패키지를 크게 세 곳으로 나눴습니다 도메인 모델 패키지, 비즈니스 로직을 담당하는 서비스 패키지, 인풋 아웃풋 메서드가 있는 유틸 패키지.
각 패키지 안에서 여러 클래스로 나눠 각 클래스는 한 가지의 책임을 가지도록 했습니다.
이렇게 하면 각 관련 로직이 한 곳에 모여 있어 가독성과 유지 관리성이 향상됩니다.
각 객체는 다른 객체와 상호작용하며 기능을 완성하고, 각 클래스는 자신의 데이터를 직접 관리합니다.
Lotto클래스 내부의 getMatchCount와 isBonusMatch 메서드에 대해 고민을 했는데 이 메서드들은 Lotto 객체 내부에 직접적인 접근을 필요로 하기 때문에 캡슐화를 지키기 위해 Lotto클래스 내부에 존재하도록 했습니다.
각 클래스는 각자의 책임과 기능을 가지고 있기 때문에 재사용성이 증가합니다.

객체지향 프로그래밍의 핵심 원칙은 SOLID로 구분할 수 있습니다.

  1. 단일 책임 원칙 (Single Responsibility Principle): 하나의 클래스는 하나의 책임만을 가져야 합니다.
  2. 개방 폐쇄 원칙 (Open-Closed Principle): 기존의 코드를 변경하지 않으면서 기능을 추가할 수 있어야 합니다. 확장에는 열려있어야하지만, 변경에는 닫혀 있어야 한다는 의미입니다.
  3. 리스코프 치환 원칙 (Liskov Substitution Principle): 자식 클래스는 언제나 자신의 부모 클래스를 대체할 수 있어야 합니다. 하위 클래스는 상위 클래스의 특성과 행동을 재정의 하지 않고 확장만 해야합니다. 다형성을 지원하기 위한 원칙입니다.
  4. 인터페이스 분리 원칙 (Interface Segregation Principle): 너무 많은 기능을 가진 인터페이스보다는 필요한 기능들로 분리된 여러 개의 인터페이스가 낫습니다. 자신이 사용하지 않는 인터페이스는 구현하지 말아야합니다.
  5. 의존성 역전 원칙 (Dependency Inversion Principle): 상세한 것에 의존하기보다는, 추상적인 것에 의존해야 한다는 원칙입니다. 어떤 클래스를 참조해서 사용해야하는 상황이 생긴다면 클래스를 직접 참조하는 것이 아니라 추상성이 높은 클래스로 참조해야합니다.

Comment on lines 18 to 21
validateLottoNumbers(receivedLottoNumbers);
this.receivedLottoNumbers = receivedLottoNumbers;
validateBonusNumbers(bonusNumber);
this.bonusNumber = bonusNumber;
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 54 to 57
for (int number : bonusNumber) {
if (number < MIN_LOTTO_NUMBER || number > MAX_LOTTO_NUMBER) {
throw new IllegalArgumentException("[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.");
}
Copy link

Choose a reason for hiding this comment

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

validateLottoNumber 메서드에도 같은 로직이 존재하네요! 하나의 메서드로 분리해볼 수 있을 것 같아요

Copy link
Author

@junseoplee junseoplee Jan 16, 2024

Choose a reason for hiding this comment

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

최대한 같은 로직을 공유하고 싶어서 bonusNumber를 List로 만들었습니다. 하지만 이럴 경우에 보너스 번호 일치 작동이 이뤄지지 않아 bonusNumber의 타입을 int로 바꾸고 로직들도 변경했습니다. 더 이상 같은 로직을 공유하지 않습니다.

추가적으로 validateLottoNumbers 메서드는 Lotto 클래스와 UserInputNumbers 클래스에서 중복되어 사용되고 있습니다.
이를 LottoValidator와 같은 별도의 유효성 검사 클래스로 분리하는 것도 하나의 방법일 수 있습니다.
하지만 이 두 클래스는 서로 다른 책임을 갖고있고 추후 한 쪽에서 유효성 검사 방법이 변경될 수도 있기 때문에 항상 같은 방식으로 유효성 검사를 하지 않을 수도 있습니다.
이 경우에는 중복을 그대로 두는 것이 객체 지향 원칙을 좀 더 잘 지킬 수 있다 생각됩니다.

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 16 to 20
try {
Integer.parseInt(inputAmount);
} catch (NumberFormatException e) {
throw new IllegalArgumentException("[ERROR] 구입금액은 숫자여야합니다.");
}
Copy link

Choose a reason for hiding this comment

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

꼼꼼한 엣지케이스 처리 💯💯

}

private static void validatePurchaseAmount(int inputAmount) {
if (inputAmount % LOTTO_PRICE != 0) {
Copy link

Choose a reason for hiding this comment

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

상수 분리 👍👍👍

@@ -0,0 +1,32 @@
package lotto.util;

public class Money {
Copy link

Choose a reason for hiding this comment

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

원시값을 클래스로 포장했군요👍 어떤 이점이 있을까요!?

Copy link
Author

@junseoplee junseoplee Jan 17, 2024

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.

오.. 불변 객체는 뭐고 어떤 장단점을 가지나요!?

Copy link
Author

Choose a reason for hiding this comment

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

불변 객체란 객체 생성 이후 내부의 상태가 변하지 않는 객체입니다. 즉 read-only 메서드만을 제공합니다.

  1. 불변 객체 및 final을 사용하면 항상 동일한 값을 반환하여 안정성을 보장하고 동시에 사용되더라도 동기화를 하지 않음으로 성능상의 이점도 있습니다.
  2. 가변 객체를 통해 작업을 하는 도중에 에러가 발생하면 또 다른 에러를 유발할 수 있습니다. 하지만 불변 객체라면 예외가 발생하더라도 상태를 유지하고 다음 로직을 처리할 수 있게됩니다.
  3. Map, Set 등의 요소로 활용하기에 적합합니다. 만약 가변 객체가 요소이고 변경된다면 갱신하는 등 부가 작업이 필요합니다. 하지만 불변 객체하면 한 번 이를 고려하지 않아도 됩니다.
  4. Side Effect를 피해 오류가능성을 최소화할 수 있습니다. Side Effect는 자신이 아닌 외부의 상태에 영향을 만드는 것을 뜻합니다. 의도하지 않은 결과가 있을 수 있습니다.
    만약 객체의 수정자를 통해 여러 객체들에서 값을 변경한다면 객체의 상태를 예측하기 어려워질 것입니다. 바뀐 상태를 파악하기 위해서는 메서드들을 보여야하고, 이는 유지보수성을 떨어뜨립니다. 그래서 Side Effect가 없는 순수 함수들을 만드는 것이 중요합니다.
    하지만 불변 객체는 기본적으로 값의 수정이 불가능하기 때문에 예측을 편하게하며 오류를 줄여 유지보수성이 높은 코드를 만들 수 있게 해줍니다.
  5. 다른 사람이 작성한 코드를 예측가능하며 안전하게 사용할 수 있습니다.

Copy link

Choose a reason for hiding this comment

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

아주 좋습니다. 적어주신 것 뿐 아니라 객체 뿐아니라 원시값에 final을 붙여 재할당을 막는 경우도 비슷한 이유에요! 한 번 불변을 만드는 습관을 들여놓으시면 대규모 시스템을 만지게 될때 본의아닌 이점을 많이 얻으실겁니다

Comment on lines 35 to 46
Money money;
while (true) {
try {
System.out.println("구입금액을 입력해 주세요.");
String input = Console.readLine();
money = new Money(input);
break;
} catch (IllegalArgumentException e) {
System.out.println(e.getMessage());
}
}
return money;
Copy link

Choose a reason for hiding this comment

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

Suggested change
Money money;
while (true) {
try {
System.out.println("구입금액을 입력해 주세요.");
String input = Console.readLine();
money = new Money(input);
break;
} catch (IllegalArgumentException e) {
System.out.println(e.getMessage());
}
}
return money;
while (true) {
try {
System.out.println("구입금액을 입력해 주세요.");
String input = Console.readLine();
return new Money(input);
} catch (IllegalArgumentException e) {
System.out.println(e.getMessage());
}
}

이렇게 try 문 안에서 직접 반환해버리면 사용하지 않는 변수를 선언할 필요가 없어집니다!

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 4 to 9
LOSE("0", 0),
FIFTH_PRIZE("5,000", 3),
FOURTH_PRIZE("50,000", 4),
THIRD_PRIZE("1,500,000", 5, false),
SECOND_PRIZE("30,000,000", 5, true),
FIRST_PRIZE("2,000,000,000", 6);
Copy link

Choose a reason for hiding this comment

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

enum 사용 좋네요! 그런데 가격을 숫자 자료형이 아닌 문자열로 해주신 이유가 뭘까요!?

Copy link
Author

Choose a reason for hiding this comment

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

실행결과 예시를 보면 1,500,000원 단위 처럼 쉼표를 표시하기 위해 String을 사용했습니다.
DecimalFormat을 사용하여 정수형으로도 같은 구현이 가능하지 않을까 생각합니다.

@junseoplee
Copy link
Author

junseoplee commented Jan 17, 2024

과정 정리

Copy link

@poi1649 poi1649 left a comment

Choose a reason for hiding this comment

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

다시 봐도 처음 미션이랑 비교해서 엄청 생각을 많이하고 짰다는 게 느껴지네요 굿굿.. 리뷰 반영해주신 이후 클래스 책임이 정말 명확하게 분리됐다는 게 느껴집니다. 특히 Input, Output 이 domain과 명확하게 분리되어 있어서 여러 장점을 가진 코드가 된 것 같아요.
저는 이런 분리의 최대 장점은 테스트 코드 작성 난이도의 하락이라고 생각합니다. 그래서 먼저 클래스 분리를 해보도록 유도했어요.
이제는 이번 미션의 핵심이라고도 할 수 있는 도메인 테스트코드 작성을 진행해볼 때가 온 것 같네요.ㅋㅋㅋㅋ 이번 주는 리뷰 확인하고 반영 + 테스트 코드 작성까지 부탁드리고 다음주 수요일 저녁 6시까지 다시 리뷰 요청 주세요! 또 달려봅시다

@@ -0,0 +1,42 @@
package lotto.sevice;
Copy link

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.

헉><


public enum WinningCheck {
LOSE(0, 0),
FIFTH_PRIZE(5000, 3),
Copy link

Choose a reason for hiding this comment

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

소소한 팁: 자바에서 숫자가 너무 길어서 보기 힘들다면 5_000 처럼 쓸 수 있습니다.

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 16 to 28
public Map<WinningCheck, Integer> calculateWinningResult(List<Lotto> lottos,
UserInputNumbers receivedLotto) {
Map<WinningCheck, Integer> result = new HashMap<>(); // WinningCheck 객체 정수를 키와 값으로 Map에 저장
for (Lotto lotto : lottos) {
int matchCount = lotto.getMatchCount(receivedLotto);
boolean bonusMatch = lotto.isBonusMatch(receivedLotto); // receivedLotto는 bonus를 포함하고있음

WinningCheck prize = WinningCheck.getPrize(matchCount, bonusMatch);
result.merge(prize, 1, Integer::sum);
}
return result;
}

Copy link

Choose a reason for hiding this comment

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

이 책임을 Lotto 클래스나 WinningCheck enum 클래스에 넘겨주지 않고 별도의 Calculator 클래스를 만들게 된 이유가 궁금하네요 😎

Copy link
Author

Choose a reason for hiding this comment

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

단일 책임 원칙을 생각하며 별도의 클래스를 만들었습니다.
Lotto 클래스는 로또 객체를 유지하고 해당 로또가 사용자의 입력과 얼마나 일치하는지 알아내는 책임이 있고
WinningCheck enum 클래스는 일치하는 수와 보너스 일치 여부에 따라 어떤 상금이 주어지는지 결정하는 책임이 있습니다.
여기에 추가로 로또들의 목록을 순회하며 각각에 대한 결과를 계산하고 집계하는 이 메서드는 다른 책임입니다.
이 원칙에 따른다면 자신의 주요 책임에만 집중하게됩니다. 유지보수가 쉬워지며, 안정성이 높아집니다.

Copy link

Choose a reason for hiding this comment

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

아주 좋습니다! 추가적으로 세 클래스의 의존의 방향에 대해서도 이 기회에 한 번 가볍게 고민해보셔도 좋아보여요!!

import lotto.domain.UserInputNumbers;
import lotto.domain.WinningCheck;

public class Calculator {
Copy link

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.

책임을 분리한다면 로또 결과 계산, 수익률의 계산 이렇게 두 책임으로 나눌 수 있습니다.
저라면 새로운 클래스를 만들고 싶습니다.
LottoResultCalculator와 LottoProfitRateCalculator로 클래스를 분리했고, 책임을 부여했습니다.
이제 각 클래스는 더 작게 나눠진 책임을 갖습니다. 향후 로직이 변경되거나 확장되는 경우, 유지보수에 이점이 있을 것입니다.

Copy link

Choose a reason for hiding this comment

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

x떡같이 말해도 찰떡같이 알아들으시는군요 👍
추가로 저는 Lotto클래스에 결과 계산의 책임을 주는 것도 괜찮아보여요! 만약 그렇다면 어떤식으로 파라미터를 받을 수 있을지 상상해보실래요!?

Comment on lines 31 to 37
double totalPrize = 0;
for (Map.Entry<WinningCheck, Integer> entry : result.entrySet()) {
WinningCheck prize = entry.getKey();
int count = entry.getValue();
totalPrize += prize.getPrizeAmount() * count;
}

Copy link

Choose a reason for hiding this comment

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

Suggested change
double totalPrize = 0;
for (Map.Entry<WinningCheck, Integer> entry : result.entrySet()) {
WinningCheck prize = entry.getKey();
int count = entry.getValue();
totalPrize += prize.getPrizeAmount() * count;
}
double totalPrize = (double) result.entrySet().stream()
.mapToInt(entry -> entry.getKey().getPrizeAmount() * entry.getValue())
.sum();

사실 어느게 깔끔한진 모르겠지만 이렇게 스트림으로 지역변수 선언과 할당을 동시에 할 수도 있습니다. ㅋㅋㅋ

Copy link
Author

Choose a reason for hiding this comment

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

저는 스트림이 더 좋은 코드라 생각합니다. 명확하네요!

.map(Integer::parseInt)
.toList();
System.out.println("\n보너스 번호를 입력해 주세요.");
int bonusNumber = Integer.parseInt(Console.readLine().trim());
Copy link

Choose a reason for hiding this comment

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

링크 읽어보시고 한 줄에 왜 하나의 점만 찍어야하는지 알아보시면 좋겠네요

Copy link
Author

@junseoplee junseoplee Jan 21, 2024

Choose a reason for hiding this comment

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

감사합니다!
대상 객체의 내부에 깊이 접근하겠다는 의도를 드러내는 것이 중요하네요!

다만, Integer.parseInt(Console.readLine().trim()) 이 라인과 같이 간단한 메서드를 호출하고 의도가 투명하게 보일 경우에는 그대로도 충분히 가독성이 좋다고 생각합니다.

물론 추가적인 메서드를 생성하여 그곳에서 처리한 후 적용하는 방법도 있습니다.
하지만 때로는 비효율적이고, 별도의 메서드로 나눠져있기 때문에 가독성이 나빠질 수 있다 생각합니다!

원칙에 대해 이해하고 상황에 따라 유연하게 적용할 수 있도록 노력해보겠습니다!

Comment on lines 22 to 23


Copy link

Choose a reason for hiding this comment

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

공백이 두줄이에요!

docs/README.md Outdated
Comment on lines 4 to 16

- 도메인 모델 패키지 (domain)
- 사용자로부터 입력받은 구입금액을 저장하는 클래스 PurchaseAmount
- 난수 생성된 로또 번호가 있는 클래스 Lotto
- 사용자로부터 로또 번호 6개와 보너스 번호 1개를 입력받아 저장하는 클래스 UserInputNumbers
- 상금과 당첨 수를 저장하는 열거형 enum WinningCheck
- 비즈니스 로직을 수행하는 서비스 패키지 (service)
- 당첨 결과와 수익률을 계산하는 클래스 Calculator
- 구입금액만큼 로또를 발행하는 클래스 LottoGenerator
- 각종 메서드가 있는 패키지 (util)
- 사용자로부터 값들을 입력받는 클래스 InputManager
- 결과들을 출력하는 클래스 OutputManager

Copy link

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.

우선 처음 목표는 입출력, 비즈니스 로직으로 나눠보고 싶었습니다.
각 파트에는 많은 메서드와 책임이 있습니다.
이때 책임을 분리하기위해 클래스를 나눴고, 관련 있는 클래스와 인터페이스를 묶는 것이 관련 기능을 찾거나 구현하기 쉬워지겠다 판단하여 패키지로 나눴습니다.
덕분에 가독성, 유지 보수성, 추후 공동 작업이나 재사용성의 이점이 생겼습니다.

Copy link

Choose a reason for hiding this comment

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

오.. .우선 매우 타당한 이유와 좋은 고민의 결과라는 생각이 듭니다.
그런데 저는 프로그래밍을 막 제대로 하기 시작했을 때, 가독성, 유지보수성 이라는 개념이 너무 막연했었거든요.
그래서 한 가지 감을 잡기 위한 힌트아닌 힌트?를 드리기 위해 한 가지 과제를 드려볼게요. (이거는 풀리퀘말고 개인톡으로 코드 캡쳐해서 보내주세요)
구매 수량 클래스의 생성자에 입력이 결합된 형태의 클래스를 만들어보았는데, 이 클래스가 똑바로 검증을 하는 지 어떻게 테스트 코드를 쓸 수 있을지 고민해보고 답 부탁드려요!

  public PurchaseAmount() {
    final var input = Console.readLine();
    validateIsNumeric(input);
    int inputAmount = Integer.parseInt(input);
    validatePurchaseAmount(amount);
    this.amount = inputAmount;
  }

Comment on lines 16 to 21
try {
Integer.parseInt(inputAmount);
} catch (NumberFormatException e) {
throw new IllegalArgumentException("[ERROR] 구입금액은 숫자여야합니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

domain 클래스가 String 이라는 input에 대해 관여하고 있는게 조금 걸립니다. 만약 입력이 콘솔이 아니라 웹 요청으로 오게 되어 항상 int 형이 오게 된다면 이 메서드는 사용하지 않는 메서드가 될 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

로컬 변수 amount를 불변으로 선언하고 싶었는데 해당 메서드(숫자인지 검사하는)로 인해 그렇게 하지 못하고 있었습니다. 생성자 안에서 int로 변경하는 과정을 거쳤기 때문이죠.
그래서 해당 과정과 NumberFormatException이 던져지는 경우를 입력 받는 메서드에서 처리하고 해당 클래스 자체를 리팩토링 했습니다.

Copy link

Choose a reason for hiding this comment

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

분리해주신 코드가 훨신 낫다고 느껴지네요!

수정 후 코드에선 InputManager 클래스(아마 프레젠테이션 계층)에서 Domain 계층의 값을 의존하고 있습니다!
이런 형태의 코드의 장점이 무엇일까요!?

(혹시 아직 프레젠테이션 레이어, Domain 레이어 라는 말이 익숙하지 않다면 이번 기회에 레이어드 아키텍쳐 라는 키워드로 학습하시고 블로그 글 하나 작성 부탁드립니다 !)

Copy link
Author

Choose a reason for hiding this comment

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

각 계층의 역할을 분명하게 할 수 있습니다.
우선 입력과 유효성 검사, 관리 역할로 확실하게 나뉘어 하나의 책임을 갖고 유지보수에 용이해집니다.
프레젠테이션 계층인 InputManager는 도메인 계층인 PurchaseAmount에 의존하고 있습니다.
상위 계층이 하위 계층에 의존하도록 하면, 코드의 변경에 대한 영향을 최소화할 수 있습니다.
자신보다 변하기 쉬운 것에는 의존해서는 안됩니다.
따라서 독립적인 테스트가 편리해지며 코드의 가독성도 향상됩니다.

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

public class UserInputNumbers {
Copy link

Choose a reason for hiding this comment

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

이 코드는 Validation 만 수행하고 있는 클래스네요🤔 이 클래스의 책임으로 줄 수 있는 건 뭘까요!?

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

Choose a reason for hiding this comment

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

좋습니다ㅋㅋㅋㅋ 사실 저도 요구사항 자체가 간단해서 책임 할당은 이 정도로 충분하다고 생각해요 🧐

- 로또 결과 계산, 수익률 계산의 책임으로 분리
- 반복문을 스트림으로 변경하여 보다 명확하고 직관적인 코드로 수정
- service 오타 수정
- PurchaseAmount의 로컬 변수를 불변으로 선언 및 생성자 로직 변경
- 숫자 검증 메서드를 입력받는 메서드에서 처리
- 변수명 수정
- 번호 입력 메서드를 로또 번호를 입력받는 메서드, 보너스 번호를 입력받는 메서드로 분리
@@ -0,0 +1,20 @@
package lotto.util;

public enum ErrorMessage {
Copy link

Choose a reason for hiding this comment

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

enum으로 관리하는 것 vs 클래스의 상수로 두는 것 어떤 장단점이 있을까요!?

Copy link
Author

Choose a reason for hiding this comment

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

enum으로 관리했을 때의
장점: 관련 있는 상수들을 묶어서 관리할 수 있습니다. 이넘의 메서드를 이용해 기능을 추가할 수 있습니다.
단점: 인스턴스화할 수 없습니다. 동적으로 에러 메세지를 추가하거나 변경할 수 없습니다.

상수로 관리했을 때의
장점: 상수 선언 및 초기화만으로 관리하여 간결해집니다.
단점: 관련 있는 상수들을 묶어서 관리하기 어렵습니다. 모든 상수가 클래스의 필드로 선언되서 구분하기 어렵습니다.

Comment on lines +16 to +30
@DisplayName("로또 번호의 개수가 6개가 넘어가면 예외가 발생한다.")
@Test
void createLottoByOverSize() {
assertThatThrownBy(() -> new Lotto(List.of(1, 2, 3, 4, 5, 6, 7)))
.isInstanceOf(IllegalArgumentException.class);
}

@DisplayName("로또 번호의 범위가 1~45가 아닌 경우 예외가 발생한다.")
@Test
void createLottoByInvalidNumber() {
assertThatThrownBy(() -> new Lotto(List.of(0, 2, 3, 4, 5, 6)))
.isInstanceOf(IllegalArgumentException.class);
assertThatThrownBy(() -> new Lotto(List.of(1, 2, 3, 4, 5, 46)))
.isInstanceOf(IllegalArgumentException.class);
}
Copy link

Choose a reason for hiding this comment

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

경계값 테스트 매우 훌륭하네요..
지금은 공감이 안되겠지만 아마 한 4개월 쯤 뒤면 테스트코드 없이는 코드를 불안해서 못 짜는 사람이 되실 겁니다 그때 이 댓글 성지순례 한 번 하러 오세요ㅋㅋㅋㅋ

Comment on lines +11 to +31
@DisplayName("당첨 번호의 개수가 6개를 넘어가는 경우 예외가 발생한다.")
@Test
void createNumberByOverSize() {
assertThatThrownBy(() -> new UserInputNumbers(List.of(1, 2, 3, 4, 5, 6, 7), 8))
.isInstanceOf(IllegalArgumentException.class);
}

@DisplayName("당첨 번호의 범위가 1~45가 아닌 경우 예외가 발생한다.")
@Test
void createNumberByInvalidNumber() {
assertThatThrownBy(() -> new UserInputNumbers(List.of(0, 2, 3, 4, 5, 6), 7))
.isInstanceOf(IllegalArgumentException.class);
assertThatThrownBy(() -> new UserInputNumbers(List.of(1, 2, 3, 4, 5, 46), 7))
.isInstanceOf(IllegalArgumentException.class);
}

@DisplayName("당첨 번호에 중복된 숫자가 있는 경우 예외가 발생한다.")
@Test
void createNumberByDuplicatedNumber() {
assertThatThrownBy(() -> new UserInputNumbers(List.of(1, 2, 3, 4, 5, 5), 7))
.isInstanceOf(IllegalArgumentException.class);
Copy link

Choose a reason for hiding this comment

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

input(입력), output(출력) 을 도메인로직으로부터 분리하면 이렇게 테스트하기 좋은 코드가 된다는 것을 알려드리고 싶었습니다. 👍👍

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.

2 participants