[1, 2단계 - 체스] 망고(고재철) 미션 제출합니다. #437

Go-Jaecheol · 2023-03-16T08:04:26Z

안녕하세요 파즈!
체스 미션 - 1단계, 2단계 PR 제출합니다 🙇‍♂️

이번 미션에서는 다음과 같은 구조로 구현해보았습니다.

Piece가 각각 PiecePosition과 Color를 가지고, ChessBoard가 Piece들을 가지도록 했습니다.
상태 패턴을 적용해서 전체 체스 게임의 진행 상황을 State로 가지도록 했고, 이 State에 따라 턴을 바꿔가며 게임을 진행하도록 했습니다.
각각 기물들은 Piece 추상 클래스를 상속하는 방식으로 구현했습니다.

주요 클래스 구조

classDiagram
  class ChessBoard {
    -List~Piece~ pieces
  }
  
  class Piece
  <<Abstract>> Piece
  
  class Color
  <<Enumeration>> Color
  
  class Direction
  <<Enumeration>> Direction
  
  ChessController ..> ChessBoard
  ChessController ..> ChessState
  
  ChessBoard --> Piece
  ChessBoard ..> PiecePosition
  ChessBoard ..> WayPoints
  ChessBoard ..> Turn
  
  Piece --> Color
  Piece --> PiecePosition
  Piece ..> Path
는
  
  Path --> PiecePosition
  WayPoints --> PiecePosition
  
  PiecePosition --> Rank
  PiecePosition --> File
  PiecePosition ..> Direction

Piece 추상 클래스

classDiagram
  class Piece
  <<Abstract>> Piece
  
  Piece <|-- King
  Piece <|-- Queen
  Piece <|-- Bishop
  Piece <|-- Rook
  Piece <|-- Knight
  Piece <|-- Pawn

상태 패턴

classDiagram
  class ChessState
  <<Interface>> ChessState
  
  class AbstractChessState
  <<Abstract>> AbstractChessState
  
  ChessState ..> Command
  ChessState <|.. AbstractChessState
  AbstractChessState <|-- Initialize
  AbstractChessState <|-- Running
  AbstractChessState <|-- End
  Command --> Type

추가로 궁금한 점은 코멘트로 남겨둘게요!
아직 부족하지만 잘 부탁드립니다 :)

감사합니다!

Go-Jaecheol · 2023-03-16T08:11:36Z

src/test/java/chess/domain/piece/position/PiecePositionTest.java

