[송세일] 숫자야구 제출합니다. #4
Conversation
|
checkNumber 메소드 요구사항에 있는 뎁스 3 이 넘지 않도록 하는것에 안맞네요 ㅜ |
mintjordy
left a comment
mintjordy
left a comment
There was a problem hiding this comment.
안녕하세요. 쉽지 않았을텐데 요구사항대로 숫자야구 구현하시느라 고생하셨습니다. 😀
나름대로 역할분리를 고민하면서 구현한 흔적이 많이 보였어요.
개인적인 생각과 아쉬운 부분들 코멘트 남겼으니 확인 후 반영할 부분들 해주시면 좋을 것 같아요.
의문 드는 부분은 편하게 의견 남겨주세요.
| public void setStrike(int strike) { | ||
| this.strike = strike; | ||
| } | ||
|
|
||
| public int getBall() { | ||
| return ball; | ||
| } | ||
|
|
||
| public void setBall(int ball) { | ||
| this.ball = ball; | ||
| } |
There was a problem hiding this comment.
사용하지 않는 setter들은 제거해주세요.
더불어서 setter는 실제로 사용하는 것을 지양하고 있어요. setXXX 라는 이름으로는 해당 메서드로 무엇을 하려는지 의도를 파악하기 힘들기 때문이죠.
| public void inputNumberToString(Scanner scanner) throws Exception { | ||
| this.userNum = String.valueOf(scanner.nextInt()); | ||
|
|
||
| if (notNumberLengthIsThree()) { | ||
| throw new Exception(Message.INCORRECT_DIGIT_MESSAGE); | ||
| } else if (existsDuplicateDigit()) { | ||
| throw new Exception(Message.DUPLICATE_DIGIT_MESSAGE); | ||
| } | ||
| } |
There was a problem hiding this comment.
SOLID 5원칙중 단일 책임원칙에 따르면 하나의 객체의 변경 포인트는 한가지만 존재해야 하는 것이 바람직합니다.
변경 포인트가 한가지여야 한다는 것을 다시 해석하면 객체들은 한가지 역할만 수행해야 한다는 의미입니다.
도메인 객체는 주요 비즈니스 로직중 한가지 기능에 대해서만 책임지는 것이 바람직합니다.
현재 User는 비즈니스 로직 외 Scanner를 사용한 외부 입력에 대해서도 책임지고 있어요.
이러한 구조는 단일책임원칙에 위배되는 구조이죠!
예를 들어, 현재는 게임에 필요한 숫자 값을 콘솔을 통해 입력 받고 있어요.
그런데 요구사항이 변경되어 숫자 값에 대한 입력을 파일, 혹은 웹 요청을 통한 입력받아야 한다면 그때마다 User를 변경해야겠죠?
User를 포함한 도메인 객체는 비즈니스 로직이 변경되는 경우에만 변경이 일어나야 합니다.
따라서 User에서 외부 입출력부분을 책임지는 대신 입출력을 책임지는 객체를 별도로 분리하는 것이 바람직해 보여요.
이는 MVC 패턴을 생각하면 더 이해가 쉬울것 같아요.
| package controller; | ||
| import constant.Message; | ||
| import domain.Computer; |
There was a problem hiding this comment.
java 코딩 컨벤션에 따르면 package문과 import문은 한칸 공백이 있어야 합니다.
IDE의 자동 포매팅 기능을 활용하면 기본적인 포매팅문제는 해결할 수 있을거에요!
(인텔리제이 , 맥 기준 command + option + L 단축키를 활용하시면 됩니다!)
| String computerNum = computer.getComputerNum(); | ||
| Result gameResult = new Result(); | ||
|
|
||
| for (int i=0; i<userNum.length(); i++) { |
There was a problem hiding this comment.
| for (int i=0; i<userNum.length(); i++) { | |
| for (int i = 0; i < userNum.length(); i++) { |
java 컨벤션 위반! 마찬가지로 IDE의 자동 포매팅을 활용하는 습관을 가지면 좋을듯 해요!
| private boolean isBall(int userDigit, Computer computer) { | ||
| String computerNum = computer.getComputerNum(); | ||
|
|
||
| for (int i=0; i<computerNum.length(); i++) { |
| if (strike > 0) | ||
| System.out.print(strike + Message.STRIKE_MESSAGE); | ||
| if (ball > 0) | ||
| System.out.print(ball + Message.BALL_MESSAGE); | ||
| if (strike == 0 && ball == 0) | ||
| System.out.print(Message.NO_STRIKE_NO_BALL_MESSAGE); |
There was a problem hiding this comment.
java 컨벤션에 따르면 한줄짜리 if 문이더라도 중괄호문을 생략하지 않는 것을 권장하고 있어요.
중괄호문을 생략할 수 있는 조건문이더라도 중괄호문을 사용해주세요!
There was a problem hiding this comment.
해당하는 기능 같이 출력을 책임지는 View 클래스 등으로 분리한 뒤 controller는 출력의 역할을 View 클래스에게 위임하면 controller가 가벼워지고 보다 응집력 있는 객체가 될 수 있을것 같아요!
| private Scanner scanner; | ||
| private Computer computer; | ||
| private User user; | ||
| private Result result; | ||
| private BaseballService baseballService; | ||
|
|
There was a problem hiding this comment.
하나의 클래스에서 최대한 필드수를 최소화 하는것을 권장하고 있어요.
필드수가 많다는 것은 그만큼 해당 클래스가 수행하는 역할이 많아진 다는 것이고 이는 클래스 분리의 신호라는 뜻이에요!
그런데 해당 클래스에서 과연 User, Result, Computer, Scanner 등의 필드가 필수적으로 필요할까요?
다시한번 고민해보시고 필드가 가벼운 클래스로 만들어보시면 좋을것 같아요.
| private Result result; | ||
| private BaseballService baseballService; | ||
|
|
||
| public BaseballConsole() { |
There was a problem hiding this comment.
해당 클래스는 컨트롤러 역할을 하고 있네요.
MVC 패턴을 기반으로 프로그램을 만들기 위해 시도했군요! 👍
그런데 지금의 컨트롤러 클래스는 컨트롤러 (프로그램 흐름 제어)와 뷰(외부 입출력) 두가지를 모두 수행하는 무거운 클래스로 보이네요.
단일 책임의 원칙 관점에서 컨트롤러에서 뷰의 역할을 수행하는 기능들을 별도의 View 클래스로 분리하면 보다 가벼운 컨트롤러로 만들 수 있을 것 같아요!
| System.out.print(Message.INPUT_NUMBER_MESSAGE); | ||
| user.inputNumberToString(scanner); |
There was a problem hiding this comment.
이 부분은 콘솔을 통한 외부 입력이 수행되는 부분인데요, 외부 입력을 책임지는 View 클래스를 부여하고 view에게 역할을 위임하면 좋을 것 같아요. 지금의 구조에서는 View의 역할을 컨트롤러와 모델(User 도메인)까지 수행하게 되어서 MVC 의 역할분리가 제대로 이뤄지지 않고 있어요.
| public static final String INPUT_NUMBER_MESSAGE = "숫자를 입력해주세요 : "; | ||
| public static final String INCORRECT_DIGIT_MESSAGE = "세 자리 수를 입력해야 합니다."; | ||
| public static final String DUPLICATE_DIGIT_MESSAGE = "중복되는 수가 존재합니다."; | ||
| public static final String STRIKE_MESSAGE = " 스트라이크 "; | ||
| public static final String BALL_MESSAGE = "볼"; | ||
| public static final String NO_STRIKE_NO_BALL_MESSAGE = "낫싱"; | ||
| public static final String SUCCESS_MESSAGE = "3개의 숫자를 모두 맞히셨습니다! 게임 종료" + System.lineSeparator() + "게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요."; |
4 participants
No description provided.