-
Notifications
You must be signed in to change notification settings - Fork 8
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
[로또 게임] 양재승 과제 제출합니다. #5
base: main
Are you sure you want to change the base?
Conversation
fix to push list winningNumber
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.
클래스 설계 능력이 많이 좋아진 것 같습니다 👍
객체 간 의존관계를 고려하면서 리팩토링 해보세요. 스프링을 배우기 시작하면 더 와닿을 내용입니다!
예외처리에 대한 내용은 리뷰에 남겨놓았습니다.
src/main/java/lotto/Calculator.java
Outdated
public void printResult(){ | ||
System.out.println("당첨 통계"); | ||
System.out.println("---"); | ||
System.out.println("3개 일치 (5,000원) - " + countThree + "개"); | ||
System.out.println("4개 일치 (50,000원) - " + countFour + "개"); | ||
System.out.println("5개 일치 (1,500,000원) - " + countFive + "개"); | ||
System.out.println("5개 일치, 보너스 볼 일치 (30,000,000원) - " + countFiveAndBonus + "개"); | ||
System.out.println("6개 일치 (2,000,000,000원) - " + countSix + "개"); | ||
} |
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.
한번의 System 메소드 호출로 출력하도록 개선해보세요. 개행문자를 적절히 사용하면 가독성도 개선할 수 있겠네요.
public class ExceptionController { | ||
|
||
public static void noIntegerValueException(String Numbers){ | ||
for(int i=0; i<Numbers.length(); i++){ | ||
int isIntger = Numbers.charAt(i) - '0'; | ||
if(isIntger<0 || isIntger>9){ | ||
System.out.println(NOT_INTAGER_ERROR_MESSAGE); | ||
throw new IllegalArgumentException(); | ||
} | ||
} | ||
} | ||
|
||
public static void noValidAmountException(String amount){ | ||
if(Integer.parseInt(amount)%1000 != 0){ | ||
System.out.println(NOT_THOUSAND_ERROR_MESSAGE); | ||
throw new IllegalArgumentException(); | ||
} | ||
} |
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.
예외를 던지기만 하지 처리를 하진 않습니다. 첫번째 과제에선 예외를 던지기만 해도 요구사항을 충족했는데 이번 과제는 더 개선해야 합니다 :)
에러메시지를 어떻게 출력할 수 있는지 알아보세요.
src/main/java/lotto/Calculator.java
Outdated
switch (correctCount) { | ||
case 3: | ||
countThree++; | ||
totalIncome += 5000; | ||
break; | ||
case 4: | ||
countFour++; | ||
totalIncome += 50000; | ||
break; | ||
case 5: | ||
countFive++; | ||
totalIncome += 1500000; | ||
break; | ||
case 6: | ||
countSix++; | ||
totalIncome += 2000000000; | ||
break; | ||
|
||
} |
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.
클린코드의 영역인데 switch case문은 지양하는 편이 좋습니다.
src/main/java/lotto/LottoGame.java
Outdated
LottoGame(){ | ||
System.out.println(Message.INPUT_PURCHASE_AMOUNT.getMessage()); | ||
String amount = Console.readLine(); | ||
|
||
ExceptionController.noIntegerValueException(amount); | ||
ExceptionController.noValidAmountException(amount); | ||
|
||
coin = Integer.parseInt(amount)/1000; | ||
} |
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.
메소드로 분리해봅시다. 초기화 한다는 측면에서 생성자의 역할을 하는 것 처럼 보이지만 입출력, 예외처리 등 너무 많은 관심사들이 섞여있습니다.
src/main/java/lotto/LottoList.java
Outdated
import java.util.List; | ||
|
||
public class LottoList { | ||
Lotto[] lottos; |
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.
숫자의 개수가 늘어나도 동작하도록 리팩토링 해보세요.
src/main/java/lotto/LottoGame.java
Outdated
LottoGame(){ | ||
System.out.println(Message.INPUT_PURCHASE_AMOUNT.getMessage()); | ||
String amount = Console.readLine(); | ||
|
||
ExceptionController.noIntegerValueException(amount); | ||
ExceptionController.noValidAmountException(amount); | ||
|
||
coin = Integer.parseInt(amount)/1000; | ||
} | ||
|
||
public void run(){ | ||
lottoList = new LottoList(coin); | ||
winningNumbers = new WinningNumbers(); | ||
|
||
Calculator calculator = new Calculator(lottoList, winningNumbers); | ||
calculator.printResult(); | ||
calculator.printIncomeRate(coin); | ||
} |
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.
생성자를 위와 같이 구현했다면 필드에 있는 lottoList와 winningNumbers를 생성자에서 초기화하는 것이 더 자연스럽지 않을까요?
src/main/java/lotto/LottoGame.java
Outdated
public class LottoGame { | ||
LottoList lottoList; | ||
WinningNumbers winningNumbers; | ||
int coin; |
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.
캡슐화가 지켜지지 않았네요.
로또 클래스를 클래스 배열로 나누었고
계산기능 함수들을 묶어 클래스로 만들었습니다.
한가지 궁금한 점은 예외처리 결과는 잘 나오는데 테스트 코드가 pass가 안되는데, 혹시 단지 기능 구현 문제인지 테스트 코드 형식에 틀리기 때문인지가 궁금합니다
남은시간 리팩토링에 몰두하겠습니다! 감사합니다!