이동민 PR#2 코드리뷰 요청 #3
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
게임 내부 & 외부 프린터 구현
- 출력 방식의 변경을 수월하게 할 수 있게 인터페이스 추출 - 게임 내/외부의 측면에서 책임을 나누는 것이 과도한 분리라고 생각됨
초기 설계와 다른 점이 있다: - 게임 내/외부 입력기의 구분을 없앰 - 확장성을 고려하여 InputReceiver 인터페이스를 선언함
인덴트 깊이를 증가시키는 try-catch 문 추출
- 세자리 수 입력이 아니면 false를 반환한다. - 세자리 수이지만 각 자리가 서로 다른 숫자가 아니면 false를 반환한다. - 세자리 수이면서 각 자리 숫자가 서로 다른 숫자이면 true를 반환한다.
난수 생성 클래스인 RandomNumberGenerator 클래스는 유일한 메서드 generate를 가지고 있고, 인스턴스 변수의 활용이 없기 때문에 static 메서드로 둬서 GameManager의 인스턴스 변수 개수를 줄였다.
값을 임의로 줄 수 없어서 생성하고 검증하는 절차를 100회 반복
결과를 담아두는 Result 클래스 구현 - 스트라이크 카운트와 볼 카운트를 상태로 갖고 있음 - 결과 출력에 사용됨 - 게임 종료 조건 확인에 사용되기도 함
Before: - digits의 인덱스에 char 형으로 접근 - digits의 인덱스에 사용자 숫자를 마킹하고 볼 카운팅할때 사용자 숫자를 비교 After: - digits의 인덱스에 접근하기 전에 대응되는 int 값으로 변환 - digits의 인덱스에 컴퓨터 생성 난수의 숫자를 마킹하도록 변경
각 테스트 메서드 상황: - 노카운트 case - 1스트라이크 case - 1스트라이크 1볼 case - 1스트라이크 2볼 case - 2스트라이크 case - 3스트라이크 case
0은 입력값이 될 수 없음 - integerDigit이 0일 경우 false 반환
InputMismatchException 예외 처리 추가 - InputMismatchException은 IllegalArumentException의 자식이 아닌 RuntimeException의 자식 클래스
- try~catch 구문 추가 - scanner는 항상 close 할 수 있도록 try-with-resources로 변경
Before: - result.isThreeStrike 로 3스트라이크인 경우에만 게임을 시작하는 버그 발생 After: - !result.isThreeStrike 로 변경
- 게임 승리 알림 - 숫자 입력 요청에 개행 x - 결과 출력에 개행
Before: - hasNext와 nextLine의 혼용 - 예외 발생시 버퍼 비우기를 두번 실행, 여러번 입력을 기다림 After: - haNext -> hasNextLine - receiveInputByIntAndClean 메서드를 지우고 scanner.nextInt로 축소
class GameManager: - 예외 처리문에 버퍼 비우기가 실행되지 않음 - 입력 관련 메서드는 InputReceiver가 하기 때문에 버퍼 비우기를 InputReceiver에게 위임 interface InputReceiver: - 인터페이스에 dealWithExceptionalInput 메서드 추가 class ConsoleInputReceiver - dealWithExceptionalInput 메서드는 ConsoleInputReceiver 구현체에서 cleanScannerBuffer에게 위임
Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
네이버 핵데이 Java 코딩 컨벤션 #8.4
상수의 위치는 클래스의 최상단
class GameNumber: Before: - 생성자로 인스턴스를 생성하였다. - 외부에서 GameNumber 객체를 얻기 위해서는 생성자를 호출해야 한다. After: - 정적 팩토리 메소드 패턴을 적용해 외부에서 GameNumber 객체를 얻기 위해서는 명시적인 팩토리 메소드를 호출해야 한다. - 생성자는 private으로 한정하여 외부에 노출시키지 않았다. class ConsoleInputReceiver: Before: - 매개변수로 GameNumber 객체 userNumber를 받아왔다. - GameNumber라는 외부 객체의 내부 상태를 설정해주고 있다. After: - 매개변수로 어떤 객체도 받고 있지 않다. - GameNumber의 정적 팩토리 메소드를 이용해 GameNumber 객체를 반환한다. - 외부 객체의 내부 상태를 변경하는 것이 아닌 새로운 객체를 생성하고 있다. class RandomnumberGenerator Before: - 매개변수로 GameNumber 객체 seedNumber를 받아온다. - GameNumber라는 외부 객체의 내부 상태를 설정해주고 있다. After: - 난수를 담은 새로운 객체를 생성해 반환한다. class GameManager, GameNumberTest: 새로운 객체 생성 방식에 맞게 코드 수정 class
InputReceiver는 단순히 표준 입력 스트림으로부터 사용자 입력을 받는 책임을 가짐 기존의 게임을 다시 시작/시작하는가는 게임의 비즈니스 로직 class ConsoleInputReceiver: Before: - 게임을 진행할지 여부를 판단하는 1이라는 상수가 InputReceiver의 구현체인 이 클래스에 위치 - 그에 따라 checkContinueGame이라는 메소드가 입력도 받고 게임을 진행할지 판단도 하는 책임이 얽힌 메소드가 됨 After: - 기존 checkContinueGame 메소드를 receiveContinueGameSelect로 바꿈 - receiveContinueGameSelect는 단지 사용자의 입력을 받아 숫자인지 검증하고 반환함 - 그에 따라 게임을 계속할지 판단하는 throwIfInputIsNotOneOrTwo 메소드가 사라짐 class GameManager: Before: - continueGame이라는 boolean 변수에 할당하는 checkContinueGame이 비즈니스 로직까지 처리함 After: - 기존에 ConsoleInputReceiver에 위치하던 START_GAME 상수가 게임 전체 로직을 담당하는 컨트롤러 클래스인 GameManager에 위치 - isContinueGame이라는 메소드 추가 - isContinueGame에서는 사용자 입력을 받고 1이나 2가 아니면 예외를 던짐. 또한 START_GAME과 같은지를 반환하여 계속할지 판단함 - throwIfInputIsNotOneOrTwo 메소드도 이 클래스에 옮겨짐 interface InputReceiver: boolean checkContinueGame() -> int receiveContinueGameSelect()
NPE에 대한 특별한 예외처리 로직이 없기 때문에 디버깅의 편의를 위해서라도 NPE throw 및 try~catch 문 삭제함
기존에 두 번호를 비교하여 스트라이크와 볼 카운트를 계산하고 할당하는 모든 책임이 GameNumber의 메소드 compare에 뭉쳐 있었던 악취 SRP로 제거 class GameNumber: Before: - compare 메소드가 두 번호를 비교하여 스트라이크/볼 카운트를 게산하고 Result 객체에 할당함 - GameNumber라는 외부 객체가 Result의 상태를 변경시키고 있음 After: - Result를 계산해야 하는 비즈니스 로직상 GameNumber에 비교하는 로직이 위치해서는 안됨 - compare 메소드 제거 class Result: Before: - 자신의 상태를 GameNumber가 변경하도록 했음 - 그에 따라 set 메소드가 존재 After - 두 번호를 받아 비교하고 스트라이크 카운트를 계산하는 calculateStrikeCount 메소드 추가 - 볼 카운트를 계산하는 calculateBallCount 메소드 추가. Java8 스트림 적용 시도. - 볼 카운트 계산은 스트라이크 카운트에 의존하기 때문에 스트라이크 카운트를 먼저 카운팅 하도록 순서를 보장하는 calculateResult만 외부에 노출시키고 calculateStrikeCount와 calculateBallCount 메소드는 private로 접근 한정
InputReceiver의 책임은 입력을 받는 것뿐이기 때문에 GameNumber 객체를 생성하는 책임을 분리시킴
kouz95
requested changes
Feb 24, 2021
Member
There was a problem hiding this comment.
리뷰 반영이 굉장히 빠르시군요 🙂
코드 퀄리티를 보니 금방 금방 성장하시는 것 같습니다. 앞으로의 모습이 기대가 됩니다. 👍
추가적으로 리뷰 남겼으니, 확인해주세요!
아 그리고 리뷰 반영이 끝난후에 PR을 닫고 다시 보낼 필요는 없습니다.
작업하던 브랜치에 커밋을 하고 push를 하면 원래 PR에 적용이 됩니다. 리뷰 반영한다음 push해주시면 됩니다.
또, PR창 우측 상단에 reviewer를 선택할 수 있는 기능이 있는데 거기서 리뷰어를 선택하면 메일로 알림이 와서 좀 더 빨리 확인할 수 있을것 같아요 🙂
리뷰 반영을 한 이후에 추가 리뷰를 요청할때도, reviewer창에 재리뷰를 요청하는 버튼(새로고침 모양)이 있으니 확인해보세요!
Comment on lines
+8
to
+11
| public Result() { | ||
| this.strikeCount = 0; | ||
| this.ballCount = 0; | ||
| } |
Member
There was a problem hiding this comment.
저번 리뷰와 비슷한 부분인데 이곳에도 적용이 될 수 있을것 같아요.
new Result()를 보면 모든 필드가 null로 초기화 되려나? 혹은 아무런 필드도 안들고 있는 객체(무상태 객체)인가? 라는 생각이 들어요.
Suggested change
| public Result() { | |
| this.strikeCount = 0; | |
| this.ballCount = 0; | |
| } | |
| public Result(int strikeCount, int ballCount) { | |
| this.strikeCount = strikeCount; | |
| this.ballCount = ballCount; | |
| } |
처럼 바꾸어 new Result(0, 0);으로 사용하거나, 정적 팩토리 메서드를 통하여 초기 상태라는 것을 표현해줄 수 있지 않을까요?
이 메서드를 통해 strikeCount와 ballCount가 0으로 초기화 될거야 라는 걸 사용하는 쪽에서 인지 할 수 있었으면 좋겠어요!
놀람 최소화 원칙(principle of least astonishment, principle of least surprise, POLA)은 사용자 인터페이스와 소프트웨어 설계에 적용되는 원칙이다. "필요한 기능에 크나큰 깜짝 놀래킬만한 요소가 있다면 해당 기능을 다시 설계할 필요가 있을 수 있다"는 것이 이 원칙의 일반적인 공식이다.
더 일반적으로 이야기하면 이 원칙은 시스템의 구성 요소가 대부분의 사용자들이 행동할 것으로 예측되는 방식으로 동작하는 것이 좋다는 것을 의미한다. 즉, 해당 동작이 사용자들을 놀래키지 않는 것이 좋다는 것이다.
kouz95
reviewed
Feb 24, 2021
kouz95
reviewed
Feb 24, 2021
| return new GameNumber(number); | ||
| } | ||
|
|
||
| public static boolean validateNumber(int number) { |
Member
There was a problem hiding this comment.
중복 또는 범위를 체크하는 과정이 생각보다 복잡한것 같아요.
(프로그래머가 아닌 숫자야구게임만 아는 사람이 이해할 수 있을까요?)
개선할 수 있는 방법은 없을지 고민해볼까요? 🤔
상수는 변경되지 않는 변수이므로 JVM에 한번 올려두고 같은 주소의 값을 참조해도 문제가 되지 않는다. static으로 선언하면 하나의 JVM이나 WAS 인스턴스에서는 같은 주소에 존재하는 겂을 참조한다. 그러므로 static을 잘 사용하면 성능을 뛰어나게 향상시킬 수 있지만, 잘못 사용하면 예기치 못한 결과를 초래하게 된다. [자바 성능 튜닝 이야기, 이상민] Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.