[조윤호] 야구 게임 미션 #2
Conversation
| public static boolean isThreeDigit (String test) { | ||
| String pattern = "\\d{3}"; | ||
| return test.matches(pattern); | ||
| } | ||
| public static boolean isBaseball (String test) { | ||
| String pattern = "^(?!.*(.).*\\1)\\d{3}$"; | ||
| return test.matches(pattern); | ||
| } | ||
|
|
||
| public static boolean isCommand (String test) { | ||
| String pattern = "[12]"; | ||
| return test.matches(pattern); | ||
| } |
There was a problem hiding this comment.
전체적으로 String.matches를 사용하고 계신데요!
matches 내부로 들어가면 Pattern 을 사용하는 것을 알 수 있습니다.
입력이 들어올때마다 Pattern 객체를 생성하고 버리는 과정이 진행되는데요
이 부분을 캐싱하면 어떨까요?
자세한 내용은 이펙티브 자바 아이템 6에서 확인하실 수 있습니다~
| } | ||
|
|
||
| public GameStatus singleTurn(List<Integer> player){ | ||
| int strike = 0, ball = 0; |
There was a problem hiding this comment.
한 줄에 두 개의 변수를 선언하면,, 가독성이 떨어지는 것 같습니다!
| public static boolean isThreeDigit (String test) { | ||
| String pattern = "\\d{3}"; | ||
| return test.matches(pattern); | ||
| } | ||
| public static boolean isBaseball (String test) { | ||
| String pattern = "^(?!.*(.).*\\1)\\d{3}$"; | ||
| return test.matches(pattern); | ||
| } | ||
|
|
||
| public static boolean isCommand (String test) { | ||
| String pattern = "[12]"; | ||
| return test.matches(pattern); | ||
| } |
There was a problem hiding this comment.
추가로 만약에 야구 게임의 숫자가 4자리로 늘어난다면?
이 클래스 뿐만 아니라 다른 클래스도 변경해야하는데,
이런식으로 변경해야할 부분이 여러 군데라면 변경하기 굉장히 힘들겠죠..?
| public SingleGame() { | ||
| while(computer.size() < 3) { | ||
| int randomNumber = Randoms.pickNumberInRange(1, 9); | ||
| if (!computer.contains(randomNumber)) | ||
| computer.add(randomNumber); | ||
| } | ||
| } |
There was a problem hiding this comment.
이런 방법으로 초기 숫자를 생성할 예정이라면
LinkedHashSet 자료구조를 썻어도 좋았을 것 같네요.
순서를 보장하면서 중복이 없도록 만들어줘서 좋습니다 :)
| List<List<Integer>> inputs = new ArrayList<>(List.of( | ||
| List.of(4, 5, 6), // n | ||
| List.of(4, 5, 1), // 1b | ||
| List.of(1, 5, 6), // 1s | ||
| List.of(1, 5, 2), // 1b1s | ||
| List.of(1, 2, 3) // 3s | ||
| )); |
There was a problem hiding this comment.
사용자가 0 을 입력해도 정상적으로 돌아가네요
예를 들어 012 같은거 ?
|
|
||
| startGame(); | ||
| while (isContinue) | ||
| isContinue = playSingleGame(); |
There was a problem hiding this comment.
playSingleGame()이 boolean을 반환하는게 좀 어색하다고 느껴집니다!
| private static UserInput userInput; | ||
| private static UserOutput userOutput; |
There was a problem hiding this comment.
전역변수로 선언하는 것보다 파라미터로 넘겨주는게 더 좋지 않을까요?
| @@ -0,0 +1,18 @@ | |||
| package baseball.inout; | |||
|
|
|||
| public class CheckRegex { | |||
There was a problem hiding this comment.
해당 클래스는 유틸성 클래스같은데, 이런 경우 기본 생성자를 private로 막아주어 외부에서 객체 생성을 하지 못하도록 막아주어야 합니다!
| public void setComputer (List<Integer> list) { | ||
| computer = list; | ||
| } |
There was a problem hiding this comment.
테스트를 위한 메소드 작성은 좋지 않은 것 같습니다.
그리고 setter 사용보다는 생성자를 이용해서 생성해주면 좋을 것 같아요!
| if (computer.get(i) == player.get(i)) strike ++; | ||
| else if (computer.contains(player.get(i))) ball ++; |
There was a problem hiding this comment.
단항연산자 앞에 공백은 삭제해주시면 가독성이 더 좋아질 것 같습니다!
| if (computer.get(i) == player.get(i)) strike ++; | |
| else if (computer.contains(player.get(i))) ball ++; | |
| if (computer.get(i) == player.get(i)) strike++; | |
| else if (computer.contains(player.get(i))) ball++; |
| public void initMessage() { | ||
| System.out.println("숫자 야구 게임을 시작합니다."); | ||
| } | ||
|
|
||
| public void statusMessage(GameStatus status) { | ||
| if (status.ball == 0 && status.strike == 0) | ||
| System.out.println("낫싱"); | ||
| else if (status.ball == 0) | ||
| System.out.println(status.strike + "스트라이크"); | ||
| else if (status.strike == 0) | ||
| System.out.println(status.ball + "볼"); | ||
| else | ||
| System.out.println(status.ball + "볼 " + status.strike + "스트라이크"); | ||
|
|
||
| } | ||
|
|
||
| public void endMessage() { |
There was a problem hiding this comment.
메소드 네이밍이 어색한 것 같습니다.
초기 메시지를 출력하다., 상태 메시지를 출력하다., 종료 메시지를 출력하다.가 메소드의 행위를 더 명확하게 나타낼 수 있을 것 같습니다!
| // then | ||
| assertThat(status).isEqualTo(expects.get(i)); | ||
| } | ||
| } |
| if (!CheckRegex.isThreeDigit(input)) | ||
| throw new IllegalArgumentException("3자리의 숫자를 입력해야 합니다."); | ||
| if (!CheckRegex.isBaseball(input)) |
There was a problem hiding this comment.
검증 로직이 모두 '어떤 조건을 만족한가'와 '그렇지 않은가' 즉, !(not)을 사용해서 이루어지는데, 저는 이런 방식보다는 조건문에 '어떤 조건을 만족하지 않은가'를 넣는게 코드가 의미하는 바를 더 명확하게 나타낼 것 같은데 어떻게 생각하시나용
| List<Integer> playNum = userInput.getNum(); | ||
| GameStatus gameStatus = singleGame.singleTurn(playNum); | ||
| userOutput.statusMessage(gameStatus); | ||
| isCorrect = gameStatus.isCorrect(); |
| boolean isCorrect = false; | ||
|
|
||
| while (!isCorrect) { | ||
| List<Integer> playNum = userInput.getNum(); | ||
| GameStatus gameStatus = singleGame.singleTurn(playNum); | ||
| userOutput.statusMessage(gameStatus); | ||
| isCorrect = gameStatus.isCorrect(); | ||
| } |
There was a problem hiding this comment.
반복문을 실행하기 위해 boolean 변수를 선언한 후 반복문에서 상태를 바꾸게 되면 처음에는 게임 상태가 올바르지 않아서(종료가 아니어서) 시작하게 되는데, 이런 동작이 어색하다고 느껴집니다.
게임을 실행하면 해당 과정을 반복하는데 게임 상태가 올바르면(종료이면) 반복을 종료한다. 이런 로직이 좀 더 자연스럽지 않을까요??
| import baseball.game.model.GameCommand; | ||
| import baseball.game.model.GameStatus; | ||
| import baseball.game.GameService; | ||
| import baseball.inout.UserInput; | ||
| import baseball.inout.UserOutput; | ||
|
|
||
| import java.util.List; |
There was a problem hiding this comment.
안쓰는 임포트는 지워주세요!
인텔리제이를 사용하신다면, 맥 기준 ctrl+option+o(영어 오) / 윈도우 기준 ctrl+alt+o 임포트 정리 단축키를 사용해서 간편하게 정리할 수 있습니다ㅎㅎ
|
|
||
| public class GameService { | ||
|
|
||
| GameBall computer; |
| throw new IllegalArgumentException("숫자의 범위가 올바르지 않습니다."); | ||
|
|
||
| List<Integer> ball = new ArrayList<>(); | ||
| for (int i = 0; i < 3; i++) |
There was a problem hiding this comment.
| for (int i = 0; i < 3; i++) | |
| for (int i = 0; i < BALL_NUM; i++) |
| if (isInvalidLength(userInput)) | ||
| throw new IllegalArgumentException("3자리의 숫자를 입력해야 합니다."); | ||
| if (isInvalidRange(userInput)) | ||
| throw new IllegalArgumentException("숫자의 범위가 올바르지 않습니다."); |
There was a problem hiding this comment.
볼을 만드는 메소드에서 검증 책임을 같이 수행하고 있네요. 검증하는 로직을 메소드로 분리하면 좋을 것 같아요!
| int commandNumber; | ||
| try { | ||
| commandNumber = Integer.parseInt(userInput); | ||
| } catch (NumberFormatException ex) { | ||
| throw new IllegalArgumentException("숫자를 입력해야 합니다."); | ||
| } |
| /** | ||
| * 게임 진행여부 입력 | ||
| */ | ||
| public static String isContinue() { |
There was a problem hiding this comment.
메소드 네이밍이 어색합니다. isContinue()(계속되는가)라고 물어보면 예/아니오(boolean값)이 반환될 것이 예상되는데 String이 반환되네요.
| if (status.ball == 0 && status.strike == 0) | ||
| System.out.println("낫싱"); | ||
| else if (status.ball == 0) | ||
| System.out.println(status.strike + "스트라이크"); | ||
| else if (status.strike == 0) | ||
| System.out.println(status.ball + "볼"); | ||
| else | ||
| System.out.println(status.ball + "볼 " + status.strike + "스트라이크"); |
There was a problem hiding this comment.
그렇군요..!! GameStatus로 옮기는 것을 고려해봐야겠네요
view에서는 단순히 문자열을 받아서 출력하는 것이 좋은가요?
There was a problem hiding this comment.
저는 입출력 클래스는 메인 서비스 로직의 영향을 받지 않아야된다고 생각합니다.
그런데 지금처럼 출력이 아닌 로직을 포함할 경우, 볼과 스트라이크를 구하는 방식이 변경된다면 '게임 결과'가 변경되는 것인데 '게임 결과'가 아닌 '출력'의 로직을 수정해야 합니다. 이는 굉장히 어색한 책임 같습니다.
'출력'을 위한 객체가 출력이 아닌 책임까지 갖게 되면 책임이 모호해져서, 입출력에는 최대한 로직을 넣지 않는 것을 선호하는 편입니다!
| @Test | ||
| void getCommand() { | ||
| } | ||
|
|
||
| @Test | ||
| void isContinue() { | ||
| } |
주관적인 의견이라도 편하게 리뷰 남겨주시면 감사하겠습니다!
잘 부탁드립니다 :)