[1, 2, 3단계 - 체스] 연로그(권시연) 미션 제출합니다. #297

yeon-06 · 2022-03-28T06:10:58Z

안녕하세요 스티치~! 연로그 입니다
이번 체스 미션으로 만나게 되어 너무 반갑습니다
많이 어렵고 복잡한 미션이었어서 어떤 리뷰를 받을지 벌써부터 두근거리네요😆
잘 부탁드립니다~!!

구현 기능

현재 구현한 체스 미션의 대략적인 기능은 아래와 같습니다.

입력 명령어에 따른 여러 동작 (start, move, status, end)
체스판 초기화
체스 말 이동
- 체스 규칙에 따라 각 기물들 이동 가능
- pawn의 대각선 이동은 다른 팀의 말을 잡을 때만 가능
- pawn의 첫수(첫 시작)는 1칸 또는 2칸 이동 가능
- pawn은 팀의 색깔에 따라 이동 가능 방향이 다름
- check 상태인 경우 check 상태를 벗어나기 위한 이동만 가능
체스 종료
- end 명령어 입력
- checkmate 상황 발생 시 종료 가능
현재 점수 계산

리팩터링 예정

아직 적용하지 않았지만 고려해보고 있는 사항은 아래와 같습니다.

Command 부분에 상태 패턴 도입 (2022.03.29 18:35 적용 완료)
- 상태 패턴을 사용해보고 싶었으나 시간적 관계로 1단계 PR 제출 전까지 구현하기 힘들 것 같아 먼저 PR을 올리게 되었습니다🙏
스티치가 리뷰해주시는 사항들

lxxjn0

연로그, 이번에 체스를 리뷰하게 된 스티치입니다 :)

생각보다 복잡한 도메인인데 매우 잘 구현을 해주었네요!! 👍🏼 몇가지 개선해보면 좋을만한 부분들에 대해서 피드백 남겨두었습니다! 확인 후 반영 부탁드릴게요 :)

README.md

src/main/java/chess/console/state/Ready.java

src/main/java/chess/console/state/State.java

src/main/java/chess/console/ChessGame.java

src/main/java/chess/domain/position/Row.java

src/main/java/chess/domain/position/Position.java

yeon-06 · 2022-03-30T13:08:58Z

src/main/java/chess/domain/state/Status.java

+    @Override
+    public State run(List<String> inputs) {
+        Score score = board.createResult();
+        OutputView.printStatus(score.getValue(), score.findWinTeam());


요 부분에 대해서는 View를 어떻게 분리할지 고민중인데 스티치가 조언을 주셨으면 좋겠어요 T.T
Score를 반환하는 메서드를 하나 생성해야 할까요?
그렇다면 Status 클래스를 위해 모든 객체에서 Score 반환하는 메서드가 필요하게 되는데 설계가 잘못된걸까요?
View를 분리하다보니 상태패턴을 적용하지 않은 초기 코드가 더 낫다고 생각이 드네요 🤔

현재 코드는 run 메서드 내에서 모든 작업을 다 완료하려고 하고 있어요!! State의 메서드를 2개로 나눠보면 어떨까요? 상태를 변경하는 메서드 1개, 그리고 해당 상태에서 처리되어야 하는 커멘드를 입력받는 메서드 1개 이렇게요. 물론 이렇게 처리하더라도 Status의 경우에는 추가적인 메서드가 필요할 것 같아요!! Game에서 Status일 때 분기로 추가적인 처리를 둔다거나 board를 받아서 Score를 생성하는 로직을 밖으로 뺄 수도 있을 것 같아요!!

lxxjn0

연로그, 리뷰를 잘 반영해주었고 기존 코드에 비해서도 훨씬 더 깔끔하게 개선된 것 같아요!!

몇몇 피드백을 남겨두었는데 해당 피드백은 다음 스텝에서 반영하여서 리뷰 요청해주시면 될 것 같아요!! :) 리뷰 반영하느라 수고 많으셨습니다 👍🏼

lxxjn0 · 2022-03-31T14:25:47Z

