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

Eian1106 · 2026-04-10T04:46:45Z

체크 리스트

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

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

안녕하세요 웨지😆
사이클2는 JDBC를 적용했어야 했는데요, 저는 DB를 사용해본 적이 없기에 우선 빠른 구현을 목표로 적용했습니다.
크루들 사이에선 Connection Pool, Transaction 등 많은 이야기가 오가는데요
해당 내용들을 생각하고 적용하기엔 사이클2의 기간도 얼마 남지 않아 우선 돌아가는 코드 리뷰요청을 드립니다!..
우테코 구현 원칙 동작하는 코드가 되면 바로 PR을 보낸다 ... 😅
잘 부탁드립니다 🙇‍♂️

Palace와 Position

Palace는 위치에 Position과 관련되어 궁성의 위치인지, 궁성 정중앙(왕 위치)인지 확인해주는 클래스 입니다.
그런데 해당 메서드들을 Position에 위치해도 되지않나? 라는 생각을 했습니다.
Position에서 해당 메서드들을 제공한다면 다음과 같이 사용할 수 있을 것 같습니다.

position.isPalacePosition()
position.isGeneralPosition()

그러나 Palace의 내용을 Position에 위치한다면, Position이 하는 책임이 너무 많아지기에 오히려 분리하는 게 궁성 영역에 대한 관리가 쉽다는 의견도 있었습니다.
따라서 웨지의 의견이 궁금합니다!

DB

JDBC를 처음 사용하다보니, 기초적인 CRUD만 학습해보고, 따라해보았는데요
다음 단계로 나아갈 방향을 듣고싶습니다.
웨지의 리뷰를 기다리는동안

커넥션 풀
트랜잭션

에 대해서 학습하고 있겠습니다!
감사합니다 🙇‍♂️

인스턴스 변수 미사용으로 변경

sihyung92

안녕하세요 이안, 리뷰어 웨지입니다.

JDBC가 처음이시라면 새로운 개념이 물밀듯이 들어오실텐데, 학습해보고 적용해보시고 싶으신 게 생기셨다면 반영해주셔요

Palace와 Position

지금은 어디에 있든 큰 차이는 없다고 생각해요. board / piece / position 3개의 패키지가 서로를 알고 있는 상태여서요. 일단 현행대로 해보시죠 ^.^

또 진행하시고 요청주셔요. 화이팅입니다~

sihyung92 · 2026-04-12T02:17:12Z

src/main/java/domain/board/Board.java

+            return Camp.CHO;
+        }
+
+        return Camp.NONE;


열거형(enum)에선 가급적이면 "NONE" 같은, 존재하지 않는 케이스에 대해 정의하지 않는게 맞습니다.

(현업에선 API 버저닝 이슈 등으로 API 파라미터에서 enum에서 정의하지 않은 명칭이 넘어왔을 때 500이 발생하는 걸 막기 위해 UNKNOWN 같은 걸 선언하는 경우가 있긴 한데요.)

매번 Camp에 대한 로직이 필요할 때 마다 NONE에 대한 분기처리를 해줘야합니다.
실은 중립 진영이 존재하지 않는데 "중립"에 대한 처리를 요구받은 개발자는 난감하기도 할테고요. (모든 NONE 케이스를 훑어봐야함)

Optional를 리턴타입으로 해보시면 어떨까요?

sihyung92 · 2026-04-12T02:19:25Z

src/main/java/domain/PieceType.java

+    ELEPHANT(3),
+    SOLDIER(2),
+    CHARIOT(13),
+    NONE(0);


이거 직전에 Camp.NONE에 대한 리뷰를 한 후 비슷한 리뷰를 위해 보다보니, 아예 쓰이지 않는 코드네요. 제거해주셔요.

sihyung92 · 2026-04-12T02:31:11Z

src/main/java/JanggiController.java

    private Camp turnCamp(Camp camp) {
        if (camp.equals(Camp.CHO)) {
            return Camp.HAN;
        }
        return Camp.CHO;
    }


cho 다음은 han, han 다음은 cho 라는 전환 규칙은 도메인 규칙이라고 생각되네요. Camp 메서드로 옮겨주시는 건 어떨까요

sihyung92 · 2026-04-12T02:49:59Z

src/main/java/domain/piece/SingleStepPiece.java

