[숫자 야구 게임] 오형석 과제 제출합니다. #4
Conversation
make it compile on Java 11
- make two array to store random, input numbers - make getRandomNumbers() - make getInputNumbers()
two methods - checkBall() - checkStrike()
… input wether go or stop
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| public class ExceptionHandler { |
There was a problem hiding this comment.
ExceptionHandler는 예외를 받아주는 쪽에 가까운데 구현된거를 보니 값을 검증하고 예외를 던지는 쪽에 가깝네요. 다른 네이밍을 생각해보면 좋을 것 같아요
src/main/java/baseball/Random.java
Outdated
|
|
||
| public class Random { | ||
|
|
||
| public int[] makeRandomNumbers() { |
There was a problem hiding this comment.
이 함수를 사용하는 입장에서 함수의 구현체를 보지 않으면 몇 개의 number가 생성될지 모를거 같네요.
리턴 타입을 int[]가 아니라 다른 타입으로 바꿔서 구현해보세요.
src/main/java/baseball/Random.java
Outdated
| Set<Integer> s = new HashSet<>(); | ||
| int[] randomNumbers = new int[3]; | ||
| while (s.size() < 3) { | ||
| int randomNumber = Randoms.pickNumberInRange(1, 9); |
There was a problem hiding this comment.
범위가 1-9가 아니라 3-6, 2-8 처럼 언제든지 범위를 수정할 수 있도록 리팩토링 해보세요.
| } | ||
|
|
||
| private void lengthCheck(String input) { | ||
| if (input.length() < 3 || input.length() > 3) { |
There was a problem hiding this comment.
여기는 or 조건 없이 하나의 조건으로 축약할 수 있을 거 같은데요.
| return ball; | ||
| } | ||
|
|
||
| private boolean getResult(int strike, int ball) { |
| } | ||
| } | ||
|
|
||
| private int checkStrike() { |
| int[] randomNumbers = new int[3]; | ||
| int[] inputNumbers = new int[3]; |
There was a problem hiding this comment.
게임 난이도를 올리기 위해 5자리 숫자로 변경하겠습니다와 같은 요구사항에도 대처할 수 있도록 수정해보세요.
| for (int i = 0; i < 3; i++) { | ||
| set.add(input.charAt(i) - '0'); | ||
| if (input.charAt(i) - '0' <= 0 || input.charAt(i) - '0' > 9) { | ||
| throw new IllegalArgumentException("1-9 숫자만 가능합니다"); | ||
| } |
There was a problem hiding this comment.
이건 약간의 팁인데 정규표현식을 사용하면 중복조건을 제외하고 조건 하나로 이 로직들을 처리할 수 있습니다.
| private void getInputNumbers() { | ||
| ExceptionHandler exceptionHandler = new ExceptionHandler(); | ||
| System.out.print("숫자를 입력해주세요 : "); | ||
| String input = Console.readLine(); | ||
| exceptionHandler.handleException(input); | ||
| for (int i = 0; i < 3; i++) { | ||
| inputNumbers[i] = input.charAt(i) - '0'; | ||
| } | ||
| } |
There was a problem hiding this comment.
상수와 출력문들을 분리해보세요. 요구사항에 따라 상수와 출력문은 변경될 수 있습니다.
… and naming - start, end, number : change constant to static variable - goOrStop -> restartOrExit - combine checkStrike, checkBall - make isSuccess method
|
해주신 코드 리뷰 반영해서 리팩토링 했습니다! 리뷰 부탁드리겠습니다 감사합니다1 |
| static int start = 1; | ||
| static int end = 9; | ||
| static int number = 3; |
There was a problem hiding this comment.
Constant는 별도의 클래스로 분리시키는 것이 좋습니다.
There was a problem hiding this comment.
public class Rule {
static int start = 1;
static int end = 9;
static int number = 3;
}
하나의 클래스가 요렇게만 쓰여도 괜찮을까요?
There was a problem hiding this comment.
Spring의 Http Status Code 관련 클래스를 참고하는 것도 도움될 것 같네요 :) 변하지 않는 값이면 final로 선언해주는 것도 괜찮고요
| int strike = 0; | ||
| int ball = 0; | ||
|
|
||
| ArrayList<Integer> randomNumbers = new ArrayList<>(); | ||
| ArrayList<Integer> inputNumbers = new ArrayList<>(); |
There was a problem hiding this comment.
외부에서 해당 값들을 변경하지 못하도록 개선해봅시다.
초기화는 생성자에서 처리해주는 것이 나아보입니다.
| private void restartOrExit() { | ||
| String input = Console.readLine(); | ||
| if (input.equals(Constant.RESTART.value)) { | ||
| start(); | ||
| } | ||
| } |
There was a problem hiding this comment.
input의 경우의 수를 나누어서 각 경우에 따른 로직이 메소드에 담기도록 리팩토링 해보세요.
| System.out.println(number + "개의 숫자를 모두 맞히셨습니다! 게임 종료"); | ||
| System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요"); |
|
|
||
| private void wrongInputCheck() { | ||
| Set<Integer> set = new HashSet<>(); | ||
| String pattern = "^[" + BaseballGame.start + "-" + BaseballGame.end +"]*$"; |
There was a problem hiding this comment.
이런건 별도의 메소드로 분리해도 괜찮을 것 같습니다. 다만 정규표현식을 사용하는 것이 좋은 코드인지는 고민할 필요가 있어보이네요.
| public class Validation { | ||
|
|
||
| String inputString; | ||
|
|
||
| Validation(String inputString) { | ||
| this.inputString = inputString; | ||
| } |
There was a problem hiding this comment.
inputString을 메소드의 파라미터로 받는 것이 더 나아보입니다. 새로운 String을 검증할 때마다 객체를 생성해줘야하는 번거로움이 있네요.
| private void lengthCheck() { | ||
| if (inputString.length() != BaseballGame.number) { | ||
| throw new IllegalArgumentException("정해진 개수의 숫자만 입력해주세요"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Exception Message도 별도의 클래스에 분리할 수 있겠네요.
| private void getResult() { | ||
| if (strike == 0 && ball == 0) { | ||
| System.out.println("낫싱"); | ||
| } | ||
| if (ball > 0) { | ||
| System.out.print(ball + "볼 "); | ||
| if (strike == 0) { | ||
| System.out.println(); | ||
| } | ||
| } | ||
| if (strike > 0) { | ||
| System.out.println(strike + "스트라이크"); | ||
| } | ||
| } |
There was a problem hiding this comment.
다음을 고려하여 리팩토링 해보세요.
- 출력 기능을 다른 메소드로 분리시키기
- early return등을 활용하여 불필요한 if문 참조 줄이기
if(strike ==0)이 필요하지 않도록 개선해보기
| private void playGame() { | ||
| while (true) { | ||
| getInputNumbers(); | ||
| checkStrikeAndBall(); | ||
| getResult(); | ||
| if (isSuccess()) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
성공과 실패여부에 대한 처리로직 모두를 코드에 표현해주는 것이 나아보입니다.
if (!isSuccess()){
continue;
}
break;| public void handleException() { | ||
| lengthCheck(); | ||
| wrongInputCheck(); | ||
| } |
There was a problem hiding this comment.
handleException()이라는 메소드명이 부자연스럽습니다. 입력에 대한 validation을 처리하는 메소드로 판단되므로 더 자연스러운 메소드명을 지어봅시다. 결국 Validation이라는 클래스를 부자연스럽게 설계해서 발생한 문제라고 판단됩니다.
|
고심하여 다시 리팩토링 해보았습니다! |
|
라이브 코드리뷰 완료 |
No description provided.