[1,2 단계 - 체스] 제나(위예나) 미션 제출합니다. by yenawee · Pull Request #486 · woowacourse/java-chess

yenawee · 2023-03-17T07:24:18Z

안녕하세요 또링 ! 😊

체스 미션을 구현해서 리뷰 요청 드려요 !!
에러 발생 시 재입력 기능이 구현되지 않아서 게임 실행 스크립트를 작성해봤습니다 .. 😿

start
move a2 a4 (white, pawn 2칸)
move b8 a6 (black, knight)
move a4 a5 (white, pawn 1칸)
move b7 b6 (black, pawn 1칸)
move a5 b6 (white, pawn 공격)
move c7 c6 (black, pawn 1칸)
move a1 a6 (white, rook 공격)
move c6 c5 (black, pawn 1칸)
move d2 d4 (white, pawn 2칸)
move e7 e5 (black, pawn 2칸)
move c1 h6 (white, bishop)
move d8 h4 (black, queen)
move g1 f3 (white, knight)
move e8 e7 (black, king)

리뷰 감사드립니다 🙇‍♀️

Co-authored-by: yenawee <yaena0319@naver.com>

jnsorn

안녕하세요 제나! 리뷰가 늦었습니다. 🥲
전반적으로 아주 잘 구현해주신 것 같다는 생각이 드네요.
이전에 학습한 내용도 적절하게 잘 적용해주신 것 같아요.
몇가지 코멘트 남겼는데 확인해보시고, 편하게 DM이나 코멘트 남겨주세요.
이번주도 화이팅입니다. ♟️

jnsorn · 2023-03-20T03:42:18Z

+    private void start(final ChessGame chessGame) {
+        Status gameStatus = new Start(chessGame);
+        while (gameStatus.isRun()) {
+            List<String> commands = InputView.getCommand();
+            final Command command = Command.findCommand(commands);
+            gameStatus = gameStatus.checkCommand(command);
+            OutputView.printBoard(chessGame.getChessBoard());
+        }
+    }


Controller에서 최대한 로직을 덜어냈네요! 특히, 상태에 따라 행위가 달라지는 부분도 별도 객체에 책임을 위임하니 Controller가 최소한의 행위만 하고 있는 것 같아요. 💯

jnsorn · 2023-03-20T03:49:29Z

+    private static char getPieceName(final Map<Position, Piece> board, final int rank, final int file) {
+        final Position position = new Position(rank, file);
+        if (board.containsKey(position)) {
+            return PieceName.findMessage(board.get(position));
+        }
+        return BLANK;
+    }


view에서 출력을 위해 Position 객체를 생성하고 있네요!
view와 model간의 의존성을 제거해보면 어떨까요?

view 와 domain 간의 의존성을 제거하기 위해서 ChessBoardDTO 를 만들어서 String 으로 보드 정보를 변환해 출력하는 것으로 변경했습니다.
근데 rank 와 file 순서대로 반복문 돌리면서 Position 에 기물이 있는지 확인하고 있으면 symbol을, 없으면 empty 를 반환하는 로직에서 계속해서 Position 이 생성되어서 계속 객체가 생성되는 것을 방지하기 위해 Position 을 캐싱하는 방법을 적용했습니다.. 다른 방법이 감이 안왔고 😥Position 이 체스판 수만큼 64개 캐싱해두면 불필요하게 객체 생성이 되는 걸 방지할 수 있어서 해당 방법을 적용해봤는데 혹시 더 좋은 방법이 있을까요ㅜㅜ ?? Map 이 순서가 있게 저장되면 좋을텐데 순서가 없어서 Position 순서대로 정렬할 수 가 없어서 방법을 찾기가 힘들었습니다 ㅠㅠ

ChessBoardDto에서는 Position을 꼭 객체로 가지고 있지 않아도 될 것 같아요. DTO 생성시에는 Map<Position, Piece>을 받더라도, DTO 내부에서는 Map<String, String>과 같은 형태로 표현할 수 있지 않을까요?

jnsorn · 2023-03-20T03:50:09Z