src/main/java/chess/console/ChessGame.java


 public class ChessGame {

-    private final Board board = new Board();
+    private final BoardGenerator boardGenerator;


객체에서의 필드는 객체의 존재 의미를 나타냅니다. 자동차 게임에서 Car는 자동차의 위치와 이름을 관리해야 하는 책임이 있기 때문에 두 값을 필드로 관리하고 유지하였죠. 그런 의미에서 ChessGame이 과연 Generator를 가져야만 할까요? generator를 관리해야 하는 책임을 가지는게 맞을까요??

현재 코드에서 Generator는 init() 메서드에 넘겨주더라도 충분히 로직이 동작될 것으로 보여요!! 필드는 최대한 적게, 만약 반드시 필드로 관리되어야 하는 값이 아니라면 아예 필드를 없애는 것이 좋습니다! 객체가 상태를 가진다는 것은 관리하는 데 포인트를 만들기 때문이죠!! 그런 관점에서라도 Generator는 해당 객체가 정말 관리해야만 하는 값인지 물어본다면 아닐 것 같아요!! 연로그도 한번 해당 필드에 대해서 확인해보면 좋을 것 같아요!! :)

lxxjn0 · 2022-03-31T14:28:55Z

src/main/java/chess/console/ChessGame.java

+        Board board = boardGenerator.create();
+        State state = new Ready(board);

        while (!state.isEnd()) {
-            if (check()) {
-                OutputView.printCheck();
-            }
+            check(board);


자바에서는 객체를 참조할 수 있기 때문에 현재 board에 대한 참조를 유지하고 있으면 State 내에서 Board의 변경이 일어나도 계속 추적할 수 있는 것은 맞습니다!! 하지만 이러한 형태의 코드는 명확한 이해를 하기엔 어려울 것 같아요!!

참조를 활용할 수도 있지만 그보다는 가독성이 좋게, 그리고 의도를 명확히 드러낼 수 있는 코드가 더 좋을 것 같아요!! 그런 맥락에서 기존에 State에 주입해 준 board의 참조를 사용하기 보다는 State의 board를 사용하는 시점에 State에게 board를 반환하도록 하고 반환된 board를 로직에서 사용해보면 어떨까요? 그렇게 사용한다면 기존의 코드에서 어떠한 점들이 달라질 수 있을지 한번 비교해보는 것도 좋을 것 같아요!! :)

lxxjn0 · 2022-03-31T14:31:11Z

src/main/java/chess/domain/state/Ready.java

