[🚀 사이클2 - 미션 (기물 확장 + DB 적용)] 파도 미션 제출합니다. by choubung · Pull Request #330 · woowacourse/java-janggi

choubung · 2026-04-08T06:58:29Z

체크 리스트

안녕하세요 베루스!
사이클 2도 잘 부탁드립니다!!

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

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

궁성

1. Place의 중복

현재 Board는 양 진영 전체의 궁성 정보를 가지고 있고, 궁성 내부 기물(왕과 사)는 MoveRule을 통해 간접적으로 자신이 속한 진영의 궁성정보를 가지고 있습니다.
기물이 이동전략을 계산할 때 board를 경유하는 것보다, 스스로 이동 제약 조건을 알고 있는 것(정확히는 Place 객체를 조합으로 가지고 있는 것)이 객체의 자율성 측면에서 더 타당하다고 판단했습니다.
지금 구조가 나을지 아니면 Board만이 Place 정보를 가지는 것이 좋을지(Place도 결국은 보드의 지형이므로) 궁금합니다!

2. 차포졸의 궁성 내 이동

궁성 안에서 대각선 이동이라는 특수한 이동을 하지만, 결국 직선 이동이라는 점은 본질적으로 궁성 밖과 동일하다는 점, 왕과 사와 달리 이동에 제약이 없다는 점에서 설계 아이디어를 착안했습니다.
대각선 이동 시에만 궁성 전용 유효성 검증(갈 수 있는 대각선으로 가는지, (대각선이기 때문에) 궁성 내부 이동인지)을 먼저 수행하고, 실제 이동은 기존의 moveRule이 수행할 수 있도록 했습니다. 유지보수와 확장성을 생각한 설계였는데, 적절한지 여쭤보고 싶습니다.

영속성

1. 전체 삭제 후 전체 업데이트

기물을 이동할 때마다 해당 기물의 정보만 업데이트 하는 게 아니라, 해당 게임의 기물을 모두 지우고 현재 상태를 통째로 재입력하는 방식을 선택했습니다.
기물 수가 많아야 32개이므로, 수정이 잘못되어 유령 데이터가 생기는 경우보다는 아예 전체 단위로 갱신하는 것이 실리적이라고 판단했습니다.
성능보단 데이터 정합성을 우선한 이 선택이 현재 규모의 프로젝트에서 합리적인지, 아니면 작은 프로젝트라도 성능을 추구하는 게 중요한지 듣고 싶어요.

2. 뷰와 도메인과 영속성 코드의 분리, 연결

Enum(PieceType, Team)과 뷰(View)를 분리하기 위해, 기존의 한글 명칭을 제거했습니다. 다만 그러다 보니 DB에 저장할 때 사용할 String정보가 모호해서 고민하다가 DB 저장 및 식별을 위한 '코드(Code)' 필드를 추가했습니다.
요구사항에 기존의 도메인 코드를 최대한 건드리지 말라고 했는데, 식별자를 넣는 위와 같은 행동은 도메인 훼손인지 불가결한 선택인지 궁금합니다.

Empty 기물의 저장 제외

장기판의 모든 좌표(90칸)를 저장하지 않고, 실제 기물이 존재하는 좌표만 DB에 기록하도록 Board와 Repository에서 필터링을 구현했습니다.
지금은 양측에서 거르도록 하고 있지만, 한 쪽이 견고하다면 다른 한 쪽은 불필요한 방어로직인가 하는 생각이 듭니다. Board 쪽으로 검증 로직을 몰아서 애초에 보드가 EmptyPiece는 들고 있지 않도록 하는 게 좋을까요? 어떻게 생각하시는지 궁금합니다.

candidatePositions -> createByDirections beteween -> straightPathBetween

…발생시키도록 변경

verus-j

안녕하세요 파도~
2단계 진행하시느라 고생하셨습니다. Palace로 분리 잘하셨습니다. 아쉬운 점은 Board와 AdvisorMoveRule이 각각 Palace를 사용하고 있는 것 같습니다. 위치에 대해 고민하셨는데 누가 Palace를 더 필요하는지 고민해보고 위치를 결정하면 될 것 같습니다.

차포졸의 궁성 안에서 유효성 검증 순서는 크게 상관없다고 생각합니다. 전체 데이터를 제거 후 새롭게 저장하는 방식을 사용하셨는데, 성능적으로 이슈가 없다면 유지보수하기 편한 쪽으로 정하면 된다고 생각합니다. 전체 제거, 전체 저장이 심플한 것 같긴하네요.

