숫자야구게임 제출합니다! #2
Conversation
# Conflicts: # src/main/java/baseball/GamePlayer.java
| System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요."); | ||
| if(scanner.nextInt()==1) | ||
| continue; |
There was a problem hiding this comment.
다른 코드에서 try~catch로 예외처리를 해주셨는데 이 코드에서만 안해주고 있습니다.
이런 비일관성이 발생한 이유가 연관된 상태와 행위를 결정하는 기능을 묶어주는 캡슐화를 하지 않은 것으로부터 발생한 것으로 보입니다. 입력을 받는 모든 행위와 그와 관련된 상태(변수)들을 클래스를 이용해 캡슐화를 해보세요! 😃
위와 같은 이유로 게임의 비즈니스 로직과 사용자 입력을 받고, 필요한 문장을 출력하는 일이 혼재되어 있습니다.
이런 코드를 생산시 유지보수하기 어렵고 코드의 가독성을 떨어뜨립니다.
이런 문제를 해결하는 솔루션으로 MVC 패턴이 있습니다. 링크를 참고해보세요 👍
| try { | ||
| temp = Input.nextInt(); | ||
| }catch (InputMismatchException e) { | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); | ||
| } |
There was a problem hiding this comment.
try~catch로 예외 처리를 시도해주셨지만 catch 문에서 발생한 예외 상황에 예외 객체를 던지기만 할뿐 어떤 처리도 해주고 있지 않고 있습니다.
이렇게 되면 프로그램이 함수 스택을 따라 예외 처리를 던지고, main에서도 처리를 해주고 있지 않기 때문에 프로그램을 종료하게 됩니다.
숫자를 다시 입력받거나 다른 처리를 해주는 것이 낫지 않을까요?
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); | ||
| } | ||
|
|
||
| inputCheck(temp); |
There was a problem hiding this comment.
코딩 컨벤션에 따르면 checkInput이 어울릴 것 같아요!
| public static void play(Scanner Input){ | ||
| Number random = new Number(); | ||
| Number player = new Number(); | ||
| Integer temp; |
There was a problem hiding this comment.
사용자 입력을 받아서 그 상태를 가지고 있는 변수가 temp라는 이름으로 그 의도를 드러내지 못하고 있습니다.
클린코드라는 책에서는 한 챕터를 의미 있는 이름으로 할당할 만큼 의도가 분명한 이름의 중요성은 아무리 강조해도 지나치지 않은 것 같습니다! 코딩할 때 힘들더라도 의미 있는 이름을 지어주려고 노력해보세요. 😃
| Integer temp; | |
| Integer userInputNumber; |
| player.setNumber(splitNumber(temp)); | ||
|
|
||
| Referee referee = new Referee(); | ||
| if (referee.judgement(random.getNumber(), player.getNumber())) |
There was a problem hiding this comment.
2.10. 메서드 이름은 동사/전치사로 시작
컨벤션이 안지켜지고 있네요.
| public static void is_Not_ThreeDigits(Integer number){ | ||
| if(number.toString().length()!=3) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); | ||
| } | ||
|
|
||
| //중복일 경우 | ||
| public static void is_overlap(ArrayList<Integer> number){ | ||
| ArrayList<Integer> temp = new ArrayList<>(); | ||
|
|
||
| for(int i = 0 ; i< 3; i++){ | ||
| if(temp.contains(number.get(i))) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); | ||
| temp.add(number.get(i)); | ||
| } | ||
| } | ||
|
|
||
| //0이 입력될 경우 | ||
| public static void is_Include_Zero(ArrayList<Integer> number){ | ||
| if(number.contains(0)) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); |
There was a problem hiding this comment.
2.9. 메서드 이름에 소문자 카멜표기법 적용
컨벤션을 지켜주고 계시다가 몇몇 메소드에서 스네이크 표기법을 사용하셨네요!
| //임의의 수 3개 생성 | ||
| public static ArrayList<Integer> createNumber(){ | ||
| ArrayList<Integer> result = new ArrayList<>(); | ||
| int temp; |
| public static ArrayList<Integer> splitNumber(Integer playerNumber) { | ||
| ArrayList<Integer> splitPlayerNumber = new ArrayList<>(); | ||
| for (int i = 0; i < 3; i++) { | ||
| splitPlayerNumber.add(0, playerNumber % 10); | ||
| playerNumber /= 10; | ||
| } | ||
| return splitPlayerNumber; | ||
| } | ||
|
|
||
| //비정상적인 입력 처리 | ||
| public static void inputCheck(Integer number){ | ||
| is_Include_Zero(splitNumber(number)); | ||
| is_Not_ThreeDigits(number); | ||
| is_overlap(splitNumber(number)); | ||
| } | ||
|
|
||
| //세자리 수가 아닐 경우 | ||
| public static void is_Not_ThreeDigits(Integer number){ | ||
| if(number.toString().length()!=3) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); | ||
| } | ||
|
|
||
| //중복일 경우 | ||
| public static void is_overlap(ArrayList<Integer> number){ | ||
| ArrayList<Integer> temp = new ArrayList<>(); | ||
|
|
||
| for(int i = 0 ; i< 3; i++){ | ||
| if(temp.contains(number.get(i))) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); | ||
| temp.add(number.get(i)); | ||
| } | ||
| } | ||
|
|
||
| //0이 입력될 경우 | ||
| public static void is_Include_Zero(ArrayList<Integer> number){ | ||
| if(number.contains(0)) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); |
There was a problem hiding this comment.
static 메서드가 너무 많이 존재하는 것 같습니다.
자바 프로그래밍에서는 static 사용을 지양하는데 그 이유는 이 링크를 참고해보세요.
모든 이유가 객체 지향적이지 않기 때문으로 귀결되는 것 같아요. 객체지향에 대해 공부해보시면 좋겠네요.
저도 어제 추천받은 책이라 읽고 있는 중이지만 <객체지향의 사실과 오해, 조영호> 라는 책을 참고해보시면 좋을것 같아요. 두께도 다른 IT 서적에 비하면 얇아서 덜 부담될 겁니다. 👍
| public int count_Strike(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ | ||
| for(int i = 0 ; i < 3; i++){ | ||
| this.strike += is_equals(randomNumber.get(i), playerNumber.get(i)); | ||
| } | ||
| return this.strike; | ||
| } | ||
|
|
||
| public int count_Ball(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ | ||
| for(int i = 0; i < 3; i++){ | ||
| for(int j = 0; j < 3; j++){ | ||
| this.ball += is_equals(randomNumber.get(i), playerNumber.get(j)); | ||
| } | ||
| } | ||
| this.ball -= this.strike; | ||
| return this.ball; | ||
| } | ||
|
|
||
| public int is_equals(int randomNumber, int playerNumber){ | ||
| if(randomNumber==playerNumber) | ||
| return 1; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
2.9. 메서드 이름에 소문자 카멜표기법 적용
컨벤션이 안 지켜지고 있네요 😢
| public int count_Strike(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ | |
| for(int i = 0 ; i < 3; i++){ | |
| this.strike += is_equals(randomNumber.get(i), playerNumber.get(i)); | |
| } | |
| return this.strike; | |
| } | |
| public int count_Ball(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ | |
| for(int i = 0; i < 3; i++){ | |
| for(int j = 0; j < 3; j++){ | |
| this.ball += is_equals(randomNumber.get(i), playerNumber.get(j)); | |
| } | |
| } | |
| this.ball -= this.strike; | |
| return this.ball; | |
| } | |
| public int is_equals(int randomNumber, int playerNumber){ | |
| if(randomNumber==playerNumber) | |
| return 1; | |
| return 0; | |
| } | |
| public int countStrike(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ | |
| for(int i = 0 ; i < 3; i++){ | |
| this.strike += is_equals(randomNumber.get(i), playerNumber.get(i)); | |
| } | |
| return this.strike; | |
| } | |
| public int countBall(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ | |
| for(int i = 0; i < 3; i++){ | |
| for(int j = 0; j < 3; j++){ | |
| this.ball += is_equals(randomNumber.get(i), playerNumber.get(j)); | |
| } | |
| } | |
| this.ball -= this.strike; | |
| return this.ball; | |
| } | |
| public int isEquals(int randomNumber, int playerNumber){ | |
| if(randomNumber==playerNumber) | |
| return 1; | |
| return 0; | |
| } |
| this.ball = 0; | ||
| } | ||
|
|
||
| public boolean judgement(ArrayList<Integer> randomNumber, ArrayList<Integer> playerNumber){ |
There was a problem hiding this comment.
이 메서드가 너무 많은 기능을 하는 것 같습니다.
미션을 수행하는 우리는 "난수와 사용자 번호를 판단한다" 라고 해석하지만,
아예 모르는 사람이 이 코드를 읽을 때 뭘 판단하는지 알 수가 없을만큼 함수 이름이 추상화 수준이 높고
추상화 수준이 높은 함수 이름은 곧 메소드가 많은 기능을 하는 것을 의미해요.
=> 스트라이크 개수 세기 + 볼 개수 세기 + 결과 출력하기 + 게임 종료 조건 판단 의 기능을 하고 있네요.
다른 이름을 고려해서 책임을 분리해보세요. 😃
| while(true){ | ||
| gamePlayer.play(scanner); | ||
| System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요."); | ||
| if(scanner.nextInt()==1) |
There was a problem hiding this comment.
여기서 1만 보고 게임 시작이란 것을 알 수 있을까요?
상수를 통해 값에 의미를 부여하면 어떨까요?
| public class GamePlayer { | ||
|
|
||
| public static void play(Scanner Input){ | ||
| Number random = new Number(); |
There was a problem hiding this comment.
Number 객체를 만든것 👍🏻
이런것을 값 객체(Value Object) 라고 해요.
VO에 대해서도 한번 학습해보세요.
객체로 감싼 것은 잘 했지만 Number라는 것만 보고는 이 숫자는 무슨 숫자인지 파악할 수가 없어요.
조금 더 명확한 이름으로 변경하는 것은 어떨까요?
ex) PhoneNumber, LottoNumber 등등
| public static void play(Scanner Input){ | ||
| Number random = new Number(); | ||
| Number player = new Number(); | ||
| Integer temp; |
There was a problem hiding this comment.
int가 아닌 Integer 클래스를 사용한 이유는 무엇인가요? 두개는 무슨 차이가 있을까요?
| System.out.print("숫자를 입력해 주세요 : "); | ||
| try { | ||
| temp = Input.nextInt(); | ||
| }catch (InputMismatchException e) { |
|
|
||
| //게임 시작 | ||
| while(true){ | ||
| gamePlayer.play(scanner); |
There was a problem hiding this comment.
지금의 코드는 인스턴스(gamePlayer)를 통해 메소드(play)를 호출하고있어요.
하지만 play는 static 메소드이고, static 메소드는 인스턴스를 생성 할 필요가 없이 Gameplayer.play로 호출이 가능해요.
GamePlayer.play와 gamePlayer.play는 무슨 차이가 있을까요?
| ArrayList<Integer> splitPlayerNumber = new ArrayList<>(); | ||
| for (int i = 0; i < 3; i++) { | ||
| splitPlayerNumber.add(0, playerNumber % 10); | ||
| playerNumber /= 10; |
| is_Include_Zero(splitNumber(number)); | ||
| is_Not_ThreeDigits(number); | ||
| is_overlap(splitNumber(number)); |
There was a problem hiding this comment.
자바에선 일반적으로 테스트메소드를 제외하면 메소드 이름에 _를 넣지 않습니다.
첫 번째 문자를 소문자로 시작하고 어절마다 첫 글자를 대문자로 사용하는 camelCase를 사용한답니다.
적어주신 방법은 공백을 _로 표현하는 snake_case에요.
| //세자리 수가 아닐 경우 | ||
| public static void is_Not_ThreeDigits(Integer number){ | ||
| if(number.toString().length()!=3) | ||
| throw new IllegalArgumentException("올바른 입력이 아닙니다!"); |
There was a problem hiding this comment.
지금 구현한 모든 예외가 같은 메시지를 출력해주고있어요.
각각 경우에 맞는 메시지를 적어준다면 사용자가 이유를 알기 쉽지 않을까요?
| if(this.strike==3) { | ||
| System.out.print("3스트라이크\n3개의 숫자를 모두 맞히셨습니다! 게임 종료"); | ||
| return true; | ||
| } | ||
| if(this.strike != 0 && this.ball != 0) | ||
| System.out.println(this.ball + "볼 " + this.strike + "스트라이크"); | ||
| else if(this.strike == 0 && this.ball != 0) | ||
| System.out.println(this.ball + "볼"); | ||
| else if(this.strike != 0 && this.ball == 0) | ||
| System.out.println(this.strike + "스트라이크"); | ||
| else{ | ||
| System.out.println("낫싱"); | ||
| } |
There was a problem hiding this comment.
count_Strike, count_Ball 처럼 메소드로 분리했다면 좀 더 명확하게 표현할 수 있을것 같아요.
어렵게 느껴지시면 메소드를 차근차근 영어 읽듯이 읽어보세요.
judgement -> count Strike -> count Ball -> 무엇이 나오면 좋을까요? 그 행위를 메소드명으로 잘 표현해주세요.
이렇게하면 복잡한 코드가 무엇을 의미하는지 파악하기 쉬운 깔끔한 코드가 된답니다.
| for(int i = 0 ; i < 3; i++){ | ||
| this.strike += is_equals(randomNumber.get(i), playerNumber.get(i)); | ||
| } | ||
| return this.strike; |
There was a problem hiding this comment.
this.strike는 클래스 내부에서 어디서든 사용이 가능해요. 위의 경우 굳이 반환하지 않아도 될 것 같아요.
감사합니당