[🚀 사이클1 - 미션 (보드 초기화 + 기물 이동)] 이프 미션 제출합니다. by jihwankim128 · Pull Request #211 · woowacourse/java-janggi

jihwankim128 · 2026-03-28T04:36:34Z

체크 리스트

미션의 필수 요구사항을 모두 구현했나요?
Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
애플리케이션이 정상적으로 실행되나요?

어떤 부분에 집중하여 리뷰해야 할까요?

여러 클래스에서 반복되던 로직을 중간 추상 클래스에서 템플릿 메서드 패턴 등을 활용해 잘 통합했는지 궁금합니다.
새로운 실체 클래스를 추가할 때, 기존의 추상 클래스 계층 구조를 건드리지 않고도 확장이 가능한 구조인지 궁금합니다.

그 외에는 아직 어떤 질문을 해야 할 지 … 잘 떠오르지 않아서 성장할 수 있는 길을 제안해주시면 정말 감사하겠습니다!

verus-j

안녕하세요~ 리뷰어 베루스입니다.
첫 번째 리뷰로 테스트 코드와 관련된 피드백 남겨놨습니다. 확인부탁드려요~
피드백 반영 후 리뷰 요청 다시 주시면 프로덕션 코드 위주의 피드백 남겨드리겠습니다!
질문의 답변은 프로덕션 코드 리뷰시 남겨드릴게요~

verus-j · 2026-03-29T09:21:05Z

src/main/java/model/Janggi.java

+        if (piece.isEnemy(turn)) {
+            throw new IllegalArgumentException(turn.getName() + "의 기물이 아닙니다.");
+        }


적군의 기물을 찾는 케이스에 대한 테스트 코드가 없습니다~

verus-j · 2026-03-29T09:21:45Z

src/main/java/model/Janggi.java

+        if (!piece.canMove(current, next)) {
+            throw new IllegalArgumentException("해당 기물이 이동할 수 없는 위치입니다.");
+        }


이동할 수 없는 위치에 대한 테스트 케이스가 없습니다~

verus-j · 2026-03-29T09:22:35Z

src/main/java/model/Janggi.java

+        List<Position> path = piece.extractPath(current, next);
+        if (board.hasPieceAt(path)) {
+            throw new IllegalArgumentException("이동 경로에 기물이 있어 이동할 수 없는 위치입니다.");
+        }


이동경로에 기물이 없어서 정상적으로 이동할 수 있는 케이스에 대한 테스트 코드가 없습니다~

verus-j · 2026-03-29T09:23:28Z

src/main/java/model/board/Direction.java

+        if (Math.abs(displacement.row()) > Math.abs(displacement.col())) {
+            return findByRowCol(Integer.signum(displacement.row()), 0);
+        }
+        return findByRowCol(0, Integer.signum(displacement.col()));


row가 0인 경우에 대한 테스트 케이스가 없습니다~

아 해당 로직은 페어와 소통 중 페어의 방식으로 진행한 것이라 나중에 로직부터 수정해도 괜찮을까요?
사실 아직 로직이 잘 이해가 안돼서요 ㅠㅠ

verus-j · 2026-03-29T09:25:39Z

src/test/java/model/piece/CannonTest.java

+    @ParameterizedTest
+    @MethodSource("model.fixture.PieceTestFixture#직선_이동_불가능한_위치")
+    void 포는_대각선으로_이동할_수_없다(Position current, Position next) {
+        // given
+        Piece cannon = new Cannon(Team.HAN);
+        // when
+        boolean canMove = cannon.canMove(current, next);
+        // then
+        assertThat(canMove).isFalse();
+    }


직선_이동_불가능한_위치라고 했는데 결국 대각선으로 이동하는 케이스인 것 같네요. 어떤 종류의 이동 동선인지에 더 집중한 네이밍이면 좋을 것 같습니다.

감사합니다! 8방위 중 4간방을 제외한 4방위를 직선이라고 생각하고, 4간방을 직선이 아니라고 정의해버렸네요...
사실 대각선도 방향 자체는 직선인데 말이죠..! 동시에 직선에 대한 케이스도 수정하겠습니다.

@ParameterizedTest @MethodSource("model.fixture.PieceTestFixture#직선_이동_가능한_위치") // 수정 void 포는_직선으로_이동할_수_있다(Position current, Position next) {

verus-j · 2026-03-29T09:29:22Z

src/test/java/model/piece/CannonTest.java