+
+    public static Command findCommand(final List<String> commands) {
+        final CommandType type = Arrays.stream(CommandType.values())
+                .filter(e -> e.name().equalsIgnoreCase(commands.get(0)))


0은 어떤 의미일까요?

명령어 리스트 중 첫번째 명령어(start, move, end)만 뽑아서 검증하기 위한 의미였습니다! 매직넘버 상수화 했습니다.

jnsorn · 2023-03-20T03:52:44Z

+public enum CampType {
+    BLACK,
+    WHITE;
+


Camp라는 용어가 체스 용어인가요?!ㅎㅎ

체스에서 각 플레이어의 팀을 뜻하는 용어가 없어보여서 '진영' 이라는 뜻의 Camp 를 사용했습니다!
다시 보니 TeamColor 로 정의하는게 더 직관적으로 이해하기 좋을 거 같아 이름 변경했습니다 😀

jnsorn · 2023-03-20T03:55:17Z

+    }
+
+    private Status move(final Command command) {
+        validateCommand(command);


move에서 입력값 검증과 move 행위 두 가지를 하고 있는 것 같아요.
검증은 checkCommand에서 하는게 더 적절해보이는데 제나 생각은 어떠세요?

또링 말씀대로 checkCommand 에서 검증해까지 해주고 move 에서는 move 행위만 해주는게 더 적절하다고 생각해 메소드 위치 이동했습니다 !!

jnsorn · 2023-03-20T04:08:34Z

+    public boolean contains(final Position possiblePosition) {
+        return board.containsKey(possiblePosition);
+    }
+
+    public Piece checkPiece(final Position source) {
+        if (contains(source)) {
+            return board.get(source);
+        }
+        throw new IllegalArgumentException("체스말이 존재하는 위치를 입력해 주세요.");
+    }


사소한 네이밍 관련 의견인데요,

contains 메서드 입장에선 매개변수가 possiblePosition인지 몰라도 될 것 같아요. position 정도로 간단하게 표현하는건 어떨까요?

piece를 반환하고 있으니 check보단 get이나 find가 어울릴 것 같아요.

jnsorn · 2023-03-20T04:13:09Z

+    /**
+     * 폰의 이동 규칙을 관리한다.
+     * 1) 첫 시도 - 대각선 위치에 다른 편의 기물 있음 : 공격
+     * 2) 첫 시도 - 대각선 위치에 기물 없음 : 최대 2칸까지 이동 가능, 가능한 위치면 이동
+     * 3) 그 외 - 대각선 위치에 기물 있음 : 공격
+     * 4) 그 외 - 대각선 위치에 기물 없음 : 앞으로 1칸 이동
+     *
+     * @param source 시작 위치
+     * @param target 종료 위치
+     * @param piece  체스 말
+     *                                                         TODO : 여기서 책임을 가지는 건 무의미한 것 같은데, 어떻게 분리하지?
+     */


TODO에도 적어주신것처럼, pawn의 공격 및 이동 규칙이 ChessGame에 있는데 더 적절한 객체로 옮겨보면 어떨까요? 혹시 고민되는 부분이 있다면, DM 남겨주세요!

Pawn 을 BlackPawn 과 WhitePawn 으로 분리하고
Piece 의 canMove 에 매개변수로 targetPiece 를 추가해서 Pawn 에서 이동 및 공격 가능 여부를 판단할 수 있게끔 리팩토링 했습니다.
다른 기물들은 canMove 메소드 사용 시에 targetPiece 가 필요가 없는데 Pawn 때문에 매개변수 하나가 추가되었습니다..🥲 Piece 가 Position 을 가지고 있지 않게 설계했고 Piece 에 chessBoard 정보를 넘기고 싶지 않아서 Piece 에서는 가능한 경로에 target 이 포함되어있는지 확인하는 책임만 주었고 chessBoard 에서 target까지 가는 경로에 기물이 있는지 판단하는 책임을 주어서 (isPossibleRoute 메소드) 이 방식이 그나마 제일 코드가 깔끔해져서 이렇게 수정했습니다..

이미 Piece가 Team을 가지고 있는데, Pawn을 BlackPawn과 WhitePawn으로 분리할 필요가 있을까요?
하나의 Pawn으로도 표현할 수 있을 것 같아요!

BlackPawn 과 WhitePawn 은 움직이는 방향이 달라서 이동 가능한 위치를 계산하는 Direction 이 각각 DOWN, UP 으로 다르고 공격할 수 있는 방향도 각각 아래 대각선, 위 대각선으로 달라서 Pawn 하나로 관리할 시에는 canMove 와 canAttack 메소드에서 if 문으로 분기처리가 필요해서 분리를 하였습니다.!

1,2,3 단계까지는 분리해서 구현하는게 더 코드가 깔끔해보였는데.. 4단계 때 DB 에서 select 로 piecetype 을 받아와서 Piece 를 생성하는 부분에서

public static Piece toPiece(final PieceType pieceType, final TeamColor color) { if (pieceType == PAWN) { return getPawn(color); } return Arrays.stream(values()) .filter(it -> it.name().equalsIgnoreCase(pieceType.name())) .map(it -> it.toPiece.apply(pieceType, color)) .findFirst() .orElseThrow(() -> new IllegalArgumentException("기물을 찾을 수 없습니다.")); } private static Pawn getPawn(TeamColor color) { if (color == TeamColor.BLACK) { return new BlackPawn(PieceType.PAWN); } return new WhitePawn(PieceType.PAWN); }

이렇게 분기처리가 필요해져서 다시 하나로 합칠 필요성도 보입니다 😥

jnsorn · 2023-03-20T04:13:36Z

+
+import java.util.List;
+
+public enum Direction {


방향을 관리하는 객체가 있군요 👍

jnsorn · 2023-03-20T04:18:36Z

+public abstract class Piece {
+    private final PieceType pieceType;
+    private final CampType campType;


Piece가 Movable을 가지고 있을꺼라 생각했는데, 조금 다르게 구현하셨군요!
Piece가 Movable을 갖지 않는 이유가 궁금한데, 얘기해주실수 있나요~?

Piece 와 PieceMove 들을 각각 TDD 로 만들면서 합치는 과정에서 추상화 계층이 불필요하게 나눠진듯합니다 사실 급하게 구현해서 기억이 잘 안납니다 😿
Movable 인터페이스를 Piece 에서 구현하는 식으로 변경했습니다!! (canMove 메소드 오버라이드)

jnsorn · 2023-03-20T04:20:54Z

+import java.util.List;
+import java.util.Set;
+
+public final class Move {


Move 객체의 책임은 무엇일까요?

Move 에서는 piece 가 갈 수 있는 Position 들의 Set 을 반환하는 역할을 하고 있었는데 네이밍이 역할에 맞지 않다는 것을 확인해서 PossibleDestination 으로 일급 컬렉션 객체로 수정했습니다 !

Piece 가 바로 Movable interface 를 구현해 canMove 메소드를 오버라이딩하도록 수정

yenawee · 2023-03-23T16:50:58Z

안녕하세요 또링! 리뷰 반영이 늦었습니다ㅜㅜ
가장 큰 변경사항으로는 Piece 의 이동 가능 규칙 추상화 수준을 좀 더 간단하게 수정했습니다. 또 ChessGame 에서 piece 가 Pawn 인지 Knight 인지 묻지 않고도 piece.canMove() 만을 사용해 타겟이 기물이 이동 가능한 위치인지 판단하고 chessBoard 에서 타겟까지 가는 경로에 기물이 있는지 판단해서 chessGame 에서는 piece 를 이동만 시키도록 했습니다 !!
Readme 에 적어놨는데 반영 못한 사항은 3단계 진행하면서 추가로 적용하도록 하겠습니다.

리뷰 감사합니다 🙇‍♀️

jnsorn

안녕하세요, 제나! 피드백 잘 반영해주셨네요 👍
코멘트도 잘 남겨주셔서 의도 파악하기에도 수월했습니다.
추가로 코멘트 남겼는데, 3,4단계 진행하면서 같이 반영해주시면 좋을 것 같아요!
남은 오후도 (..♟️와 함께.....) 잘 보내시고, 다음 단계에서 만나요 👋

jnsorn · 2023-03-26T07:43:42Z

+        movePiece(source, target, piece);
+    }
+
+    private void validateCamp(final Piece piece) {


여긴 아직 용어가 안바뀐 것 같아요 👀

확인했습니다!!

jnsorn · 2023-03-26T07:44:38Z

+    /**
+     * 폰의 이동 규칙을 관리한다.
+     * 1) 첫 시도 - 대각선 위치에 다른 편의 기물 있음 : 공격
+     * 2) 첫 시도 - 대각선 위치에 기물 없음 : 최대 2칸까지 이동 가능, 가능한 위치면 이동
+     * 3) 그 외 - 대각선 위치에 기물 있음 : 공격
+     * 4) 그 외 - 대각선 위치에 기물 없음 : 앞으로 1칸 이동
+     *
+     * @param source 시작 위치
+     * @param target 종료 위치
+     * @param piece  체스 말
+     *                                                         TODO : 여기서 책임을 가지는 건 무의미한 것 같은데, 어떻게 분리하지?
+     */


이미 Piece가 Team을 가지고 있는데, Pawn을 BlackPawn과 WhitePawn으로 분리할 필요가 있을까요?
하나의 Pawn으로도 표현할 수 있을 것 같아요!

jnsorn · 2023-03-26T07:44:59Z

+    static ChessBoardFactory getInstance(final ChessGame chessGame) {
+        if (CACHE.containsKey(chessGame)) {
+            return CACHE.get(chessGame);
+        }
+        final ChessBoardFactory chessBoardFactory = new ChessBoardFactory();
+        CACHE.put(chessGame, chessBoardFactory);
+        return chessBoardFactory;
+    }


엇 제가 위에 코멘트를 살짝 잘못 단 것 같네요. 😭현재 ChessBoardFactory를 캐싱하여 사용하고 있는데 왜 캐싱을 사용하는지 이유가 궁금했어요! Factory가 캐시로 관리되어야 하는 이유가 있을까요~?

jnsorn · 2023-03-26T07:45:10Z

+    private static char getPieceName(final Map<Position, Piece> board, final int rank, final int file) {
+        final Position position = new Position(rank, file);
+        if (board.containsKey(position)) {
+            return PieceName.findMessage(board.get(position));
+        }
+        return BLANK;
+    }


ChessBoardDto에서는 Position을 꼭 객체로 가지고 있지 않아도 될 것 같아요. DTO 생성시에는 Map<Position, Piece>을 받더라도, DTO 내부에서는 Map<String, String>과 같은 형태로 표현할 수 있지 않을까요?

jnsorn · 2023-03-26T07:53:51Z

+    public boolean isUpsideDiagonalPosition(Position target) {
+        int fileGap = target.file - this.file;
+        int rankGap = target.rank - this.rank;
+        return Math.abs(fileGap) == 1 && rankGap == 1;
+    }
+
+    public boolean isDownSideDiagonalPosition(Position target) {
+        int fileGap = target.file - this.file;
+        int rankGap = target.rank - this.rank;
+        return Math.abs(fileGap) == 1 && rankGap == -1;
+
+    }
+


방향과 관련된 로직은 Direction 객체를 이용해보면 어떨까요?

해당 로직을 Direction 객체를 이용해 판단하는 것으로 수정했습니다 !

Cl8D and others added 30 commits March 14, 2023 17:41

docs: 기능 명세서를 작성한다

5274392

Co-authored-by: yenawee <yaena0319@naver.com>

feat(Position) : 체스판 목표 위치 반환 기능을 구현한다

03ee6d5

Co-authored-by: yenawee <yaena0319@naver.com>

feat(CampType): 입력받은 위치에 따라 진영을 나누는 기능을 추가한다

21c2ca1

Co-authored-by: yenawee <yaena0319@naver.com>

feat(Camp): 입력받은 위치 정보를 검증하고, 진영을 나눈다

0d3d52b

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 체스판 목표 위치 반환 기능을 구현한다

70d1806

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 입력받은 위치에 따라 진영을 나누는 기능을 추가한다

b74cd59

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 입력받은 위치 정보를 검증하고, 진영을 나눈다

dc81448

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 진영을 검은색과 흰색으로 분리한다

1a837b3

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 체스말의 종류를 관리한다

71a8a9b

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 체스판을 초기화한다

248d295

Co-authored-by: yenawee <yaena0319@naver.com>

refactor: 도메인별로 패키지를 분리한다

03c9a07

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 체스판 초기화 시 체스말에 대한 정보만 저장한다

8c8c198

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 사용자가 입력한 명령어를 검증한다

d340443

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 체스말이 흰색 진영인지 확인한다

a55dd78

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 게임을 시작하면 체스판을 초기화한다

f073c49

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 입력받은 위치를 정수값으로 변환한다

fdff963

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 특정 위치에 체스말이 존재하는지 확인한다

af16004

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 특정 위치에 존재하는 체스말을 반환한다

6ae4743

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 퀸이 이동할 수 있는 모든 경로를 확인하고, 이동 가능한지 판단한다

0d03670

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 가능 방향에 체스말이 존재하는지 검증하는 기능을 Move의 책임으로 변경한다

1b8fd8c

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 움직임을 표현하는 인터페이스를 추가한다

4472050

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 룩이 이동할 수 있는지 판단한다

2ea85ee

Co-authored-by: yenawee <yaena0319@naver.com>

test: CampType의 수정에 따라 테스트를 변경한다

aecdcf9

Co-authored-by: yenawee <yaena0319@naver.com>

test: 퀸, 룩의 테스트 조건을 수정한다

a7d2f08

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 비숍이 이동할 수 있는지 판단한다

0d8d1c0

Co-authored-by: yenawee <yaena0319@naver.com>

refactor: 각 체스말들이 최대 이동 거리를 상수화한다

2063c36

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 킹이 이동할 수 있는지 판단한다

62f5fb5

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 나이트가 이동할 수 있는지 판단한다

26a7030

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 폰이 이동할 수 있는지 판단한다

0bbac7f

Co-authored-by: yenawee <yaena0319@naver.com>

feat: 입력받은 위치에 대한 검증을 PositionConverter의 역할로 위임한다

f2d3668

Co-authored-by: yenawee <yaena0319@naver.com>

yenawee added 3 commits March 17, 2023 14:33

학습 : 람다, 스트림

e78feec

Merge branch 'step1' of https://github.com/yenawee/java-chess into step1

f0b03c0

학습 : 정렬 순서 string 길이 내림차순으로 수정

45281d0

jnsorn self-requested a review March 18, 2023 09:57

jnsorn requested changes Mar 20, 2023

View reviewed changes

yenawee added 20 commits March 21, 2023 16:23

docs : 1단계 피드백 사항 및 추가 수정사항 정리

b6173f1

docs : 수정사항 추가

f03ab94

refactor : 매직 넘버 상수화

a7371af

refactor : validatemove 메소드 위치 이동

62c2515

refactor : ChessBoardFactory 생성자 private 변경

d7fa39a

refactor : CampType -> TeamColor 이름 변경

f0c1a94

refactor : checkPiece -> getPiece 메소드 이름 변경

354fdff

refactor : Piece 움직임 추상화 수준 변경

d918fec

Piece 가 바로 Movable interface 를 구현해 canMove 메소드를 오버라이딩하도록 수정

refactor : position 패키지 분리

df72bb1

docs : 반영사항 업데이트

e9491a0

refactor : PossibleDestinations 일급컬렉션 추가

46c778f

refactor : Pawn 공격 및 이동 책임 Pawn 에 위임. WhitePawn / BlackPawn 으로 계층 분리

f464e8d

feat : end 입력 시에는 체스 보드 출력 안되게 수정

b01db32

feat : 예외 발생시 재입력 기능 구현

e583c3b

refactor : view 의 도메인 의존성 제거. ChessBoardDTO 클래스 생성

013816d

refactor : Position 캐싱

636b6ea

fix : 움직이는 경로에 기물 확인 메소드 수정

584d015

fix : BlackPawn, WhitePawn 공격 각각 아래, 위 대각선만 하도록 수정

332371e

fix : pawn 이동 시에는 타겟이 비어있어야 이동 가능

eeeb760

refactor : 사용 안하는 메소드 삭제

83e71cc

yenawee added 2 commits March 24, 2023 02:03

style : 자바 컨벤션

da3505f

refactor : 클래스 이름 컨벤션

1f4fd10

jnsorn approved these changes Mar 26, 2023

View reviewed changes

jnsorn merged commit 721e1bc into woowacourse:yenawee Mar 26, 2023

Conversation

yenawee commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnsorn 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!

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!

yenawee commented Mar 23, 2023

Uh oh!

jnsorn 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yenawee commented Mar 17, 2023 •

edited

Loading