Sdrs6921 by sdrs6921 · Pull Request #1 · sdrs6921/java-baseball-playground

sdrs6921 · 2021-08-18T03:34:50Z

자바 baseball 기능 구현

seonghun127

전반적으로 코드 퀄리티가 매우 높아서 놀랬습니다. 실력이 나날이 증가하고 있군뇨 :)
다만 코드 포맷팅, final 키워드, 상수의 접근 제어자는 한번 전체 정리가 필요해보여요.

view 영역을 비지니스와 분리하는 부분에 대해 코멘트를 남겼습니다. 이번 리뷰에 핵심인 내용이라 생각해요. 한번 고민해보시고 궁금한게 있다면언제든지 편한 방법으로 질문주세요~

seonghun127 · 2021-08-19T15:17:52Z

src/main/java/domain/Ball.java

+    public static final int MIN_NUMBER = 0;
+    public static final int MAX_NUMBER = 9;
+    public static final int MIN_POSITION = 0;
+    public static final int MAX_POSITION = 3;


여기 상수들은 Ball 클래스 내부에서만 사용되고 있네요.

단축키를 사용하다 보니 private 으로 바꾸는 것을 깜빡했습니다. number와 position 모두 값 객체로 따로 빼서 private으로 접근 제어자를 수정하였습니다

seonghun127 · 2021-08-19T15:27:45Z

src/main/java/domain/Balls.java