DB에서 뷰와 관련된 데이터를 제거한건 잘하신 것 같습니다. 다만 code로 조회하는 로직에서 Map으로 관리할 필요는 없을 것 같아요.

EmptyPiece에 대해 고민을 하셨는데, DB와 도메인 객체를 항상 동일하게 맞출 필요는 없습니다. Board에 EmptyPiece를 가지고 있지 않지만 DB에 EmptyPiece 정보를 저장할 수도 있고 반대로 Board에 EmptyPiece 정보를 가지고 있지만 DB에 EmptyPiece 정보를 저장하지 않을 수도 있어요. 서로 다르게 변화한다고 생각을 해보면 좋을 것 같아요~

verus-j · 2026-04-09T08:16:25Z

src/test/java/janggi/domain/JanggiGameTest.java

+        janggiGame.decideWinner();
+        assertThat(janggiGame.getWinner()).isEqualTo(Team.CHO);


decideWinner와 getWinner를 합치는건 어떤가요?

코멘트 보고 생각해보니 getWinner가 단순 getter이고, decide Winner랑 떨어져서 사용될 일이 없을 것 같아서 decideWinner가 이긴 진영을 반환하는 것까지 하도록 수정했습니다!

verus-j · 2026-04-09T08:17:07Z

src/test/java/janggi/domain/JanggiGameTest.java

+        janggiGame.skipTurn(); // 초(CHO) 턴 쉼
+        janggiGame.changeTurn();
+        janggiGame.skipTurn(); // 한(HAN) 턴 쉼


주석 설명이 잘못 된 것 같습니다.

턴 쉼이라는 표현이 모호해서일까요? 좀 더 직관적으로 이해하기 쉽도록 "턴 이행하지 않음" 으로 수정했습니다!! 게임 시작시 초나라가 자동으로 선이 되기 때문에 턴을 쉬어가는 팀 순서 자체는 저 순서입니다!

verus-j · 2026-04-09T08:17:52Z

src/test/java/janggi/domain/JanggiGameTest.java

+        janggiGame.skipTurn();
+        janggiGame.changeTurn();


skipTurn 임에도 changeTurn을 호출해줘야 하나요?

아 이 부분때문에 위 코멘트도 달아주신 거였군요! 턴 넘김을 합칠 수 있도록 수정하겠습니다!

verus-j · 2026-04-09T08:23:08Z

src/test/java/janggi/repositiory/game/JdbcGameRepositoryTest.java

