[1, 2단계 - 체스] 성하(김성훈) 미션 제출합니다. #517

sh111-coder · 2023-03-17T08:49:37Z

안녕하세요! 제이온!
백엔드 5기 크루 성하입니다!!

이번 미션은 너무 설계 고민이 많아서 미션이 하루 늦춰졌음에도 만족스럽지 못한 코드가 나온 것 같아요.

그래서 급하게 설계 및 구현하느라 고민해야 할 점들을 많이 못 적어놔서 아쉬운 것 같습니다 ㅠㅠ 😭

원래 목표는 다이어그램 이미지도 만들고, 주요 설계점들을 적어 놓으려고 했는데
그런 부분은 추후에 리팩토링 시에 추가해보도록 하겠습니다.

질문할 부분은 Piece 부분에서 추상 클래스의 인스턴스화가 불가능해서 생긴 문제가 있었어서
instanceof를 사용했는데, 해당 부분은 코드에 코멘트로 남겨두도록 하겠습니다!

설계 시 오랜 시간 고민했던 점이 있어서 아래에 질문 남기겠습니다!

🎯 Position을 어느 객체가 알게 할 것인가?

처음에 Position을 Board가 알게 할지, Piece가 알게 할지 고민이었습니다.
저는 객체가 자율성을 가져야 한다고 생각하여 Piece가 Position을 알고 이동하는 객체로 설계하였습니다.

Board가 Position을 알게 되면 Piece는 진영만 가지는 빈 껍데기 객체가 되는 것 같았습니다.
이렇게 하다보니 Board가 64칸의 실제 보드가 아닌, List<Piece>로 Piece들을 가지는 주머니 느낌이 되어
이상하다는 느낌을 받았습니다.

이러한 근거를 가지고 설계한 저의 생각이 괜찮은 건지 궁금합니다!

이동 기능 매개변수로 Rank,File 별 이동 횟수를 전달받도록 수정

모든 기물이 Piece를 상속 받았었는데, Pawn/Knight : SpecialPiece 상속 King/Queen/Rook/Bishop : NormalPiece 상속

sh111-coder · 2023-03-18T19:38:59Z

안녕하세요 제이온!
꼼꼼하게 리뷰 남겨주셔서 감사합니다!! 😃

피드백 주신 부분들 리팩토링 진행했습니다!

리팩토링 진행 전, 진행 후 제이온이 달아놓으신 코멘트에 제 생각이 담긴 답변 및 질문들을 남겼어요!! 시간 나시면 답변 부탁드립니다!

다음은 이번 리팩토링 진행 시 주요 변화점과 질문할 점을 정리한 것을 나열해보겠습니다!

🎯 이번 리팩토링 주요 변화

1. 테스트 케이스를 @MethodSource로 많이 추가했습니다!

2. `Piece`의 상속 구조 변경

Piece의 상속 구조를 변경하여 중복되는 코드를 줄였는데, 추상화가 너무 복잡해서 오히려 좋지 않을지 모르겠습니다!
그래도 상속 구조를 변경하면서 중복되는 코드가 어떻게 줄었는지 정리해보겠습니다!

상속 다이어그램은 다음과 같습니다.

✅ 2-1. DirectionalPiece 추상 클래스

방향으로 갈 수 있는지 판단하지 않고 가는 경로도 반환하지 않는 Knight를 제외하고,
방향을 가지고 가는 경로를 반환하는 나머지 기물들을 DirectionalPiece로 추상화하였습니다.

DirectionalPiece 추상화를 통해, 각 서브 클래스에 중복되는 필드 List<Direction>을 제거하였습니다.

✅ 2-2. NormalPiece 추상 클래스

기물 진영 및 초기 상태 여부에 따라 가는 경로가 달라지는 Pawn을 제외하고,
기본 규칙을 가지고 이동하는 기물들을 NormalPiece로 추상화하였습니다.

NormalPiece 추상화를 통해, 각 서브 클래스에 중복되는 isPawn() 메소드와 getPaths() 메소드를 제거하였습니다.

✅ 2-3. LongRangePiece 추상 클래스