+
+import java.util.List;
+
+public class SingleStepPiece extends Piece {


Horse, Elephant도 거의 같은 로직을 공유하는데요.

건너 뛸 수 있는 기물들의 공통 부모를 정의해주어도 좋겠어요.

SingleStepPiece 객체도 그렇고, Horse/Elephant의 공통 부모도 그렇고 바로 생성할수는 없도록 abstract class로 정의하는 게 좋겠습니다.

java 17부터는 sealed class 로 선언하는 방법도 있으나, 현업 코드에선 java 17 이하 버전인 경우도 있어 활발히 쓰이는 문법은 아닙니다.

Horse/Elephant 의 공통부모인 MultiStepPiece를 만들어 공통 로직을 개선하였습니다!

건너 뛸 수 있는 기물들의 공통 부모

건너 뛸 수 있는 기물들 이라는 것은 Cannon이 건너 뛸 수 있는 기물들. 즉 Cannon을 제외한 모든 것 인가요?
Cannon을 제외한 기물들은 다른 말을 건너 뛸 수 없기에, 말씀해주신 '건너 뛸 수 있는 기물'이라는 표현이 이동 경로 중간에 장애물이 없는지 검사해야 하는 마(Horse)와 상(Elephant)을 지칭하신 것으로 이해하여 반영해 보았습니다!

Chariot도 건너 띌 수가 없으며

Chariot과 Cannon도 공통되는 부분(linear)
이 많지만 사이클2의 시간관계상 방학 중 개선해보도록 하겠습니다 😭

sihyung92 · 2026-04-12T02:54:28Z

src/main/java/domain/piece/Cannon.java

            }
        }
+
+        //궁성 로직


아래 코드의 내용과 동일한 설명이니 제거해주셔요

sihyung92 · 2026-04-12T03:03:53Z

src/main/java/domain/piece/Chariot.java

+        Position current = from;
+        boolean PassGeneralPosition = false;
+        while (current.canMove(direction)) {
+            current = current.move(direction);
+            if(Palace.isGeneralPosition(current)) {
+                PassGeneralPosition = true;
+            }
+            if (current.equals(to) && PassGeneralPosition) {
+                return true;
+            }
+
+            if (boardReader.isExist(current)) {
+                break;
+            }
+        }


from의 궁성의 중심, 즉 (4,8) 등인 경우 버그가 있습니다.

(4,8) -> (5,9) 시나리오를 검토해보세요.

이렇게 버그로 제보된 경우는 테스트를 통해 방어해두는 습관을 들이는게 좋습니다. (왜 기존에 테스트로 해당 버그를 잡을 수 없었는가를 고민해보는 습관)

sihyung92 · 2026-04-12T03:08:39Z

src/main/java/domain/piece/Chariot.java