+        }
+    }
+
+    private static List<Integer> extractDistinctNumber(final List<Integer> numbers) {


중복 제거가 목적이라면 33번째줄에서 Set 자료구조를 사용하는건 어떨까요?

중복을 제거할 때 Set 자료구조를 사용하도록 변경하였습니다

seonghun127 · 2021-08-20T15:43:15Z

src/main/java/controller/RoundController.java

+    }
+
+    private static List<Integer> initializeRandomBalls() {
+        Set<Integer> set = new HashSet<>();


HashSet 자료구조는 순서를 보장하지 않기 때문에 입력된 숫자 그대로 List에 담기지 않을 것으로 보여요~

근데 이 로직은 사용자가 맞춰야하는 숫자 정보군요. 순서가 크게 상관없겠네요.

그래도 LinkedHashSet을 사용하여 순서가 보장되도록 변경하였습니다!

seonghun127 · 2021-08-20T16:30:56Z

src/main/java/view/GameView.java

+
+import static java.lang.System.out;
+
+public class GameView {


view에 대해 이야기를 해보고 싶은데요,

기본적으로 view는 비지니스 영역과 철저하게 분리되어야합니다. 이미 프로젝트를 하면서 알겠지만 view는 그자체로 독립적이어야하며 view의 변경으로 비지니스 영역에 변경이 일어나면 상당히 골치 ㅜ아파지거든요. 그순간 소프트웨어가 아니게 된다고 생각합니다. (전혀 소프트하게 대응이 불가하니까요.)

view 영역을 인터페이스로 더 분리시키면 좋겠다는 생각이 들어요. 현재 view를 담당하는 클래스들이 있는데 모두 정적 메서드만 제공하고 있습니다. 그래서 이를 사용하는 사용처 (Controller)에선 직접 호출하고 있습니다. 즉, 매우 구체적으로 view에 대해 알고 있다고 볼 수 있어요.
물론 Controller이기에 view 아는게 문제가 되는게 뭐냐 할 수 있지만 view는 변경이 매우 잦을 수 있다는 점을 감안하면 이는 큰 문제가 될 수 있습니다. 만약 view가 콘솔이 아닌 다른 방식으로 처리된다면 어떻게 될까요?
File 형태로 input이 들어온다고 가정하면 Controller에 변경이 일어나게 됩니다. File 전용 view 클래스가 만들어지고 Controller에선 File 전용 view를 의존하게 되고 콘솔 전용 view와 File 전용 view를 분기처리해야하는 경우까지 이어질 수 있습니다.
이처럼 어떠한 클래스를 구체적으로 안다는 것은 변경에 유연하지 못합니다. 이를 테스트 코드가 증명하죠. 테스트로 컨트롤하기가 어렵기 때문입니다.

그래서 view와 비지니스 영역 사이에 인터페이스를 두는 구조로 가면 좋을 것 같습니다. 리펙토링하는 방향에 대해서는 구체적으로 말씀드리진 않을게요. 한번 고민해보시면 좋을 듯 합니다. 일요일 만남 때 이야기해봐도 좋고요.

자주 변경되는 view 와 비즈니스 영역을 인터페이스를 사용하여 분리하도록 변경하였습니다.

seonghun127

view 단 인터페이스 분리하니 좋네요. 코드도 전반적으로 더 깔끔해진 것 같습니다. 👍
몇가지 피드백 남겼으니 확인부탁드려요~

다만 아래 항목들이 코드 전반적으로 컨벤션을 지키지 못한 채로 남아있네요. 한번 프로젝트 전반적으로 훑어보고 개선시켜주면 좋겠습니다.

접근제어자
final 키워드
띄워쓰기

seonghun127 · 2021-09-04T06:44:35Z

src/main/java/domain/vo/BallNumber.java

+    private static final int MIN_BALL_NUMBER = 1;
+    private static final int MAX_BALL_NUMBER = 9;
+
+    private int number;


final 키워드가 있으면 좋겠네요.

코드 전체적으로 final 키워드를 붙여 주었습니다

seonghun127 · 2021-09-04T06:47:35Z

src/main/java/domain/vo/BallNumber.java

+    }
+
+    @Override
+    public boolean equals(final Object o) {


equals(), hashCode() 구현 로직은 클래스 제일 하단에 위치하는게 컨벤션입니다.

validate() 메서드가 클래스 제일 하단에 위치하고 있는데요, 보통은 private 메서드는 이를 사용하고 있는 메서드 바로 아래에 위치하는게 좋습니다. 지금은 from() 메서드를 읽다가 validate() 의 내부 로직을 보려면 화면이 이동해야하는데요, 코드를 읽는 피로도가 늘어납니다. 만약 validate()메서드가 from() 메서드 바로 아래 위치했다면 화면 이동없이 자연스레 코드를 읽을 수 있을 것입니다.

메서드 위치랑 자바 컨벤션에 대해 조금 더 알게 되었습니다! validate()와 eqauls(), hashcode()의 위치를 바꿔주었습니다!

seonghun127 · 2021-09-04T06:52:57Z

src/main/java/domain/Ball.java

+    private final Position position;

-    private Ball(final int number, final int position) {
+    public Ball(final BallNumber number, final Position position) {


이 생성자는 외부에서 사용되는 곳이 없네요. 접근 제어자 변경이 필요해보입니다.

외부에서 사용되지 않는 생성자를 신경쓰지 못했습니다. 생성자의 접근 제어자를 변경하였습니다!!

seonghun127 · 2021-09-04T07:18:44Z

src/main/java/domain/Balls.java

+
+public class Balls {
+
+    public static final int START_POSITION = 1;


여기도 접근 제어가 변경이 필요해보이네요.

접근제어자에 대한 피드백이 계속 반복되고 있어요.
IDE의 단축키 사용 목적을 다시 한번 생각해볼 필요가 있어보입니다.
단축키 사용은 개발 편의성이 주된 목적인데요, 그 편의성에 전제는 적절한 코드 컨벤션과 객체지향 코드입니다.
다시 말해서 단축키로 상수화는 좋지만 private 접근제어자를 설정해줬을 때 그 의미가 있다고 생각해요. 지금 코드는 public 이기 때문에 객체의 캡슐화를 방해하고 있습니다.
단축키를 사용하면 public 으로 자동 완성해주기 때문에 휴먼에러가 발생하는 것은 한두번까지는 괜찮았지만 이제부턴 발생하면 안되는 문제라고 생각해요. 코드 짜고 고민하느라 정신없겠지만 반복되는 문제가 있다면 이를 어떻게 해결할 수 있을지 고민해보는 것도 좋은 개발자의 자세라고 볼 수 있습니다~

아래 링크 참고하시고 단축키의 자동완성을 private으로 되도록 바꿔주면 좋겠네요~

https://www.jetbrains.com/help/idea/extract-constant.html

접근제어자에 대한 피드백이 계속해서 나오고 있는데도 잘 반영하지 못했네요... 이 피드백은 추후에 다시는 나오지 않도록 철저하게 반영하겠습니다!!

seonghun127 · 2021-09-04T07:45:40Z

src/main/java/view/GameConsoleView.java

+        return new GameConsoleView(scanner);
+    }
+
+    public GameStatus gameInput() {


메서드 재정의 시 @Override 어노테이션을 붙여주세요.
어떤 메서드가 재정의 됐는지 가시적으로 볼 수 있는게 코드를 더 수월하게 읽을 수 있습니다.

implements 단축키를 사용하면 자동으로 어노테이션이 달린 메서드가 추가될거에요.

재정의한 메서드에 @Override 키워드를 추가해주어서 재정의한 메서드라는 것을 명시해 주는 것으로 변경하였습니다

seonghun127 · 2021-09-04T08:13:42Z

src/main/java/controller/GameController.java

+    private final GameView gameView;
+    private final RoundController roundController;
+
    private GameStatus gameStatus;


GameController가 GameStatus를 클래스 필드로 갖고있어야하는 이유가 무엇인가요?

굳이 필드로 두지 않아도 괜찮을 것 같습니다. 메서드 내 변수로 관리하도록 변경하였습니다

seonghun127 · 2021-09-04T08:21:15Z

src/main/java/domain/Round.java

+public class Round {
+    public static final int ENOUGH_STRIKE_INCLUSIVE = 3;
+    private Balls balls;
+    private boolean isOver;


이 필드는 특정 상황에 따라 계속 변경되는 값인데요, Round 객체를 불변 객체로 변경해보면 어떨까요?
isOver 필드가 담당하는 기능을 충분히 메세지를 보내서 확인할 수 있어보여요~

모든 객체를 불변 객체로 변경하였습니다. isOver 필드가 담당하는 기능은 메세지를 보내서 활용하도록 변경하였습니다!

seonghun127

간단하게 피드백 남겼어요~ 확인부탁드려요 :)

seonghun127 · 2021-09-08T14:37:34Z

src/main/java/baseball/controller/RoundController.java

@@ -36,13 +36,17 @@ public static RoundController of(final RoundInputView roundInputView, final Roun

    public void run() {
        Round round = nextRound();


nextRound() 가 RoundController에 있어야할까요?
Round에게 책임을 위임하면 어떨까요?

참고로 Controller 영역은 로직이 있으면 안된다고 생각해요.
Controller의 역할은 input과 output을 그리는 것이고 이를 위해 필요한 비지니스 로직을 호출하는 곳이라고 생각합니다.
객사오에서 이야기하는 협력의 장이 될 수 있을거고요.

seonghun127 · 2021-09-08T14:39:54Z

src/main/java/baseball/controller/RoundController.java

-        while (!round.isOver()) {
+        int strike = INITIAL_STRIKE_VALUE;
+
+        while (!round.hasEnough(strike)) {


while 문의 처음 round.hasEnough()는 무조건 false를 반환할테니 최초 1회는 연산이 수행될 필요가 없습니다. do~while을 적용해보면 좋을 것 같습니다.

seonghun127 · 2021-09-08T14:43:20Z

src/main/java/controller/RoundController.java

-    public static RoundController getInstance() {
+    private Round nextRound() {
        List<Integer> numbers = initializeRandomBalls();
        Balls balls = Balls.from(numbers);


Balls.from()이 반드시 List로 인자를 받아야하나요?
이 부분 때문에 밑에 initializeRandomBalls()메서드에서 Set이었던 자료구조가 다시 List로 복사되고 있습니다.
이 복사는 불필요해보여요.

sdrs6921 added 17 commits August 1, 2021 15:20

chore : 자바 버전 변경

2e3d8b4

feat : Ball 도메인 테스트 및 구현

d4664a6

feat : Ball 정적 팩토리 메서드 변환

6405206

feat : Balls 테스트 및 구현

6e98c18

feat : 숫자와 위치를 비교하는 메서드 테스트 및 기능 구현

095f012

feat : 사용자 입력 DTO 테스트 및 구현

9dd497a

feat : Dto 이름 변경

78e36d0

feat : RoundOutputDto 테스트 및 구현

75cc6cd

refactor : final 키워드 추가

638f9c5

feat : Round 도메인 테스트 및 구현

5d7a0ad

feat : GameStatus 도메인 테스트 및 구현

5b40911

feat : RoundInputView, RoundOutputView 구현

7c335bc

feat : RandomUtils 테스트 및 기능 구현

852b6d7

feat : RoundController 기능 구현

ca1ce3f

feat : GameView 구현

437ac9f

feat : GameController 기능 구현

2c18d86

refactor : 게임 종료시 출력 추가

0266c7e

seonghun127 reviewed Aug 20, 2021

View reviewed changes

sdrs6921 added 12 commits August 31, 2021 14:41

refactor : View 인터페이스 분리

79051c8

refactor : BallNumber VO 객체 테스트 및 구현

f68276c

refactor : Position VO 객체 테스트 및 구현

510f345

refactor : BallStatus 테스트 및 구현

1126d10

refactor : Ball 상태를 리턴하도록 변경

fa9bdf2

refactor : Balls 비교 메서드 로직 변경

0058a55

refactor : BallStatus 일급컬렉션 테스트 및 구현

60d98f8

refactor : RoundInputDto 권한 검증 로직 수정

4835be7

refactor : 난수 생성 전략 패턴 구현

30b244f

refactor : Round BallStatus 일급 컬렉션 사용

aba6370

refactor : GameStatus 문자열로 변경

ea315f2

refactor : Util 클래스 삭제 및 전략 패턴 클래스로 변경

70ba8b8

sdrs6921 added 3 commits September 1, 2021 01:15

refactor : Main 메서드 수정

9e5fd66

refactor : 패키지 구조 수정

5c89623

refactor : 중복 제거시 Set 자료구조 사용

bda0bd7

seonghun127 reviewed Sep 4, 2021

View reviewed changes

sdrs6921 added 6 commits September 6, 2021 15:10

refactor : final 키워드 추가

cc706a7

style : 메서드 위치 변경

1212900

refactor : 생성자 접근 제어자 변경

677c907

refactor : 외부에서 사용하지 않는 상수 접근 제어자 변경

5f0b905

refactor : 재정의한 메서드에 @OverRide 키워드 추가

650891f

refactor : 모든 객체를 불변 객체로 변경

a6ae180

seonghun127 reviewed Sep 8, 2021

View reviewed changes

sdrs6921 added 2 commits September 16, 2021 15:51

refactor : Balls 생성자 매개변수 변경

728b2fd

refactor : Controller 로직 Round로 분리

2e86f14


		public class Balls {

		public static final int START_POSITION = 1;

		@@ -36,13 +36,17 @@ public static RoundController of(final RoundInputView roundInputView, final Roun

		public void run() {
		Round round = nextRound();

Conversation

sdrs6921 commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seonghun127 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seonghun127 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seonghun127 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seonghun127 Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sdrs6921 commented Aug 18, 2021 •

edited

Loading

seonghun127 Sep 8, 2021 •

edited

Loading