+        janggi.move(new Position(5, 0), new Position(1, 0));
+
+        // then
+        assertThat(board.pickPiece(new Position(1, 0)).isCannon()).isTrue();


적군 기물을 잡았는데, 검증문이 충분한지 고민해보면 좋을 것 같아요~

verus-j · 2026-03-29T09:31:31Z

src/test/java/model/piece/ChariotTest.java

+    @Test
+    void 차가_한_칸_이동하면_중간_경로가_없다() {
+        // given
+        Piece chariot = new Chariot(Team.HAN);
+        Position current = new Position(3, 4);
+        Position next = new Position(3, 5);
+
+        // when
+        List<Position> path = chariot.extractPath(current, next);
+
+        // then
+        assertThat(path).isEmpty();
+    }


extractPath에 시작점과 끝점이 없다는 사실은 테스트 케이스를 통해 확실히 알 수 있을 것 같네요. extractPath 메서드에서 해당 정보를 알 수 있도록 메서드명을 지으면 좋을 것 같아요~

verus-j · 2026-03-29T09:31:53Z

src/test/java/model/piece/ChariotTest.java

+    }
+
+    @Test
+    void 이동_경로에_기물이_있으면_멱이_적용된다() {


멱이 적용된다는건 어떤 의미인가요?

장기에서 이동 방향에 기물이 있는 경우, 멱이라고 오인 했습니다.
나무위키-멱을 찾아 본 결과 마나 상에만 적용되는 개념이더라구요 !

테스트에 대해 수정하겠습니다.

verus-j · 2026-03-29T09:33:09Z

src/test/java/model/piece/ChariotTest.java

+        // given
+        Piece chariot = new Chariot(Team.HAN);
+        Position current = new Position(0, 0);
+        Position next = new Position(0, 5);
+
+        Map<Position, Piece> pieces = new HashMap<>();
+        pieces.put(current, chariot);
+        pieces.put(new Position(0, 3), new Chariot(Team.CHO));
+        Board board = new Board(pieces);
+
+        // when
+        List<Position> path = chariot.extractPath(current, next);
+
+        // then
+        assertThat(board.hasPieceAt(path)).isTrue();


정확히 어떤걸 검증하려는걸까요? 테스트 코드만 봤을 때는 Chariot보다는 board의 hasPieceAt인거 같습니다. 테스트 케이스가 적절한 위치에 있는지 고민해보면 좋을 것 같아요~

저도 동의합니다.
페어와 빠른 제출을 목표로 하면서 조금 더 깊게 생각하지 못한 것 같습니다.
수정하겠습니다 😭

verus-j · 2026-03-29T09:37:10Z

src/test/java/model/piece/ElephantTest.java

+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+public class ElephantTest {


대부분의 기물과 관련된 테스트 클래스 안에서 Piece의 하위 클래스(Cannon, Elephant 등), Board, Janggi 클래스의 동작을 테스트하고 있네요. 다른 피드백을 확인하셔서 적절한 위치에서 테스트 케이스를 작성했는지 고민해보면 좋을 것 같아요. 그리고 하나의 기물을 테스트하기 위해 여러 클래스를 활용해야만 한다면 기물과 관련된 기능이 흩어져 있는건 아닌지 고민해보는 것도 좋을 것 같아요~

verus-j

안녕하세요 이프~ 리뷰어 베루스입니다.
프로덕션 코드에 대한 피드백 남겨놨습니다. 확인하셔서 반영해주세요~
Piece를 활용해 공통 로직을 모으고 각 기물별로 특화된 로직을 하위 클래스에 배치하도록 하신건 잘하셨습니다. 새로운 기물이 추가되는 케이스에서는 무리 없이 추가할 수 있을 것 같습니다. 한가지 걸리는 점은 포의 이동 조건에 대한 일부 로직이 Board에 있는 점인데, 만약 새로운 기물이 포와 비슷한 이동 조건이 필요하다면 Board에도 추가 로직을 작성해야하겠네요. 관련된 피드백도 있으니 확인해보시고 개선하면 좋을 것 같아요~

verus-j · 2026-04-01T13:11:24Z

src/main/java/model/formation/FormationFactory.java

+        formations.putAll(extractFormationByTeam(hanFormation, Team.HAN));
+        formations.putAll(extractFormationByTeam(choFormation, Team.CHO));


hanFormation인데 Team.HAN을 지정하고 있네요. 느낌이 hanFormation을 만들 때부터 Formation 객체에 Team 정보를 가지고 있는게 좋을 것 같네요.

좋은 피드백 감사합니다!
팀 정보가 이리저리 남용되고 있는 것 같네요 :)
개선해보겠습니다!

verus-j · 2026-04-01T13:13:46Z

src/main/java/model/board/Position.java

+    public static final Position HAN_LEFT_OUTER = new Position(0, 1);
+    public static final Position HAN_LEFT_INNER = new Position(0, 2);
+    public static final Position HAN_RIGHT_INNER = new Position(0, 6);
+    public static final Position HAN_RIGHT_OUTER = new Position(0, 7);
+
+    public static final Position CHO_LEFT_OUTER = new Position(9, 1);
+    public static final Position CHO_LEFT_INNER = new Position(9, 2);
+    public static final Position CHO_RIGHT_INNER = new Position(9, 6);
+    public static final Position CHO_RIGHT_OUTER = new Position(9, 7);


FormationStrategy 하위 클래스에서만 사용하는데 Position에서 public 전역 변수로 만들지 않아도 될 것 같습니다. 더 좋은 위치를 고민해보면 좋을 것 같아요~

이 부분에 대해서도 고민을 했는데, PR에 포함을 못했네요 ... ㅎㅎ

객체가 가져야 할 책임을 어느 위치에 두는가에 대해 고민했습니다.
당시 저는 포지션이 위치 객체이므로 스스로 책임을 관리해야 된다고 생각했습니다.

하지만 지금 생각해보니 스스로 책임을 관리한다는 것이, 객체 자신의 정보에 대해 책임을 관리하는 것을 의미한다고 느껴지네요.
그래서 Formation에 대한 위치 정보는, 위치 객체의 정보를 활용하는 것이 아닌 Formation의 정보를 위해 위치 객체를 사용하는 관점이네요 !

좋은 피드백 주셔서 감사합니다 :)