+
+public class Ready implements State {
+
+    private boolean isStart = false;


Ready에서만 해당 값을 필드로 관리하고 있는데 이유가 따로 있을까요?

아래 run 함수에서 갱신을 하는군요!! 그런데 run을 하는 순간 running 상태를 반환하면서 ready의 isStart 필드에 대한 접근이 일어나지 않을 것 같은데 혹시 다른 사용 용도가 있을까요? 로직상에서는 해당 필드가 없어도 될 것 같아서요!!

lxxjn0 · 2022-03-31T14:35:45Z

src/main/java/chess/domain/board/generator/BasicBoardGenerator.java

@@ -15,13 +16,14 @@
 import java.util.Map;

 public class BasicBoardGenerator implements BoardGenerator {


해당 클래스와 인터페이스의 패키지를 Board랑 같게 두고 Board 생성자를 package-private으로 두는 것은 어떨까요?? 그렇게 한다면 보드의 생성도 패키지 레벨로 한정할 수 있어서 팩토리 클래스의 목적을 더 명확히 보여줄 수 있을 것 같아요!!

추가로 Generator에서는 메서드를 호출하는 시점마다 새로운 보드를 만들어야 합니다!! 현재 필드로 board를 저장해서 관리한다면 2번째 보드를 생성하는 시점에는 Board가 원하는 대로 생성이 되지 않을 것 같아요!!

lxxjn0 · 2022-03-31T14:37:11Z

src/main/java/chess/domain/position/Column.java

+    public int calculateDifference(Column column) {
+        return this.index - column.index;
    }


메서드 이름이 기존에 비해 훨씬 더 명확하고 이해가 잘 가는 것 같아요 👍🏼

lxxjn0 · 2022-03-31T14:44:29Z

src/main/java/chess/domain/state/State.java

+
+public interface State {
+
+    boolean isStart();


start라는 상태가 실제로 관리되고 있지 않아서 해당 메서드의 의미가 조금 명확하지 않은 것 같아요!! 전체적으로 running 상태에서만 해당 메서드가 true로 사용되는 것 같아서 이름을 isRunning으로 바꿔보는 것은 어떨까요?

lxxjn0 · 2022-03-31T15:18:28Z

src/main/java/chess/console/ChessGame.java

+    private void check(Board board) {
+        try {
+            if (!board.isEmpty() && board.check()) {
+                OutputView.printCheck();
+            }
+        } catch (IllegalArgumentException e) {
+            OutputView.printErrorMessage(e.getMessage());
+        }
+    }


코드가 2 depth인데 한번 확인 부탁드려요 :)

lxxjn0 · 2022-03-31T15:19:35Z

src/main/java/chess/domain/board/Board.java

+    public boolean check() {
+        Position to = findKingPosition(turn);
+        return checkAnyMatch(to);
+    }
+
+    public boolean checkmate() {
+        Position kingPosition = findKingPosition(turn);
+        List<Position> kingPaths = board.get(kingPosition).findMovablePosition(kingPosition);
+
+        return kingPaths.stream()
+                .allMatch(this::checkAnyMatch);
+    }


boolean의 메서드 명을 한번 더 고민해보면 좋을 것 같아요!! :)

lxxjn0 · 2022-03-31T15:20:56Z

src/main/java/chess/domain/state/Move.java

+    @Override
+    public State run(List<String> inputs) {
+        if (board.isEmpty()) {
+            throw new IllegalStateException("체스 게임을 시작해야 합니다.");
+        }
+
+        board.move(Position.of(inputs.get(FROM_POSITION_INDEX)), Position.of(inputs.get(TO_POSITION_INDEX)));
+        return new Running(board);
+    }


여기서도 InputView의 상수를 의존하고 있어요!!

lxxjn0 · 2022-03-31T15:42:58Z

src/main/java/chess/domain/state/Status.java

+    @Override
+    public State run(List<String> inputs) {
+        Score score = board.createResult();
+        OutputView.printStatus(score.getValue(), score.findWinTeam());


현재 코드는 run 메서드 내에서 모든 작업을 다 완료하려고 하고 있어요!! State의 메서드를 2개로 나눠보면 어떨까요? 상태를 변경하는 메서드 1개, 그리고 해당 상태에서 처리되어야 하는 커멘드를 입력받는 메서드 1개 이렇게요. 물론 이렇게 처리하더라도 Status의 경우에는 추가적인 메서드가 필요할 것 같아요!! Game에서 Status일 때 분기로 추가적인 처리를 둔다거나 board를 받아서 Score를 생성하는 로직을 밖으로 뺄 수도 있을 것 같아요!!

her0807 and others added 30 commits March 22, 2022 17:16

docs: 체스 1단계 요구사항 도출

00a2566

feat: Column 도메인을 생성한다.

fb22530

feat: Row 도메인을 생성한다.

eda82b8

refactor: contains -> of 로 변경

15f0a28

feat: Position 도메인을 생성한다.

73f2426

faet: 체스판 초기 세팅 기능 추가

803a3fd

feat: 보드 출력

bf322c1

feat: 체스 시작 가이드 메세지 출력 기능 추가

11d4ecf

feat: 명령어 입력 로직 추가

e6a289f

refactor: 명령어 담당 클래스명 변경

af919ea

feat: ChessGame 시작하는 로직 추가

54a6fa6

refactor: 입력값 MOVE 추가

f509c2c

refactor: 생성자 추가

20ae56a

test: 테스트 케이스 추가

c042866

feat: ChessGame에 move 기능 구현

dd0a2c4

feat: Piece 생성 및 King 이동 가능 여부 구현

b75e53a

feat: Rook movable 기능 구현

88c08fe

feat: Bishop 이동 가능 여부 확인

3544c46

refactor: 불필요한 메서드 제거

8833a49

fix: Direction 찾는 메서드 로직 변경

53a2e72

feat: Queen 이동 가능 여부 확인

6ff9e19

feat: Knight 이동 가능 여부 확인

7b912b5

refactor: Team 생성 및 abstract class -> interface 변환

abc3ccc

feat: Pawn 이동 가능 여부 확인

0313946

docs: 2단계 요구사항 + 페어 룰 추가

ac24924

style: test의 폴더 구조 main과 일치하도록 변경

35d4bb5

refactor: pawn 2칸 이동 가능 여부 확인

c03c636

fix: column, row index 값 변경 및 calculateIndex 메서드 변경

8368360

fix: 폰의 첫수 조건 검증 로직 오류 수정

068594c

fix: row <-> column 변경

c540935

yeon-06 added 15 commits March 28, 2022 11:29

refactor: findDirection 메서드에 boolean 값을 넘기지 않도록 메서드 분리

c593dd1

refactor: 매직 넘버 제거 및 메서드 설명 추가

58a10f3

refactor: 매직 넘버, 하드 코딩 제거

28bd09b

refactor: 메시지 출력에 관한 책임은 View가 갖도록 함

ae7752f

test: 테스트 케이스 깨지는 현상 수정

81cd1d1

fix: 실행 하자마자 종료되는 현상 수정. 체크 확인 로직 위치 변경

55de8c2

refactor: 정적 팩토리 메서드 도입

d144983

test: 테스트 케이스 수정 및 추가

12c76c0

refactor: 입력값 String[]을 반환하도록 수정

2704494

style: generator 한 폴더로 묶음

6f2f90c

refactor: 메서드 분리

7d1951c

refactor: 클래스 이름 변경 Menu -> Command

aae0827

feat: mysql 연동

2075317

refactor: 명령어 입력 부분에 상태 패턴 적용

90d822f

refactor: 매직 넘버 제거

96893fe

lxxjn0 requested changes Mar 29, 2022

View reviewed changes

yeon-06 added 11 commits March 30, 2022 19:18

refactor: 내부에서만 사용하는 메서드 접근제어자 변경

a5521fb

refactor: generator 외부에서 주입

669b4fb

refactor: 사용자 입력 값 반환 타입 배열 -> 리스트로 변경

1415e64

refactor: State와 View 분리

3f16a02

refactor: State 구체화의 선언은 Reay가 될 수 있도록 이외의 클래스들은 접근제어 최소화

3d81369

refactor: 불필요하게 equalsIgnoreCase 호출하는 현상 제거

dfc1aa9

refactor: generator에서 Board 생성

2693c3c

refactor: Team 도메인과 OuputView 뷰 분리

33e1301

refactor: calculateIndex -> calculateDifference 메서드 이름 변경

5de849f

refactor: 매직넘버 제거

cfa0460

refactor: state 폴더 위치 변경

7da8ae2

yeon-06 commented Mar 30, 2022

View reviewed changes

lxxjn0 approved these changes Mar 31, 2022

View reviewed changes

lxxjn0 merged commit 90fefcc into woowacourse:yeon-06 Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1, 2, 3단계 - 체스] 연로그(권시연) 미션 제출합니다. #297

[1, 2, 3단계 - 체스] 연로그(권시연) 미션 제출합니다. #297

yeon-06 commented Mar 28, 2022 •

edited

lxxjn0 left a comment

yeon-06 Mar 30, 2022 •

edited

lxxjn0 Mar 31, 2022

lxxjn0 left a comment

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022

lxxjn0 Mar 31, 2022


		public class Ready implements State {

		private boolean isStart = false;

		@@ -15,13 +16,14 @@
		import java.util.Map;

		public class BasicBoardGenerator implements BoardGenerator {

[1, 2, 3단계 - 체스] 연로그(권시연) 미션 제출합니다. #297

[1, 2, 3단계 - 체스] 연로그(권시연) 미션 제출합니다. #297

Conversation

yeon-06 commented Mar 28, 2022 • edited

구현 기능

리팩터링 예정

lxxjn0 left a comment

Choose a reason for hiding this comment

yeon-06 Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxxjn0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeon-06 commented Mar 28, 2022 •

edited

yeon-06 Mar 30, 2022 •

edited