한 칸씩만 이동할 수 있는 King을 제외하고,
방향에 따라 여러 칸을 이동할 수 있는 나머지 기물들을 LongRangePiece로 추상화하였습니다

LongRangePiece 추상화를 통해, 각 서브 클래스에 중복되는 isMovable()` 메소드를 제거하였습니다.

🎯 리팩토링 시 질문

✅ Turn 구현 부분 질문

ChessGame에서 Turn을 구현하기 위해 Piece의 Side를 출력용으로 사용되는 getter로 가져와서 턴을 체크하고 있습니다.
이렇게 사용해도 되는 건지 궁금합니다!
또, ChessGame에서 Turn의 불변을 보장하려고 하다보니, 매번 턴이 바뀔 때마다 Board도 새로운 Board로 바꿔야 했습니다.
그래서 어쩔 수 없이 Turn의 불변을 해제했는데, Turn의 불변을 보장해야 하는 걸까요?
또, Turn Enum이 Side Enum을 필드로 가지고 있는데, Enum 안에 Enum을 필드로 써본 적이 없어서
Enum 안에 Enum을 필드로 가져도 되는지 궁금합니다!

나머지 리팩토링 질문들은 제이온의 답변 코멘트에 남겼습니다!! 시간나실 때 답변 부탁드려요!! 항상 감사합니다 😃

pjy1368

안녕하세요 성하!
1차 피드백을 잘 반영해 주시고 Piece의 계층을 나누어서 중복 코드를 제거하는 시도는 좋은 연습이 되셨을 것 같습니다 👍
몇 가지 피드백을 남겨뒀으니 반영해 보시고 리뷰 요청 주세요!
PR 본문에 달아두신 질문은 피드백 곳곳에 답변을 해뒀는데, Piece 계층에 관련된 부분만 여기에 별도로 답변드릴게요~

Q. Piece의 상속 구조 변경
기존 Piece의 계층은 2계층이며 한 눈에 코드를 알아보기가 좋았습니다. (중복 코드가 있더라도 말이죠!) 하지만 5계층으로 분리가 되면서 성하께서 사진도 첨부해 주시고 설명까지 달아주신 것을 정독하고 코드도 사진 및 설명과 비교해야 이해가 되었습니다 😥
여기서 알 수 있는 사실은 중복 코드가 있는 것은 불편하다정도지만, 복잡한 추상화를 해 놓으면 코드를 해석하기 어렵다는 것입니다! 한 번 기존 형태로 다시 돌아가 보시고 반드시 계층을 나눠야하는지 고민해 보시면 어떨까합니다.
또한, 지금 코드를 보면 "움직이는 방식"에 따라 Piece에 상속을 받은 체스말들이 있는데, 이렇게 설계가 된다면 Piece는 일반 클래스로 두고 "움직이는 방식"을 인터페이스로 하여 Piece가 필드로 들고 있는 방식이 낫지않을까하는 생각도 드네요 ㅎㅎ 요건 한 번 선택적으로 반영해보세요!

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

pjy1368 · 2023-03-19T05:51:26Z

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

+        return this.position.equals(position);
+    }
+
+    public Piece move(final Position positionToMove) {


저는 비슷한 코드라고는 생각하나 각기 다른 인스턴스를 반환하므로 중복된 코드라고 생각하지는 않았습니다 ㅎㅎ
또한 지금은 move가 인스턴스만 반환하는 로직이지만, 상황에 따라 각 객체가 자유롭게 move()를 정의할 여지를 주기 때문에 더 객체지향적이라고 생각해요~
instanceof는 상위 클래스가 하위 클래스를 명확하게 알고 조작한다는 점도 문제가 되지만, 성능 상에서도 단점이 있습니다. 아래 아티클을 확인해 보세요!
https://tecoble.techcourse.co.kr/post/2021-04-26-instanceof/

pjy1368 · 2023-03-19T06:04:27Z

src/main/java/chess/board/dto/BoardDto.java

+    public BoardDto(final Board board) {
+        this.nameBoard = new ArrayList<>();
+        initBoardSpaces();
+        fillPieceNames(board);
+    }


사실 static과 무관하게 DTO에 로직이 많이 들어가는 건 DTO가 아니라 OutputView나 Controller 단에서 처리할 수 있는 것이 아닌지 고민해 보아야 할 신호이기도 합니다 ㅎㅎ
우선 static을 붙이는 것 자체는 정답을 드리기 보다는 아래 아티클을 소개해 드릴게요!
https://tecoble.techcourse.co.kr/post/2020-07-16-static-method/
https://steady-coding.tistory.com/603

pjy1368 · 2023-03-19T06:05:30Z

src/main/java/chess/board/dto/BoardDto.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class BoardDto {


DTO는 왜 필요한 걸까요?
누가 필요로하길래 이러한 객체로 따로 포장해서 전달하는지 생각해 보시면 해답이 나오지 않을까 합니다~

pjy1368 · 2023-03-19T06:06:14Z

src/main/java/chess/board/dto/Name.java

+public enum Name {
+    KING("k", "K"),
+    QUEEN("q", "Q"),
+    BISHOP("b", "B"),
+    KNIGHT("n", "N"),
+    ROOK("r", "R"),
+    PAWN("p", "P");


아, 이건 성하 의견이 맞습니다. 제가 체스 요구 사항에 대해 조금 착각을 했네요 ㅎㅎ
출력을 위해 사용되는 로직은 도메인이 가지지 않는 것이 좋습니다.

src/main/java/chess/piece/directional/Pawn.java

src/main/java/chess/piece/Direction.java

src/main/java/chess/piece/Knight.java

src/main/java/chess/piece/directional/normal/King.java

sh111-coder · 2023-03-19T16:14:00Z

안녕하세요! 제이온! 2차 리팩토링 진행했습니다 👍

제이온의 1차 피드백 코멘트대로 상속 구조를 변경한 후에 다이어그램을 보고 처음 든 생각은 '복잡하다..' 였어요!
그래서 이전의 추상화 계층이 더 간단하고 좋았다고 생각은 들었지만, 이미 시도했으니 그대로 가보자! 이 생각으로 리팩토링했었습니다!
그래도 해당 구조로 리팩토링 하다보니 중복 코드들을 쏙쏙 뽑아내다보니
제일 하위 계층인 Rook, Bishop, Queen에는 코드가 정말 적어진 것을 보고 뿌듯했습니다!!

계층이 2,3,4 계층 정도이고 묶이는 클래스들이 여러 개라면 정말 효과적으로 중복을 제거할 수 있다는 생각이 들었어요!
이러한 경험을 한 것으로도 만족합니다!! 👍

또한, 지금 코드를 보면 "움직이는 방식"에 따라 Piece에 상속을 받은 체스말들이 있는데, 이렇게 설계가 된다면 Piece는 일반 클래스로 두고 "움직이는 방식"을 인터페이스로 하여 Piece가 필드로 들고 있는 방식이 낫지않을까하는 생각도 드네요 ㅎㅎ
요건 한 번 선택적으로 반영해보세요!

이 부분은 시간상 다음에 해보도록 하겠습니다!! 뭔가 할 수 있을 것 같은데 지금 당장은 안되네요 ㅠㅠ

이번 2차 리팩토링도 어떤 것을 리팩토링했는지 나열해보겠습니다!!

🎯 2차 리팩토링 변경점

1. 상속 구조 원래대로 변경

앞에서 말했듯이, 기존 상속 구조로 재변경하였습니다!

2. `Board`의 Piece 사용 시 추상화 계층 지키기

instanceof를 지양하는 이유를 주제로 프롤로그를 적다가, 한 크루의 질문을 받고 메소드 네이밍을 수정하게 되었습니다!

public class Board {

    public void movePiece(...) {
        ...
        // 수정 전 코드
        if (piece.isPawn()) {
            checkPawnDiagonalMove(sourcePosition, targetPosition);
        }
        
        // 수정 후 코드
        if (piece.isNeedToCheckWhenDiagonalMove()) {
            checkPieceDiagonalMove(sourcePosition, targetPosition);
        }
    }
}

Board가 상위 계층인 Piece를 사용하고 있지만
기존 Board의 isPawn()메소드와 checkPawnDiagonalMove()메소드는
메소드 네이밍을 통해 하위 계층인 Pawn을 간접적으로 알 수 있었습니다.

따라서 메소드 네이밍을 하위 계층에 의존하지 않도록 수정하여 Board에서 해당 로직이 Pawn에 대한 로직인지 모르게 했습니다!

제 생각대로 이렇게 변경하게 되었는데, 이렇게 변경하는 것이 좋은 방향인지 궁금합니다!!

3. NameDto, BoardDto 삭제 후 출력용 객체인 NameBoard 구현

제이온의 코멘트를 읽어보고 제가 사용한 DTO는 데이터 전송용 객체가 아닌 것을 느끼게 되었습니다.

그래서 DTO를 삭제하고, 처음에는 컨트롤러에 모든 로직을 넣어 봤습니다.

그렇게 했더니 컨트롤러의 역할이 커지는 것 같고 코드도 길어지게 되어

해당 로직을 따로 빼서 출력용 보드판 객체인 NameBoard를 구현하게 되었습니다.

해당 객체를 Controller에서 생성하고 List<List<String>> 형태로 View에게 전달하고 있는데, 괜찮은 방향일까요?!

pjy1368

안녕하세요 성하!
잘 계층화된 구조에서 원래 구조로 돌아오면서 많은 것을 배우고 느끼셨을 것 같아요~
나머지는 소소한 피드백이거나 다음 단계에서 진행해보면서 반영해도 괜찮은 내용이라 이만 머지할게요 😁

pjy1368 · 2023-03-20T00:33:53Z

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

+    public abstract boolean isMovable(Position targetPosition);
+
+    public boolean isSamePosition(Position position) {
+        return this.position.equals(position);
+    }
+
+    public abstract Piece move(final Position positionToMove);


isMovable과 move를 합쳐보는 건 어려울까요?
느낌상 move 안에 isMovable이 들어가는 것이 적절해 보여서요~!

isMovable() 메소드로 Board가 해당 기물이 갈 수 있는지 판단을 한 후에

해당 기물이 move()로 이동하도록 Board에서 관리하기 때문에 두 가지 행위를 분리했던 것 같아요!!

pjy1368 · 2023-03-20T00:34:43Z

src/main/java/chess/game/Turn.java

+    public boolean isCorrectTurn(Side side) {
+        return this.side == side;
+    }


그렇긴한데, 저는 getter 정도는 유도리있게 열어주는 편이긴 합니다 ㅎㅎ

pjy1368 · 2023-03-20T00:37:10Z

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

+        checkPieceMovable(targetPosition, sourcePiece);
+        checkPath(targetPosition, sourcePiece);
+        checkTargetPosition(targetPosition, sourcePiece);
+        if (sourcePiece.isNeedToCheckWhenDiagonalMove()) {


isPawn()을 썼을 때 계층이 드러나는 것은 맞으나, 체스 게임이라는 규칙상 예외적으로 Pawn이라고 명시했을 때 코드를 읽기 더 좋은 것 같습니다~
실제로 isNeedToCheckWhenDiagonalMove() 메서드를 봤을 때, "Diagonal이 대각선이니 대각선으로 움직이는 말인지 확인하는 건가? 그러면 퀸, 비숍, 폰 등이 있겠네?"라고 오해할 여지가 있더라고요 ㅎㅎ

pjy1368 · 2023-03-20T00:48:24Z

src/main/java/chess/controller/NameBoard.java

+public class NameBoard {
+
+    private static final Map<Class<? extends Piece>, Name> nameByPiece = new HashMap<>();
+    private static final int BOARD_ROW_SIZE = 8;
+
+    private final List<List<String>> nameBoard;
+
+    static {
+        nameByPiece.put(King.class, Name.KING);
+        nameByPiece.put(Queen.class, Name.QUEEN);
+        nameByPiece.put(Bishop.class, Name.BISHOP);
+        nameByPiece.put(Knight.class, Name.KNIGHT);
+        nameByPiece.put(Rook.class, Name.ROOK);
+        nameByPiece.put(Pawn.class, Name.PAWN);
+    }


저도 이 친구는 controller 패키지가 적합해보이네요~
내부 로직 까보면 도메인을 살짝 이용하고 있고, static 블록으로 도메인을 쓰고 있으면서 전체적인 핵심 로직은 View를 위한거니까요!

pjy1368 · 2023-03-20T00:49:42Z

src/main/java/chess/piece/Pawn.java

+    @Override
+    public List<Position> getPaths(final Position targetPosition) {
+        final Direction direction = position.getDirectionTo(targetPosition);
+        final int moveCount = position.getMoveCount(targetPosition, direction);
+
+        if (isStartPosition() && moveCount == INITIAL_MOVE_COUNT) {
+            return List.of(position.getNextPosition(direction));
+        }
+        return Collections.emptyList();
+    }


여기서 발생하는 중복 코드는 다음 단계 진행해보면서 리팩터링해 보면 좋을 것 같습니다~
일단은 이대로 두고 가시죠!

yujamint added 30 commits March 14, 2023 14:34

docs: 기능 목록 작성

f53a46d

test(PositionTest): Position 생성 테스트

5f4eff4

feat(Position): Position 생성

ab6b082

test(PositionTest): Position 이동 기능 테스트

0ee7958

feat(Position): Position 이동 기능 구현

762399f

test: Position 이동 기능 리팩토링

331b298

이동 기능 매개변수로 Rank,File 별 이동 횟수를 전달받도록 수정

refactor: Position 이동 기능 리팩토링

04da08f

이동 기능 매개변수로 Rank,File 별 이동 횟수를 전달받도록 수정

test(ChessGameTest): Piece 생성 테스트

f97f831

feat(ChessGame): Pieces 생성 기능 구현

4ca0034

test(PiecesTest): Pieces 생성 기능 테스트 ChessGameTest -> PiecesTest로 위임

3cbc9f6

feat: 보드판 출력 DTO 구현

449e6f9

refactor(BoardDto): getter 추가

997fe0f

refactor: dto 패키지 이동

128e3f6

feat(OutputView): 게임 시작 메시지 출력 기능 구현

258caec

feat(OutputView): 게임 커맨드 입력 메시지 출력 기능 구현

6536e2a

refactor: Board가 Pieces를 받아서 생성되도록 리팩토링

115e361

feat: 보드판 출력 기능 구현

1571089

feat: 게임 커맨드 입력 기능 구현

8a2ea56

feat(OutputView): 에러 메시지 출력 기능 구현

0baffe7

feat(ChessController): 체스 게임 컨트롤러 구현

9ac0112

feat(Application): 체스 게임 애플리케이션 구현

61b2fe5

docs: 1단계 기능 목록 완료

821f763

style: 보드 출력 시 줄바꿈 추가

49ad1d6

test(PositionTest): 대상 Position으로 향하는 방향 반환 기능 테스트

fecabc5

feat(Position): 대상 Position으로 향하는 방향 반환 기능 구현

dde92f7

test(PositionTest): 두 Position의 Slope 구하는 기능 테스트

c503400

feat(Position): 두 Position의 Slope 구하는 기능 구현

87ab2ee

test(RankTest): 대상 Rank와의 값 차이 반환 기능 테스트

3bfecb4

feat(Rank): 대상 Rank와의 값 차이 반환 기능 구현

6ebb3be

test(RankTest): 대상 Rank와의 값 차이에 따른 수직 좌표 값 반환 기능 테스트

29339dc

sh111-coder added 14 commits March 18, 2023 17:55

refactor: Piece 받은 위치로 이동하는 기능 하위 클래스에서 구현하도록 리팩토링

ba83421

test(PieceTest): 해당 기물이 Pawn인지 확인하는 기능 테스트

d2b58a8

feat(Piece): 해당 기물이 Pawn인지 확인하는 기능 구현

9bf5af5

refactor(Board): Pawn 체크 시 instanceof 대신 Piece에 메시지를 던지도록 리팩토링

f558bbc

refactor(Board): 타겟 위치까지의 경로에 말 존재하는 경우 체크 Stream 사용하여 리팩토링

585bc58

refactor: Piece 상속 구조 변경 리팩토링

eb224ea

모든 기물이 Piece를 상속 받았었는데, Pawn/Knight : SpecialPiece 상속 King/Queen/Rook/Bishop : NormalPiece 상속

remove: SpecialPiece 클래스 삭제

568af11

refactor: 상속 구조 변경 리팩토링

eeb0b85

refactor: BoardDto 정적 팩토리 메소드로 생성하도록 리팩토링

bb27eb1

refactor: Board Size 매직넘버 상수화 리팩토링

91625d1

refactor(InputView): 에러 메시지 커맨드 부분 상수 추가 리팩토링

e610f8f

refactor: 랭크 값이 숫자가 아닌 경우 검증 InputView에 위임

19a5bf9

feat: Turn 구현

93235bb

feat: ChessGame 구현하여 Controller의 책임 분리

d57f64a

pjy1368 requested changes Mar 19, 2023

View reviewed changes

sh111-coder added 11 commits March 19, 2023 21:04

refactor: 추상화가 지켜지도록 메소드 네이밍 변경

55eb743

refactor(ChessController): 조건문 가독성 개선 리팩토링

38f13a3

refactor: Position 생성 로직 Controller, ChessGame에 적절하게 분리

4add086

style: 불필요한 라인 정리

ba6b40f

refactor: 초기 방향 설정 시 인라인 & addAll로 설정하도록 리팩토링

8063247

refactor: 매직 넘버 상수화 리팩토링

ffeeb4b

refactor(Direction): 예외 메시지 추가 리팩토링

d546a82

move: DTO 패키지 Controller로 이동

cf37109

refactor: NameDto, BoardDto 삭제 & 출력용 NameBoard 객체 생성하여 출력하도록 리팩토링

eb7a823

refactor: NameBoard 정적 팩토리 메소드 추가하여 리팩토링

c603b3e

refactor: 기존 상속 구조로 재변경

c434d1b

pjy1368 approved these changes Mar 20, 2023

View reviewed changes

pjy1368 merged commit b3183d8 into woowacourse:sh111-coder Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1, 2단계 - 체스] 성하(김성훈) 미션 제출합니다. #517

[1, 2단계 - 체스] 성하(김성훈) 미션 제출합니다. #517

sh111-coder commented Mar 17, 2023

sh111-coder commented Mar 18, 2023

pjy1368 left a comment

pjy1368 Mar 19, 2023

pjy1368 Mar 19, 2023

pjy1368 Mar 19, 2023

pjy1368 Mar 19, 2023

sh111-coder commented Mar 19, 2023

pjy1368 left a comment

pjy1368 Mar 20, 2023

sh111-coder Mar 20, 2023

pjy1368 Mar 20, 2023

pjy1368 Mar 20, 2023

pjy1368 Mar 20, 2023

pjy1368 Mar 20, 2023

[1, 2단계 - 체스] 성하(김성훈) 미션 제출합니다. #517

[1, 2단계 - 체스] 성하(김성훈) 미션 제출합니다. #517

Conversation

sh111-coder commented Mar 17, 2023

🎯 Position을 어느 객체가 알게 할 것인가?

sh111-coder commented Mar 18, 2023

🎯 이번 리팩토링 주요 변화

1. 테스트 케이스를 @MethodSource로 많이 추가했습니다!

2. Piece의 상속 구조 변경

✅ 2-1. DirectionalPiece 추상 클래스

✅ 2-2. NormalPiece 추상 클래스

✅ 2-3. LongRangePiece 추상 클래스

🎯 리팩토링 시 질문

✅ Turn 구현 부분 질문

pjy1368 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

sh111-coder commented Mar 19, 2023

🎯 2차 리팩토링 변경점

1. 상속 구조 원래대로 변경

2. Board의 Piece 사용 시 추상화 계층 지키기

3. NameDto, BoardDto 삭제 후 출력용 객체인 NameBoard 구현

pjy1368 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

2. `Piece`의 상속 구조 변경

2. `Board`의 Piece 사용 시 추상화 계층 지키기