+                                                         BoardReader boardReader,
+                                                         Direction direction) {
+        Position current = from;
+        boolean PassGeneralPosition = false;


변수명이 PascalCase로 선언되어 있네요

그리고 이걸 다섯번째 인자로 받아주면, 동일한 로직을 재사용할 수 있겠죠.

private boolean canReachTarget(..., Direction direction) { return slideToTarget(from, to, boardReader, direction, true); } private boolean canReachTargetViaGeneral(..., Direction direction) { return slideToTarget(from, to, boardReader, direction, Palace.isGeneralPosition(from)); } private boolean slideToTarget(Position from, Position to, BoardReader boardReader, Direction direction, boolean reachable) { Position current = from; while (current.canMove(direction)) { current = current.move(direction); if (Palace.isGeneralPosition(current)) { reachable = true; } ...

인자가 많이지는 것에 경계를 해야할까요?
실무에서는 인자를 어느정도까지 사용해도 되는지 궁금합니다!

sihyung92 · 2026-04-12T03:15:13Z

src/main/java/domain/PieceGenerator.java

@@ -1,13 +1,15 @@
 package domain;


domain root에 남겨진 class 들이 많은데요.
기존 패키지에 포함시키거나, 새로운 패키지를 만들 수 없을까요?

domain root가 '집 못 찾은 미아 모음집' 이 되어가고 있네요.

sihyung92 · 2026-04-12T03:22:27Z

src/main/java/JanggiController.java


    private final InputView inputView;
    private final OutputView outputView;
+    private final JanggiRepository janggiRepository;


MVC는 Model(데이터 계층)과 View의 교집합 역할을 하죠.
그러나 domain까지 도출되어있는 상황에서 DB 직접 접근은 controller의 코드를 복잡하게 만들어요.

service 계층과 클래스를 하나 만들어 "key를 받아 DB에 접근해 Board를 만들어서 응답하는 절차 메서드", "Board 객체를 DB에 저장하는 절차 메서드"를 만들고 controller에선 좀더 가볍게 질의하는 패턴을 구현해보면 어떨까요?

sihyung92 · 2026-04-12T03:25:43Z

src/main/java/repository/JanggiRepository.java

+    private static final String PASSWORD = "";
+
+    public void saveGame(Map<Position, Piece> boardStatus, Camp camp) {
+        initTable();


테이블 초기화는 매번 하는 작업이 아니기 때문에 Repository 에서 구현하지 않는 경우가 많아요. 때에 따라 매우 위험한 코드가 될 수 있어서요.

개발 환경이나 테스트 환경에서만 별도로 해당 코드가 실행되도록 구현하는 편입니다. 이를 위해 여러 프레임워크에선 방법론이 지원되고 있는데, 우선 어플리케이션이 시작할 때 한번 시도하도록 코드를 만들어보세요. (그럼 나중에 java 환경변수 등을 제공받아 실행할지말지 분기하는 코드로 발전시킬 수 있겠죠?)

sihyung92

안녕하세요 이안, 리뷰어 웨지입니다.
DM에서 말씀드린 대로 우선 머지 후 최종 리뷰를 드릴게요.
어려운 장기 미션 수행하시느라 수고 많으셨어요. 앞으로의 레벨들도 화이팅입니다

Eian1106 added 15 commits April 7, 2026 14:35

feat(Palace): 궁성 위치를 관리하는 객체

5a6ea00

feat(MoveDirection): 대각선 방향 반환 메서드 추가 및 반환타입 수정

dd36370

feat: SingleStepPiece 궁성 범위 내로 이동 제한

5a0490c

refactor: 인스턴스 변수 미사용으로 변경

40d69df

feat: 각 기물 궁성 이동 로직 추가 및 테스트 작성

8f0cb61

인스턴스 변수 미사용으로 변경

rename: rename package pieces to piece

56140b8

feat: 보드에 남은 기물 점수 계산 및 출력

7c5a127

feat: 왕이 잡히면 게임 종료 및 결과 출력

892ed32

build: H2 의존성 추가

2926f5b

feat(inputView): 저장된 게임 불러오기 여부 묻는 메서드 추가

1b7abeb

feat(repository): 장기 게임 저장 및 읽기 기능 추가

07efd61

feat: 저장된 게임 불러오기 및 저장 사용

a206229

docs: readme 업데이트

3fdfae0

docs: readme 업데이트

51eb0b3

refactor: 도메인 패키지 구조 세분화 및 클래스 이동

8ed0089

sihyung92 requested changes Apr 12, 2026

View reviewed changes

Eian1106 added 12 commits April 12, 2026 21:11

refactor: Camp NONE 타입 삭제

360b3b1

refactor: 사용하지 않는 PieceType NONE 제거

5ef111e

refactor: turn 전환 규칙 Camp에서 처리하도록 변경

dfd1a44

refactor: SingleStepPiece abstract클래스로 변경

1bd1bb5

refactor: 불필요한 주석 제거 및 변수명 수정

1bbc0d5

test(Chariot): 궁성 중심에서 궁성 모서리 이동 테스트

dc46e45

fix: 로직 개선 및 궁성의 중심에서 궁성 모서리 이동 버그 수정

2ba7084

refactor: 도메인 패키지 내부 클래스 이동

d1f24d6

refactor: service 계층 도입 및 DB 접근 로직 이관

c821eb3

refactor: gameId를 사용하도록 개선

46fbc4d

refactor: 말과 차 공통부모 정의

ce0c10c

refactor: controller, service package 생성

f388cdb

sihyung92 approved these changes Apr 13, 2026

View reviewed changes

sihyung92 merged commit 58df04b into woowacourse:eian1106 Apr 13, 2026


		import java.util.List;

		public class SingleStepPiece extends Piece {

Conversation

Eian1106 commented Apr 10, 2026

체크 리스트

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

Palace와 Position

DB

Uh oh!

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

Eian1106 Apr 13, 2026 • 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!

sihyung92 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Eian1106 Apr 13, 2026 •

edited

Loading