+    /**
+     * File 에서도 동일한 테스트가 존재하는데 이곳에도 있어야 할까요?
+     * 둘 중 하나가 없어도 된다면, 둘 중 어느 부분의 테스트를 지워야 할까요?
+     * <p>
+     * TDD 로 구현하다 보면 FileTest 에 존재하게 되겠지만, 상위 계층의 동작이 하위 계층의 구현에 관계없이
+     * 올바르게 작동함을 확인하기 위해서는  PiecePositionTest 에 있어야 한다고 생각하는데, 리뷰어님의 생각이 궁금합니다.
+     */
+    @ParameterizedTest(name = "File 사이의 간격을 구할 수 있다. 현재[{0}]인 경우 목적지인 [{1}] 과의 차이는 [{2}] 이다.")
+    @CsvSource({
+            "b,a,-1",
+            "c,a,-2",
+            "a,c,2",
+            "a,b,1",
+            "a,a,0",
+            "c,c,0",
+    })
+    void File_사이의_간격을_구할_수_있다(final char currentFile, final char destination, final int distance) {
+        // given
+        final PiecePosition from = PiecePosition.of(1, currentFile);
+        final PiecePosition dest = PiecePosition.of(2, destination);


해당 테스트는 PiecePosition 클래스에 있는 fileDistance 메서드를 테스트 하기 위한 테스트 코드입니다.

fileDistance 메서드는 File 클래스에 있는 interval 메서드 호출하는 기능만 하고 있습니다.

FileTest에서 interval 메서드의 테스트는 이미 했고, 이런 경우에 상위 클래스인 PiecePosition에서도 같은 테스트 코드가 필요한지 궁금합니다!

상위에서 수행되고 있으니 사실 테스트코드가 필요없어도 되긴하지만, PieceCondition의 fileDistance에서 File의 interval 메서드 호출 전에 다른 로직이 추가될 경우도 있을 수 있기 때문에 존재한다면 좋긴할 것 같네요. 물론 현재 미션에서는 크게 의미가 없을 것 같긴 합니다!

Be-poz

안녕하세요 망고, 구현해주신 내용 잘 봤습니다. 코드가 굉장히 깔끔해서 크게 손댈 부분이 없는 것 같네요!
코멘트 남겼으니 확인해주시면 감사하겠습니다.

Be-poz · 2023-03-18T01:06:19Z

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

+        createExcludePawn(1, Color.WHITE);
+        createExcludePawn(8, Color.BLACK);


맨 처음 해당 코드를 보고 exclusive 한 pawn을 만드는 걸까? 하고 받아들였네요.
폰을 제외한 piece를 만드는 것이니깐 piece 에 대한 정보도 이름안에 들어가있으면 더 이해하기 좋을 것 같습니다!

폰을 제외한 piece 생성이라는 의미를 더 명확하게 나타내기 위해 createExcludePawn -> createPieceExcludingPawn으로 메서드 명을 변경했습니다!

Be-poz · 2023-03-18T01:15:39Z

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

+    public static ChessBoard create() {
+        return ChessBoard.from(pieces.stream()
+                .map(Piece::clone)
+                .collect(Collectors.toList()));
+    }


Be-poz · 2023-03-18T01:23:00Z

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

+    private final List<Piece> pieces;
+
+    private ChessBoard(final List<Piece> pieces) {
+        this.pieces = pieces;


방어적 복사가 되었으면 좋겠습니다.

방어적 복사를 하도록 변경했습니다!

Be-poz · 2023-03-18T01:25:14Z

src/main/java/chess/domain/piece/position/PiecePosition.java

+    private final Rank rank;
+    private final File file;


이건 취향이고 정말 사소한거긴한데 생성자에서도 그렇고 File, Rank 순으로 오는게 조금 더 보기 편하지 않을까? 라는 생각이 들었습니다.
움직이는 것 또한 b5 이런 식으로 file rank 순이니깐요! Direction 내부에서도 File이 우선적이기도 하구요.

확실히 이 부분도 기준을 정하고 통일해주는 게 좋을 것 같네요.
입력 형식처럼 File, Rank 순서로 변경했습니다!

Be-poz · 2023-03-18T01:50:55Z

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

+    private Running movePiece(final Command command) {
+        final List<String> parameters = command.parameters();
+        final PiecePosition from = PiecePosition.of(parameters.get(FROM_POSITION_INDEX));
+        final PiecePosition to = PiecePosition.of(parameters.get(TO_POSITION_INDEX));
+        chessBoard.movePiece(turn, from, to);
+        return new Running(chessBoard, turn.change());
+    }


상태가 ChessBoard를 들고 있어야 하는 이유를 잘 모르겠습니다.
Running 이라는 상태가 기물을 옮기는 것에 대한 일에 관여하는 것 또한 상태의 역할이 맞는지 의문이 듭니다.
상태는 말 그대로 상태 그 자체만의 의미로 두고 체스 보드와 관련된 것들은 상태를 참조하는 외부에서 상태를 보고 판단하여 움직이게끔 하면 좋을 것 같습니다.

이 부분이 사실상 체스의 핵심 로직 중 하나여서 고민이 굉장히 많았을 것 같습니다. 망고는 어떤 생각들을 했는지 듣고 싶습니다.

기물을 움직이는 기능은 Running 상태일 때만 가능하기 때문에 Running 클래스 내부에서 움직인 후, Turn을 변경해서 새로운 Running 상태로 변경해야겠다고 생각했습니다.

하지만 위 피드백을 보니 상태는 상태만 나타내고 외부에서 상태를 보고 움직이는 기능을 하는 게 맞는 것 같습니다!

그래서 ChessState 구조를 다음과 같이 변경하고, Controller에서도 state를 확인한 후 기물을 이동하고 상태를 변경하도록 수정했습니다.

classDiagram class ChessState <<Interface>> ChessState class Runnable <<Abstract>> Runnable class Finished <<Abstract>> Finished ChessState <|.. Runnable ChessState <|.. Finished Runnable <|-- Initialize Runnable <|-- Run Finished <|-- End

Loading

하지만 이렇게 바꾸고 보니 전보다 Controller의 구조가 약간 복잡해진 것 같고, 상태 패턴을 제대로 사용하고 있는 건지 잘 모르겠습니다.
지금보다 더 좋은 방법이 있을까요?? 😢

저는 괜찮은 것 같습니다! 깔끔한 것 같은데요 !?👍👍👍

Be-poz · 2023-03-18T01:54:32Z

src/main/java/chess/domain/piece/position/PiecePosition.java

+    private PiecePosition(final Rank rank, final File file) {
+        this.rank = rank;
+        this.file = file;
+    }
+
+    public static PiecePosition of(final int rank, final char file) {
+        return new PiecePosition(Rank.from(rank), File.from(file));
+    }
+
+    public static PiecePosition of(final Rank rank, final File file) {
+        return new PiecePosition(rank, file);
+    }


여러 클래스들에서 정적 팩토리 메서드를 사용하고 있는 것을 확인했습니다. 일부 클래스에서는 생성자를 사용하고 있었고요.
어떤 기준을 두고 구분했는지 궁금합니다!

피드백을 보고 생각해보니 생성자랑 정적 팩토리 메서드 사용에 대한 저만의 기준이 없었네요..

필드와 파라미터에 차이가 있는 경우, 일반적인 생성자 기능 외에 추가적인 작업이 필요한 경우에 정적 팩토리 메서드를 사용하고
이 외에는 생성자를 사용하도록 새로운 기준을 정했고 반영했습니다!

Be-poz · 2023-03-18T02:05:27Z

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

+    @Override
+    public boolean equals(final Object o) {
+        if (this == o) return true;
+        if (!(o instanceof Turn)) return false;
+        final Turn turn1 = (Turn) o;
+        return turn == turn1.turn;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(turn);
+    }
+
+    public boolean incorrect(final Color color) {
+        return turn != color;
+    }


incorrect 앞에 is 가 붙으면 좋을 것 같습니다.
그리고 위치 또한 오버라이드한 equals, hashCode 위 쪽에 위치하는 것이 좋을 것 같습니다.

변경했습니다!

Be-poz · 2023-03-18T02:26:07Z

src/main/java/chess/domain/piece/Piece.java

+    }
+
+    private void validateKill(final Piece enemy) {
+        if (!isEnemy(enemy)) {


부정연산자 라는 기능이 있긴하지만 가독성을 굉장히 떨어트리고 헷갈리게 할 수 있는 연산자입니다.
해당 코드 뿐만 아니라 다른 코드에서도 반대의 메서드를 추가해서 부정 연산자를 깔끔하게 덜어낼 수 있다면 리팩토링 하면 좋을 것 같습니다. 애매하다 생각되면 놔두셔도 됩니다!

해당 코드에서 isEnemy 메서드는 부정 연산자를 사용하지 않도록 isAlly 메서드로 변경했습니다!

나머지 대각선이나 직선 경로인지 확인하는 부분은 오히려 더 복잡하다는 생각이 들어서 놔두었습니다.

Be-poz · 2023-03-18T02:31:19Z

src/test/java/chess/domain/piece/position/PiecePositionTest.java

+    /**
+     * File 에서도 동일한 테스트가 존재하는데 이곳에도 있어야 할까요?
+     * 둘 중 하나가 없어도 된다면, 둘 중 어느 부분의 테스트를 지워야 할까요?
+     * <p>
+     * TDD 로 구현하다 보면 FileTest 에 존재하게 되겠지만, 상위 계층의 동작이 하위 계층의 구현에 관계없이
+     * 올바르게 작동함을 확인하기 위해서는  PiecePositionTest 에 있어야 한다고 생각하는데, 리뷰어님의 생각이 궁금합니다.
+     */
+    @ParameterizedTest(name = "File 사이의 간격을 구할 수 있다. 현재[{0}]인 경우 목적지인 [{1}] 과의 차이는 [{2}] 이다.")
+    @CsvSource({
+            "b,a,-1",
+            "c,a,-2",
+            "a,c,2",
+            "a,b,1",
+            "a,a,0",
+            "c,c,0",
+    })
+    void File_사이의_간격을_구할_수_있다(final char currentFile, final char destination, final int distance) {
+        // given
+        final PiecePosition from = PiecePosition.of(1, currentFile);
+        final PiecePosition dest = PiecePosition.of(2, destination);


상위에서 수행되고 있으니 사실 테스트코드가 필요없어도 되긴하지만, PieceCondition의 fileDistance에서 File의 interval 메서드 호출 전에 다른 로직이 추가될 경우도 있을 수 있기 때문에 존재한다면 좋긴할 것 같네요. 물론 현재 미션에서는 크게 의미가 없을 것 같긴 합니다!

Be-poz · 2023-03-18T02:35:51Z

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

+    }
+
+    public List<Piece> pieces() {
+        return pieces.stream().map(Piece::clone)


한 줄에 점을 하나만 찍는다. 개행 고고

Be-poz · 2023-03-18T02:39:05Z

src/main/java/chess/ChessApplication.java

+
+import chess.controller.ChessController;
+
+public class ChessApplication {


게터/세터/프로퍼티를 쓰지 않는다. 추가 요구 사항을 리뷰가 마친 후 확인했는데요~
프로퍼티를 .getField 형식이 아니라 .field 형식으로 참조한 것 들도 게터라고 판단하고 리팩토링 진행해주시면 감사하겠습니다.

출력과 테스트를 위한 getter는 존재해도 되나요??
그리고 List<String>을 필드로 가지고 있는 클래스에서 리스트 내 특정 위치의 String 값만 반환하는 메서드로 바꾸는 것도 여전히 getter로 판단되어 지양해야 하는지 궁금합니다.
또한, getter 사용을 피하면서 도메인이 서로 의존하게 된다면 이럴 땐 어떻게 하는 게 좋을까요..?

getter 사용 지양하기.. 너무 어렵습니다 😢

사실 getter 사용을 지양하라는 것은 알고 계시겠지만 객체에 역할을 부여할 수 있으면 해당 객체 내부에 역할을 주고 해당 메서드를 콜해서 사용해라~ getter를 통해서 값만 들고와서 바깥에서 처리하는 방식을 최대한 하지마라~~~ 라는 의미인데요.
이렇나 관점에서 각 객체의 필드를 가지고 오는 것을 getter 그 이외의 비즈니스 로직을 거친 필드가 아닌 다른 값을 내뱉는 것은 getter라고 보기 힘들죠. 하지만 getter가 필요한 경우도 당연 있습니다! 이럴 때에는 어쩔 수 없는거죠.

그냥 요구사항의 의미는 객체지향적으로 최대한 해라~ 이니 남은 체스 미션에서는 망고가 이 선을 어느정도 지키면서 융통성 있게 getter를 사용하시면 될 것 같습니다. view 클래스의 보드 현 상황을 출력하는 메서드를 호출을 할 때에 ChessBoard의 piece를 그냥 getter를 통해서 가지고 와서 파라미터로 보내고 해당 클래스에서 조립을 하는 방식도 있을 수 있겠지만, 요구사항을 따른다면 ChessBoard 내부에 보드현황에 대한 String을 만드는 메서드를 만들어서 호출하는 식으로 할 수도 있겠죠!

망고가 미션을 진행하면서 아 이거는 getter를 그냥 쓰는게 진짜 맞는 것 같은데? 오히려 이걸 안쓰면 더 복잡해지겠는데? 싶으면 그냥 사용하시면 될 것 같습니다! 😆

Be-poz

안녕하세요 망고! 피드백 반영해주신 내용들 다 확인했습니다.
커밋별로 보고 이후 전체 파일로 다시 봤는데 이번 단계에서는 더 이상 크게 손댈 곳이 없는 것 같습니다.
1,2단계 수고하셨습니다. 그 다음 미션에서 다시 뵙도록 하겠습니다. 수고하셨어요!

shin-mallang added 30 commits March 14, 2023 14:32

docs: 기능 명세 문서 작성

f819e70

feat: 체스 기물 생성

8632943

feat: 체스 기물에 색상 추가

ebfe4a6

feat: 체스 위치 추가

efc16dd

refactor: Piece에서 PiecePosition 사용

490bd32

refactor: Piece 의 역할을 enum으로 관리

8f786e1

feat: ChessBoard 생성

d938d00

feat: 체스판 뷰 구현

b1b56a2

refactor: Piece를 추상 클래스를 통해 구현하도록 변경

d0674d0

feat: 1단계 뷰 구현 완료

78a2104

feat: 방위를 나타내는 Direction 구현

d6cfaa0

feat: PiecePosition에 기능 추가

03cea30

feat: Path 구현

2ecd841

feat: Piece 에 경로 탐색을 위한 템플릿 메서드 작성

04a9fe7

feat: King 의 이동 조건 탐색 기능 구현

ff4781a

feat: Bishop 의 이동 조건 탐색 기능 구현

4cc1eb6

feat: Rook 의 이동 조건 탐색 기능 구현

44335df

feat: Queen 의 이동 조건 탐색 기능 구현

c9e4051

feat: Pawn 의 이동 조건 탐색 기능 구현

d50d83d

feat: Knight 의 이동 조건 탐색 기능 구현

585e209

refactor: path -> wayPoints 단어 변경

db7727e

fix: Pawn 움직일 수 없는데 움직여지는 버그 수정

aba299f

feat: 체스 보드에서 말을 움직이는 기능 구현

8d4c42a

refactor: IMPOSSIBLE Condition 제거

a49ad60

refactor: Condition 완전히 제거

2debf96

refactor: ChessBoard Polishing

1e0b005

feat: 체스 게임 구현

0628e86

refactor: Command 분리

60044eb

fix: 예외 메세지 수정

21e9aba

refactor: 매직 넘버 상수로 관리

8f8675f

shin-mallang and others added 4 commits March 16, 2023 14:24

refactor: ChessBoard 생성을 Factory에서 담당

f8e2753

docs: 2단계 기능 목록 정리

f92e75f

refactor: 사용하지 않는 import문 삭제

b371786

docs: 클래스 다이어그램 추가

bc18b37

Go-Jaecheol commented Mar 16, 2023

View reviewed changes

Be-poz requested changes Mar 18, 2023

View reviewed changes

Be-poz reviewed Mar 18, 2023

View reviewed changes

Go-Jaecheol added 13 commits March 22, 2023 14:21

mission: [미니 미션] Lambda, Stream 이해하기 - 미션 1 구현

ad46fa0

mission: [미니 미션] Lambda, Stream 이해하기 - 미션 2 구현

2c52ec8

mission: [미니 미션] Lambda, Stream 이해하기 - 미션 3 구현

b5c5790

refactor: createExcludePawn 메서드 명 변경

6201740

refactor: 생성자에서 방어적 복사를 하도록 변경

37b99b4

refactor: PiecePosition에서 File, Rank 순서 변경

fdf03d0

refactor: Turn을 확인하는 incorrect 메서드 명 변경 및 메서드 순서 변경

ed024b5

style: 한 줄에 점을 하나만 찍도록 수정

c95316d

refactor: ChessState에서 기물을 직접 움직이지 않도록 변경

b2f7bd6

refactor: isEnemy 메서드 부정 연산자 사용하지 않도록 변경

eccf422

refactor: 필드와 파라미터가 같으면 정적 팩토리 메서드 대신 생성자를 사용하도록 수정

2a06db3

refactor: getter 사용 지양하도록 수정

48ea72c

docs: 1, 2단계 리팩토링 요구 사항 정리

571ba92

Be-poz approved these changes Mar 23, 2023

View reviewed changes

Be-poz merged commit ebbbe75 into woowacourse:go-jaecheol Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1, 2단계 - 체스] 망고(고재철) 미션 제출합니다. #437

[1, 2단계 - 체스] 망고(고재철) 미션 제출합니다. #437

Go-Jaecheol commented Mar 16, 2023

Go-Jaecheol Mar 16, 2023

Be-poz Mar 18, 2023

Be-poz left a comment

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023

Be-poz Mar 18, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023

Be-poz Mar 23, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023 •

edited

Loading

Be-poz Mar 18, 2023

Be-poz Mar 18, 2023

Be-poz Mar 18, 2023

Go-Jaecheol Mar 22, 2023 •

edited

Loading

Be-poz Mar 23, 2023

Be-poz left a comment

		createExcludePawn(1, Color.WHITE);
		createExcludePawn(8, Color.BLACK);


		import chess.controller.ChessController;

		public class ChessApplication {

[1, 2단계 - 체스] 망고(고재철) 미션 제출합니다. #437

[1, 2단계 - 체스] 망고(고재철) 미션 제출합니다. #437

Conversation

Go-Jaecheol commented Mar 16, 2023

주요 클래스 구조

Piece 추상 클래스

상태 패턴

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-poz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Go-Jaecheol Mar 22, 2023 • edited Loading

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

Go-Jaecheol Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-poz left a comment

Choose a reason for hiding this comment

Go-Jaecheol Mar 22, 2023 •

edited

Loading

Go-Jaecheol Mar 22, 2023 •

edited

Loading