-
Notifications
You must be signed in to change notification settings - Fork 0
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
[로또] 조윤호 제출합니다 #3
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 util 클래스는 어떤 프로젝트에 갖다 놔도 사용 가능한 클래스라고 생각해요.
LottoUtil 같은 경우는 딱 이 미션에서만 사용가능한 것이겠죠?
이 미션의 핵심 로직이 Util 클래스로 새어나갔다는 뜻..
public final static int LOTTO_NUM_LENGTH = 6; | ||
public final static int LOTTO_MIN_NUM = 1; | ||
public final static int LOTTO_MAX_NUM = 45; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final 형태로 상수를 관리하는 것은 굉장히 안좋은 방법입니다!
절차 지향적인 방법으로 코드를 작성하고 계신것 같아요
try { | ||
numbers.sort(Comparator.naturalOrder()); | ||
} catch (UnsupportedOperationException ignored) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로덕션 코드에서 테스트코드의 영향을 받아야할까요?
테스트 작성시 문제가 생겼다면 테스트 코드를 변경하는게 맞지 않을까요??
for (Lotto lotto : lottos) | ||
sb.append(lotto).append("\n"); | ||
|
||
UserOutput.printLottos(lottos.size(), sb.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domain에서 view에 의존하는 것이 맞을까요?
MVC 패턴을 생각해보면 Model View Controller 가 서로 분리되도록 설계합니다.
왜 분리되어야할까요?
public class LottoBundle { | ||
private List<Lotto> lottos = new ArrayList<>(); | ||
private Lotto winningLotto; | ||
private int bonus; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불변 객체로 만들지 않으면 프로그램이 동작할때 어떤 문제가 발생할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수에서 한 가지보다 많은 일들을 수행하고 있는 경우가 많은 것 같아요!
개인적으로 코드 컨벤션을 중요하게 생각해서, 코드 컨벤션에 대해 한 번 읽어보시고 프로그래밍할 때 신경써서 구현하시면 전체적으로 코드 가독성이 더 높아질 것 같아요ㅎㅎ
아래 링크에 나오는 컨벤션이 정답은 아닙니다! 참고하시라고 같이 첨부해드려요😄
자바 코드 컨벤션
if (numbers.size() != LottoConfig.LOTTO_NUM_LENGTH) | ||
throw new IllegalArgumentException("[ERROR] 숫자의 개수가 올바르지 않습니다."); | ||
if (numbers.size() != (new HashSet<>(numbers)).size()) | ||
throw new IllegalArgumentException("[ERROR] 숫자에 중복이 없어야 합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate가 한 번에 '개수 검증'과 '중복 검증'이라는 두 가지 일을 처리하고 있네요.
개수 검증과 중복 검증을 분리하면 메소드의 책임이 좀 더 명확해질 것 같아요!
if (cost <=0 ) | ||
throw new IllegalArgumentException("[ERROR] 1 이상의 양수를 입력해주세요"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음수와 0 처리는 생각하지 못했던 부분이네요! 👍🏻
if (cost <=0 ) | ||
throw new IllegalArgumentException("[ERROR] 1 이상의 양수를 입력해주세요"); | ||
if (cost % PRICE_PER_LOTTO != 0) | ||
throw new IllegalArgumentException("[ERROR] 가격의 배수에 해당하는 가격을 입력해주세요"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검증의 책임을 분리하면 좋을 것 같아요!
int num; | ||
try { | ||
num = Integer.parseInt(input); | ||
} catch (NumberFormatException ne) { | ||
throw new IllegalArgumentException("[ERROR] 정수를 입력해주세요."); | ||
} | ||
return num; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 식으로 early return 패턴을 사용하는게 더 가독성이 좋을 것 같아요!
int num; | |
try { | |
num = Integer.parseInt(input); | |
} catch (NumberFormatException ne) { | |
throw new IllegalArgumentException("[ERROR] 정수를 입력해주세요."); | |
} | |
return num; | |
try { | |
return Integer.parseInt(input); | |
} catch (NumberFormatException ne) { | |
throw new IllegalArgumentException("[ERROR] 정수를 입력해주세요."); | |
} |
int cost = getNum(userInput); | ||
int lottoNum = PurchaseService.getLottoNum(cost); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구입금액은 그냥 정수보다 좀 더 의미있는 값 같아요. '구입금액'이라는 객체를 생성하는 건 어떨까요?
그렇게 되면 '구입금액'이라는 객체를 생성할 때 검증을 할 수 있어서, 구입금액을 구매할 로또 개수를 가져올 때 계산하는 것보다 흐름이 자연스러울 것 같아요!
/** | ||
* 보너스번호 저장하기 | ||
*/ | ||
public void setBonus(int bonus) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setter 사용 대신 좀 더 의미있는 네이밍을 하면 좋을 것 같아요!
Lotto lotto = new Lotto(winningNumList); | ||
this.winningLotto = lotto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotto lotto = new Lotto(winningNumList); | |
this.winningLotto = lotto; | |
this.winningLotto = new Lotto(winningNumList); |
재사용 가능성이 없는 경우 변수에 담지 않고 바로 사용하는게 더 좋은 것 같아요!
public int[] getPrizeStat() { | ||
int[] prizeStat = new int[Prize.values().length]; | ||
for (Lotto lotto : lottos) { | ||
Prize prize = lotto.comparePrize(winningLotto, bonus); | ||
if (prize.isPrized()) | ||
prizeStat[prize.ordinal()]++; | ||
} | ||
return prizeStat; | ||
} | ||
|
||
public int getTotalReward(int[] prizeStat) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 메소드는 LottoBundle 내부에서만 사용되네요. private로 선언해주시면 좋을 것 같아요!
for (Prize p : Prize.values()) { | ||
if ( | ||
p.isPrized() && | ||
p.getMatchNum() <= matches && | ||
p.getGrade() <= prize.getGrade() | ||
) { | ||
if (p.isBonus() && !isBonus) // 보너스 true인 경우 isBonus 확인한다. | ||
continue; | ||
prize = p; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- indent(인덴트, 들여쓰기) depth를 3이 넘지 않도록 구현한다. 2까지만 허용한다.
- 예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
- 힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메서드)를 분리하면 된다.
라는 프로그래밍 요구사항을 만족하지 않는 구현 같습니다!
로또의 최대, 최솟값, 숫자개수를 여러 클래스에서 사용해야 해서, 어느 클래스에 저장해 놓아도 부자연스럽더라구요. 결국 LottoConfig라는 새로운 클래스에 저장해놓았습니다. 이렇게 하는게 맞는지 모르겠네요~
또 테스트코드에서 List.of()를 사용하게 되면 immutable이라 sort가 안되더라구요. 그래서 immutable 관련 예외가 발생하면 다음과 같이 그냥 무시해버렸는데, 마음에 걸립니다 ㅎㅎ.. (프리코스때도 이 문제로 꽤 고생했던걸로 기억해요!)
코드 보시며 다양한 의견과 조언 적어주시면 큰 도움이 될 것 같습니다!