verus-j · 2026-04-01T13:15:17Z

src/main/java/controller/JanggiController.java

+        Map<Position, Piece> pieceByFormation = FormationFactory.generateFormation(hanFormation, choFormation);
+        Board board = BoardFactory.generatePieces(pieceByFormation);
+        outputView.displayBoard(board.board());


BoardFactory에 Map으로 포메이션 별 기물을 전달하지 않고 FormationFactory를 전달하는게 좀 더 깔끔하지 않을까 싶습니다.

verus-j · 2026-04-01T13:16:17Z

src/main/java/model/board/BoardFactory.java

+
+public class BoardFactory {
+
+    private static final Map<Position, Piece> RED_PIECES = Map.ofEntries(


RED_PIECES, GREEEN_PIECES라는 이름은 뷰와 연관된 이름 같네요. 도메인에 맞는 이름을 사용하면 좋을 것 같아요~

verus-j · 2026-04-01T13:18:52Z

src/main/java/controller/JanggiController.java

+        Position current = inputView.readSource(currentTurn);
+        Piece piece = janggiGame.selectPiece(current);
+
+        Position next = inputView.readDestination(currentTurn, piece);


current와 next 이름과 readSource, readDestination 이름은 서로 안맞는 것 같습니다. 공통된 이름을 사용하면 좋을 것 같아요~

네이밍 너무 어렵네요 😭
조금 더 이해하기 쉽게 개선해보겠습니다!

verus-j · 2026-04-01T13:26:11Z

src/main/java/model/JanggiGame.java

+        Piece piece = board.pickPiece(current);
+        validateCurrentTurn(piece);


selectPiece를 사용해도 될 것 같습니다~

verus-j · 2026-04-01T13:31:09Z

src/main/java/model/JanggiGame.java

+    public Piece selectPiece(Position position) {
+        Piece piece = board.pickPiece(position);
+        validateCurrentTurn(piece);
+        return piece;
+    }
+
+    private void validateCurrentTurn(Piece piece) {
+        if (piece.isOtherTeam(turn)) {
+            throw new IllegalArgumentException(turn.getName() + "의 기물이 아닙니다.");
+        }
+    }


코드가 심플해서 남기기 애매한 리뷰이긴 하지만 메서드 추상화 수준을 학습하기 좋을 것 같아 남깁니다. baord.pickPiece와 validateCurrentTurn의 추상화 수준이 동일한지 고민해보면 좋을 것 같고 클린 코드 관점에서 추상화 수준이 동일하면 어떤 점들이 좋을지도 같이 고민해보면 좋을 것 같아요~

verus-j · 2026-04-01T13:36:14Z

src/main/java/model/board/Board.java

+    private void validateMove(Position current, Position next, Piece piece) {
+        validateCanMoveByPiece(current, next, piece);
+
+        List<Position> path = piece.extractPath(current, next);
+        if (piece.isCannon()) {
+            validateContainOnlyOnePiece(path);
+            validateNotContainCannon(path);
+            return;
+        }
+
+        validateNotContainPiece(path);
+    }


여기도 추상화 수준이 섞여있네요. 추상화 수준을 맞춰보면 좀 더 코드가 정리될 것 같습니다. 그리고 포에 특화된 로직이 Board에 있는 것 같네요. 포에 특화된 로직들은 Cannon으로 옮기는게 좋을 것 같은데, 고민해보면 좋을 것 같아요~

verus-j · 2026-04-01T13:44:04Z

src/main/java/model/piece/Cannon.java

+        Direction direction = Direction.from(current, next);
+        List<Position> path = new ArrayList<>();
+        Position step = current.move(direction);
+        while (!step.equals(next)) {
+            path.add(step);
+            step = step.move(direction);
+        }
+        return path;


기물별로 어느정도는 중복된 로직인 것 같은데 어떻게하면 중복 로직을 제거할 수 있을지 고민해보면 좋을 것 같아요. 중복을 제거한다고 상속 구조를 복잡하게 만들지는 않았으면 합니다. 최대한 심플하게 고민해보면 좋을 것 같아요~

* 중복되는 로직 제거 * 뷰와 연관된 네이밍 수정 * 책임에 맞는 변수 위치 수정

jihwankim128 and others added 15 commits March 24, 2026 20:28

docs: 1단계 보드초기화 문서 작성

31156ca

feat: 한/초 상차림 입력 기능 추가

479c1d2

feat: 상차림 결과 출력 기능 추가

d48f485

docs: 2단계 기물 이동 기능 명세

e3deb3e

feat: 기물 이동 기능 추가

43b6a34

차/상/마에 대한 멱 기준 적용

4aa1071

refactor: 기물의 진영 판단 로직 메서드로 분리(isCho)

c0b0c98

feat: 포 이동 규칙 추가

6dc5899

refactor: 사용하지 않는 import 제거 및 메소드 순서 step-down 방식 적용

f389157

style: 주석 javadocs로 변경

a994b25

refactor: import.* 해제

8a9035d

refactor: 상, 마가 멱일 때 테스트 코드 추가

ca929e7

refactor: 메서드 순서 수정

e980fe6

refactor: 장기판 로직 중괄호 Depth 감소하는 로직으로 수정

ebba9f4

refactor: 주석 제거

c26b932

verus-j requested changes Mar 29, 2026

View reviewed changes

jihwankim128 added 6 commits March 30, 2026 14:58

test: 차와 포 기물 이동 test fixture 데이터 셋 수정

7015c8b

test: 테스트 중복 제거 및 올바른 객체 테스트로 이동

d6a8dd6

refactor: 기물 이동 경로 테스트 리팩토링

7fba746

refactor: 보드 일급 컬렉션과 장기 AggregateRoot 명확히 분리

6e7083e

refactor: 장기판 관련 클래스 패키지 분리

b44e3fd

refactor: board move 로직 리팩토링

ba78637

verus-j requested changes Apr 1, 2026

View reviewed changes

jihwankim128 added 5 commits April 2, 2026 17:30

refactor: 장기 포메이션 로직 리팩토링

cb70ddc

* 중복되는 로직 제거 * 뷰와 연관된 네이밍 수정 * 책임에 맞는 변수 위치 수정

refactor: 기물 선택 및 배치에 대한 입력 메서드 네이밍 수정

528918a

refactor: 보드 및 장기게임 추상화 수준 통일

b743a35

refactor: 기물 별 이동 중복 로직 분리

a531224

refactor: 기물 이동 검증 로직 개선

f20b496

		formations.putAll(extractFormationByTeam(hanFormation, Team.HAN));
		formations.putAll(extractFormationByTeam(choFormation, Team.CHO));


		public class BoardFactory {

		private static final Map<Position, Piece> RED_PIECES = Map.ofEntries(

		Piece piece = board.pickPiece(current);
		validateCurrentTurn(piece);

Conversation

jihwankim128 commented Mar 28, 2026

체크 리스트

어떤 부분에 집중하여 리뷰해야 할까요?

Uh oh!

verus-j left a comment • 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!

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!

verus-j 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

verus-j left a comment •

edited

Loading