+    @Test
+    void save_테스트() {
+        // when
+        Long id = gameRepository.save(false, Team.CHO);


false는 어떤 값일까요?

해당 부분 바로 식별하기 어렵다고 판단하여, 'FinishStatus'라는 vo를 만들어 감쌌습니다!

verus-j · 2026-04-09T08:23:41Z

src/test/java/janggi/repositiory/game/JdbcGameRepositoryTest.java

+        Long id = gameRepository.save(false, Team.CHO);
+
+        // when
+        gameRepository.update(id, true, Team.HAN);


어떤걸 update하는걸까요?

메서드명만 봐도 직관적으로 알 수 있도록 updateStatus로 변경했습니다!

verus-j · 2026-04-09T08:49:31Z

src/main/java/janggi/domain/mouveRule/AdvisorMoveRule.java

    @Override
    public boolean canMove(Position from, Position to, BoardView board) {
-        return Path.candidatePositions(from, ADVISOR_PATHS).contains(to);
+        return palace.canInnerGo(from, to);


호출하기 전에 1칸만 이동하는지 검증이 필요할 것 같네요. 여러칸 이동 시 Palace.canInnerGo의 Direction.between에서 방향을 찾기 못한다고 예외가 나올 것 같습니다.

해당 검증 로직 추가했습니다!

verus-j · 2026-04-09T08:51:19Z

src/main/java/janggi/domain/mouveRule/CannonMoveRule.java

    @Override
    public boolean canMove(Position from, Position to, BoardView board) {
-        if (!from.isStraightLine(to)) {
+        if (!(board.palace().isOnDiagonalPath(from, to) || from.isStraightLine(to))){


Palace를 관리하는 곳이 AdvisorMoveRule과 다르네요. 통일하는게 좋을 것 같습니다~

Palace는 지형이므로 보드에서 관리하는 게 맞다고 생각해 보드로 전부 옮겼습니다!

디미터 법칙을 아실까요? board.place().isOnDiagonalPath는 디미터 법칙을 어겼는지 학습해보면 좋을 것 같아요.

verus-j · 2026-04-09T08:54:25Z

src/main/java/janggi/domain/piece/PieceType.java

+    public static PieceType fromCode(String code) {
+        if (!BY_CODE.containsKey(code)) {
+            throw new IllegalArgumentException("알 수 없는 기물 코드입니다: " + code);
+        }
+
+        return BY_CODE.get(code);


code로 pieceType을 변환할 때 꼭 Map을 써야할까요? 그냥 values()로 순회해서 찾아도 될 것 같습니다.

넵!! 비슷하게 Team도 stream을 통해 값을 찾아 반환하도록 수정했습니다!

verus-j · 2026-04-09T08:55:25Z

src/main/java/janggi/repositiory/game/GameRepository.java

+    Long save(Boolean isFinished, Team currentTurn);
+    Optional<GameData> findLatestOngoingGame();
+    void update(Long id, boolean isFinished, Team currentTurn);


CRUD를 각 컬럼의 값을 받지 말고 Game 자체를 받아서 처리하는건 어떨까요?

그게 확실히 캡슐화를 더 잘 지킬 수 있는 방법인 것 같네요 그렇게 수정하겠습니다!

verus-j · 2026-04-09T08:57:31Z

src/main/java/janggi/repositiory/piece/PieceRepository.java

+    void updateALL(Long gameId, Map<Position, Piece> map);
+    List<PieceData> findAll(Long gameId);


Map<Position, Piece>를 감싼 객체를 하나 만드는 것도 좋을 것 같네요. 거기에 gameId를 지정하면 편하게 조회 및 수정을 객체하나 단위로 할 수 있을 것 같아요~

BoardSnapshot 객체를 만들어 사용하도록 변경했습니다!

기존에 턴 쉼으로 되어 있던 주석을 로직 행위와 같은 맥락으로 이해하기 쉽도록 턴 이행하지 않음으로 수정함

…<Position, Piece>로 변경

무엇을 업데이트하는지 직관적으로 알 수 있도록 메서드명을 변경

…을 객체로 감쌈

choubung · 2026-04-10T07:48:15Z

안녕하세요 베루스!

Palace는 지형이므로 Board가 관리하는 것이 맞다고 생각하여, 기물들은 Board를 통해 단순 요청만 진행하고 진영 판단 및 Palace 활용은 Board가 알아서 수행하도록 수정하였습니다.

Enum에서 code로 조회하는 로직도 코멘트에 단 것처럼 수정하였어요!

DB에 대해서는 도메인과 맞춰야한다는 생각도 했고, 존재 기물만 관리하는 편이 개수가 적어서 더 좋지 않을까 생각한 것도 있었습니다! 여태까지는 DB와 도메인을 최대한 일치시켜야한다고 생각했는데, 리뷰 덕분에 새로운 인사이트를 얻었어요.

추가로 이번 PR에서 여쭤보고 싶은 게 한 가지 있는데, fileChanged에 코멘트로 남겨놓겠습니다. 감사합니다!

choubung · 2026-04-10T07:51:12Z

src/main/java/janggi/service/JanggiService.java

+        this.pieceRepository = pieceRepository;
+        this.janggiGame = janggiGame;
+        this.boardSnapshot = boardSnapshot;
+        this.isResumed = isResumed;


이 변수는 '이전 게임이 재시작된건지/새로운 게임이 시작된건지' 여부를 저장하는 변수입니다.
view에서 이전 게임이 재시작된건지 아니면 새 게임이 시작된 건지를 출력하고 싶어서 넣었어요,
그런데 이건 시작 한 번만 사용되는 변수라 되도록이면 service의 상태로 두고 싶지 않은데, 현재 코드에서 제 아이디어로는 service와 isResumed를 함께 감싸 날리는 객체를 만드는 것밖에 생각이 안 나더라고요.
그런데 이 한 번을 위해 객체를 만드는 게 적절한가라는 생각도 들어서 고민이 됩니다...

개인적으로 지금은 Service를 활용하지 않아도 될 것 같습니다. 심플하게 Controller에서 Repository를 이용하도록 구현해도 좋을 것 같아요. 다른 곳에 피드백 남겨놨는데, Service에 대한 학습은 레벨2에서 집중해보면 좋을 것 같아요~

verus-j

안녕하세요 파도~
리뷰가 늦었습니다. 마지막으로 피드백들 남겨놨는데 확인부탁드려요~

verus-j · 2026-04-12T12:06:29Z

src/main/java/janggi/controller/JanggiController.java

+        this.janggiService = JanggiService.startGame(gameRepository, pieceRepository);
+    }

    public void run() {


run 메서드가 너무 기네요 ㅠㅠ 정리를 조금하면 좋을 것 같아요~

verus-j · 2026-04-12T12:09:44Z

src/main/java/janggi/domain/board/BoardView.java

    Piece findByPosition(Position position);

    boolean isEmptyPosition(Position position);

    PieceType findTypeByPosition(Position position);
+
+    Team findTeamByPosition(Position position);


문득 드는 생각이 이 4가지 인터페이스는 findByPosition만 제공해도 충분히 받는 쪽에서 처리할 수 있을 것 같네요. 편리하게 기능을 제공하는 것도 좋지만 BoardView의 인터페이스를 작게 구성하는 것도 한번 고민해보면 좋을 것 같아요.

verus-j · 2026-04-12T12:11:06Z

src/main/java/janggi/domain/janggiGame/JanggiGame.java

+    }
+
+    public void skipTurn() {
+        skip.put(currentTurn, true);


skip을 이후에 다시 false로 세팅하는 곳이 없는데 의도한걸까요?

엇 아니요..! play turn을 move로 합치는 과정에서 실수로 누락한 것 같습니다 move 메서드에 false 세팅 구문 추가했습니다 감사합니다!

verus-j · 2026-04-12T12:12:06Z

src/main/java/janggi/domain/mouveRule/CannonMoveRule.java

    @Override
    public boolean canMove(Position from, Position to, BoardView board) {
-        if (!from.isStraightLine(to)) {
+        if (!(board.palace().isOnDiagonalPath(from, to) || from.isStraightLine(to))){


디미터 법칙을 아실까요? board.place().isOnDiagonalPath는 디미터 법칙을 어겼는지 학습해보면 좋을 것 같아요.

verus-j · 2026-04-12T12:13:02Z

src/main/java/janggi/service/JanggiService.java

+        this.isResumed = isResumed;
+    }
+
+    public static JanggiService startGame(GameRepository gameRepository, PieceRepository pieceRepository) {


startGame도 너무 긴 메서드네요. 정리를 하면 좋을 것 같아요~

verus-j · 2026-04-12T12:14:03Z

src/main/java/janggi/service/JanggiService.java

+        janggiGame.move(moveCommand.getFrom(), moveCommand.getTo());
+        boardSnapshot = boardSnapshot.updatePieces(janggiGame.getBoard());


boardSnapshot은 janggiGame에서 생성하는 방식으로 관리해도 되지 않을까요? janggiGame에서 최신 스냅샷을 가져와서 DB에 저장하는게 깔끔해보입니다.

리팩토링 과정에서, play시 skip여부를 false로 만드는 구문이 삭제되었음 버그이므로 move 메서드에 해당 로직을 추가하는 것으로 수정함

choubung added 30 commits April 3, 2026 14:49

refactor(Path): 메서드 이름 변경

93dde0c

candidatePositions -> createByDirections beteween -> straightPathBetween

refactor(EmptyPiece) : EmptyPiece가 moveRule()을 호출했을 때 null 반환 대신 예외를 …

ff267e5

…발생시키도록 변경

feat : between 메서드가 찾을 수 있는 방향에 대각선 추가

06ae73e

refactor(Path) : straightPathBetween함수가 8방향을 찾음에 따라 Between으로 이름 변경

aff00ff

test : 궁성 안 이동 테스트 케이스 추가

417793e

feat : 궁성 내 이동 구현

54b927f

test : 사 궁성 이동 테스트케이스 작성

b5bec61

feat : 사 궁성 이동 구현

c9cccd7

test(cannon) : 궁성 내 대각선 이동 테스트 케이스 추가

fb2ea8f

test : 방향 찾기 테스트 케이스에 대각선 추가

c38ace6

feat : 보드 전체에 궁성 구현

903e3a0

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

9085744

feat : 궁성 내 포 이동 구현

a4be080

test : 불필요한 주석 삭제

ee82a66

test(tank) : 궁성 연관 이동 테스트 케이스 추가

cc44781

feat(tank) : 궁성 연관 이동 구현

a8de20c

test : 궁성 연관 이동 테스트 케이스 추가

248aa3d

feat(direction) : 남/북에 따라 {남동, 남서} / {북동, 북서} 방향 리스트를 반환하는 메서드 작성

aa84b7e

feat : 졸 궁성 연관 이동 구현

40edcc5

build : h2 의존성 추가

9ca80ab

docs : 뷰와 승패 비즈니스 로직 설명 추가

c201d18

refactor(Team) : 도메인에서 view 분리

7e1d854

feat(inputView) : 턴 옵션 선택 메서드 추가

ef0dc6a

test : 장기 게임 테스트 작성

18d4814

test : 장기 게임 테스트 추가

9f080d7

Test : ScoreBoard 테스트 작성

a70d031

test : 왕 배치를 위한 헬퍼 메서드 작성

52133ce

feat : 무승부 점수 계산을 위한 ScoreBoard 작성

192dec0

feat(board) : 팀별 기물 조회와 kIng 총 개수 조회 메서드 추가

2221595

featoutputView) : 승패 관련 출력 메서드 추가

8359d52

choubung added 2 commits April 8, 2026 14:58

feat : 이전 게임 출력 문구 메서드 작성

3e7b33e

docs(readme) : 승패와 영속성 구현에 따른 문서 설명 내용 업데이트

b205d18

verus-j requested changes Apr 9, 2026

View reviewed changes

choubung added 23 commits April 10, 2026 09:55

refactor(janggiGame) : 단순 getter인 getWinner를 decideWinner 메서드로 병합

4d9941e

test(janggiGame) : 모호한 주석 수정

acea135

기존에 턴 쉼으로 되어 있던 주석을 로직 행위와 같은 맥락으로 이해하기 쉽도록 턴 이행하지 않음으로 수정함

test : 서비스 테스트 추가

f4024d0

test : 일부 객체를 변수명에 할당하여 활용

8d76e5f

test : 장기 게임 테스트 수정

fba6432

refactor(pieceRepository) : findAll이 반환하는 자료형을 List<PieceData> 에서 Map…

be21d24

…<Position, Piece>로 변경

refactor(gameRepository) : 메서드명 변경

919b957

refactor(EmptyPiece) : EmptyPiece를 싱글톤으로 변경

44e81c7

refactor : 전반적인 흐름 다듬기

a097528

refactor(gameRepository) : game 객체를 바로 넘기도록 변경

dfecd14

refactor(board) : remove와 place의 추상화 수준 맞춤

ae0b19d

refactor(FinishStatus) : 게임 종료 여부 value object 작성

cff6888

refactor : code로 타입을 찾을 때 map 대신 순회를 사용하도록 변경

d3cfecc

refactor(JanggiGame) : changeTurn 메서드를 삭제하고 해당 로직을 사용 위치에 직접 작성

b1d0599

refactor(ScoreBoard) : score를 객체 생성 시에 저장할 수 있도록 수정

c98dbbf

test(ScoreBoard) : ScoreBoard 생성자 구조 변경에 따른 변경

0fc2b72

refactor(ScoreBoard) : 점수 저장을 생성자에서 수행

edc8e21

refactor(GameRepository) : 메서드 이름 update를 updateStatus로 변경

a3a2e2e

무엇을 업데이트하는지 직관적으로 알 수 있도록 메서드명을 변경

refactor(Palace) : Palace를 Board만 알도록 수정

77f2606

refactor : Map<Position, Piece>와 gameId를 감싸는 BoardSnapshot를 생성해 기존 값들…

88609d0

…을 객체로 감쌈

test : BoardSnapshot 사용에 따른 테스트 변경

6df1f4c

fix(BoardSnapshot) : 업데이트가 제대로 되지 않는 문제 해결

36d8365

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

e7e156e

choubung commented Apr 10, 2026

View reviewed changes

verus-j requested changes Apr 12, 2026

View reviewed changes

fix(JanggiGame) : skip 여부 관리 버그 수정

83be7cc

리팩토링 과정에서, play시 skip여부를 false로 만드는 구문이 삭제되었음 버그이므로 move 메서드에 해당 로직을 추가하는 것으로 수정함

		janggiGame.decideWinner();
		assertThat(janggiGame.getWinner()).isEqualTo(Team.CHO);

		void updateALL(Long gameId, Map<Position, Piece> map);
		List<PieceData> findAll(Long gameId);

		janggiGame.move(moveCommand.getFrom(), moveCommand.getTo());
		boardSnapshot = boardSnapshot.updatePieces(janggiGame.getBoard());

Conversation

choubung commented Apr 8, 2026

체크 리스트

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

궁성

영속성

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!

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!

choubung commented Apr 10, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone