[3, 4단계 - 체스] 페드로(류형욱) 미션 제출합니다. #730

hw0603 · 2024-03-27T05:39:45Z

안녕하세요 제이! 3단계와 4단계 필수 요구사항의 구현을 마쳐 다시 리뷰 요청 드려요.

4단계 DB 적용에서 사용한 DDL은 다음과 같습니다.

CREATE TABLE `game` (
  `gameId` int NOT NULL AUTO_INCREMENT,
  `turn` varchar(10) DEFAULT NULL,
  PRIMARY KEY (`gameId`)
);

CREATE TABLE `piece` (
  `file` int NOT NULL,
  `rank` int NOT NULL,
  `pieceColor` varchar(50) NOT NULL,
  `pieceType` varchar(50) NOT NULL,
  `gameId` int NOT NULL,
  PRIMARY KEY (`file`,`rank`,`gameId`),
  KEY `gameId` (`gameId`),
  CONSTRAINT `gameId` FOREIGN KEY (`gameId`) REFERENCES `game` (`gameId`)
);

3단계와 4단계에서 생각보다 변경 사항들이 꽤 생겼는데요, 그러다 보니 1, 2 단계와 달리 질문드릴 것들이 상당히 많아진 것 같아요. 본문이 너무 길어질 것 같아서 질문사항들은 별도의 코멘트로 남겨 두겠습니다!

이번 리뷰도 잘 부탁드려요🙂

- 상태 클래스 패키지 이름 변경(/status -> /state)

- 상태 객체들을 final class 로 변경

- 게임 종료 시 재시작 가능하도록 변경

hw0603 · 2024-03-27T08:40:33Z

Record의 캐싱

체스 도메인 특성 상 자주 사용되는 File Rank, Position 객체에 대한 캐싱을 구현하였습니다.
File과 Rank는 Enum이라 static 블록을 활용하여 쉽게 구현할 수 있었지만 레코드에서 캐싱을 진행할 경우 의문점이 들었습니다.
구현할 수 있는 가장 간단한 캐싱을 생각해 보면, 객체 내부에 Map 형태의 인스턴스를 유지하고 있고, 생성 요청이 들어왔을 때 해당 요청에 해당하는 키가 Map에 존재하는지 확인해야 하며, 정적 팩토리 메서드를 사용하면 쉽게 구현할 수 있습니다.
하지만 레코드에 팩토리 메서드가 존재하는 것이 어색하다는 의견도 있었고(저는 상관없다는 입장이긴 합니다), 무엇보다 기본 생성자로 생성했을 때 캐시를 통과하지 않는다는 점이 마음에 걸립니다. 그렇다고 레코드에서 기본 생성자를 private으로 두자니 레코드를 사용하는 의미가 없어지는 것 같구요. 결국 hashCode()와 equals()를 직접 오버라이드하는 비용을 감수하고 클래스로 변환하거나, 레코드의 기본 생성자를 private으로 두는 다소 어색한 구현 중에 선택해야 할 것 같은데 제이의 생각은 어떤지 듣고 싶습니다.

상태패턴에서 항상 모든 동작을 상태객체로 위임해야 하는가?

ChessGame 이 도입되면서 게임의 진행 상태(백 차례, 흑 차례, 종료)에 따라 기능을 달리하기 위하여 상태패턴을 도입했습니다. 이때 현재 각 팀의 평가치를 구하는 로직은 상태와 무관한 로직이라고 생각되어 상태객체 안으로 넣지 않았습니다. 상태패턴을 사용한 다양항 예제를 검색해 보니 이러한 방식으로 구현한 코드는 찾기가 힘들어 상태객체를 지금과 같이 사용하는 것이 어색한 것인지, 아니면 별도의 컨벤션이 존재하는지 궁금합니다.

상태패턴에서 상태 전이의 책임은 어느 객체에 있는가?

현재까지의 생각 흐름은 다음과 같습니다.

상태를 가지는 객체(Context)가 변경하는 경우 -> 상태를 직접 설정해 주는 것이 부자연스럽게 느껴짐
State가 전이함수 호출 시 다음 State를 반환 -> 전이함수에서 다른 반환값이 필요할 때는 사용 불가능 + FSM 구조 자체에서 사이클이 있을 경우(흑과 백의 상태가 계속 toggle 되는 경우 등) 상태객체가 서로를 알고 있게 됨
State.nextState() 에서 다음 State를 반환 -> State별 전이함수가 하나만 존재할 때만 적용 가능

3가지 모두 각자의 단점이 뚜렷하여 어떤 것이 좋은 방향인지 잘 모르겠습니다. 이것도 결국 활용하는 상황 별로 트레이드오프가 적용되는 부분일까요?

`Optional`의 올바른 사용

Optional은 반환 값 등 제한적인 곳에서만 사용되도록 설계되었기 때문에 필드나 파라미터에는 사용하지 않아야 한다고 알고 있습니다. board.move()에서 잡힌 기물을 반환하는데, 잡힌 기물이 있을 수도 있고 없을 수도 있으니 Optional<Piece>를 반환하도록 설계했습니다. 이 반환값을 다음과 같이 지역변수로 가지고 있는 것은 큰 문제가 없는 것인지 궁금합니다.

...
        Optional<Piece> caughtPiece = getCaughtPiece(destination);
        chessBoard.put(destination, piece);

        if (caughtPiece.isPresent()) {
            PieceType caughtPieceType = caughtPiece.get().getPieceType();
            return new CaughtMoveResponse(caughtPieceType);
        }
        return new NormalMoveResponse();

또한, 현재 chessGame.move() 는 내부적으로 board.move() 로 포워딩되어 있습니다. board.move() 를 호출했는데 뜬금없이 Piece(잡힌 말)가 반환되는 것이 어색할 수 있다고 생각되어 이를 래핑한 MoveResponse를 추가하고 응답 객체의 다형성으로 처리되도록 해 두었습니다. 단순히 Optional을 계속하여 리턴해 주는 구현과 차이점을 생각해 보면

.isPresent() 검사를 caller에게 위임하지 않을 수 있음
메서드명-반환형태 간의 어색함이 조금 해소됨

정도일 것 같은데 1번이 과연 장점이 맞는가? 에 대한 의문이 들었습니다. Optional 객체에 대한 isPresent 검사를 상위 caller에게 계속 위임하는 것(메서드 시그니처에 exception을 명시해서 caller에게 위임하는 것 처럼)은 보통 잘 사용되지 않는 방식인가요?

명령-쿼리 분리 원칙

위의 질문과 연관된 질문인데, 제이의 개인적인 생각을 듣고 싶어서 별도로 분리했습니다.
.move()가 해당 이동으로 인해 죽은 상대편 말을 반환하는 것은 CQRS 위반이지만, stack.pop() 처럼 예외적으로 허용 가능한 범위 내에 속한다고 생각하여 현재 구현을 유지하였습니다. 제이는 이에 관해 어떤 생각이신가요?
(사실 Board에서 팀 별 잡힌 말의 리스트를 계속 유지하고 있지 않은 이상 어떻게 분리해야 할지 감이 잘 안 오기도 합니다..ㅎㅎ)

DAO 객체에 대한 테스트 격리, 테스트 멱등성 유지 + 트랜잭션

JDBC를 잘 사용해 보지 않아 테스트 코드를 작성하는 데 어려움이 있었습니다.
현재는 @BeforeEach와 @AfterEach를 통해 모든 테이블을 초기화해 주고 있는데, 별로 좋지 못한 방법인 것 같아요.

트랜잭션을 사용하여 테스트하고, 테스트가 끝나면 롤백하는 기법을 많이 사용하던데, 이러한 구조에서는 만약 하나의 서비스 메서드에서 다수의 DAO를 참조하는 경우 각 DAO의 작업은 공통된 connection에서 행해져야 하고, 다시 말해 DAO에서 connection을 생성하거나 close 하는 책임을 가질 수 없습니다.
그렇다면 Service 없이 DAO 객체를 직접 사용할 때는 외부에서 connection을 직접 생성하여 전달하고, 사용한 후에는 다시 외부에서 close 해 주어야 하는 불편이 발생할 것 같은데, 전체적으로 connection을 어떻게 관리하고 트랜잭션을 어떻게 설정해 주어야 할지 궁금합니다.

혹은, 트랜잭션 롤백을 사용하지 않고 테스트 코드의 멱등성을 유지하고 테스트 대상 코드 간 상호 의존을 없앨 수 있는 방법이 있을까요?
(JDBC 개념이 잘 잡혀있지 않은 것 같아서 제가 질문을 맞게 한 건지도 잘 모르겠네요😅)

DB 저장을 위한 데이터는 어느 계층까지 전파되어도 괜찮은가?

DB가 도입되기 전(3단계) 까지는 컨트롤러에서 도메인 데이터를 알 일이 크게 없었는데요, DB 저장을 위해 컨트롤러에서 서비스를 호출하다 보니, 서비스에 전달할(DB에 저장할) 데이터를 컨트롤러 단까지 모두 가져와야 하는 경우가 생겼습니다. 데이터가 있는 도메인에서 DAO를 바로 사용하지 않고 컨트롤러 -> 서비스 -> DAO 순으로 흘러가다 보니 발생한 문제인 것 같은데, 각 계층별 책임을 분리하면서 데이터의 영속성을 유지하기 위한 트레이드오프라고 보면 될까요?

hw0603 · 2024-03-27T08:58:03Z

src/main/java/domain/game/ChessGame.java

+    }
+
+    public void move(Position source, Position destination) {
+        MoveResponse moveResponse = state.move(board, source, destination);


ChessGame이 board를 알고 있음에도 책임을 위임하기 위해 board를 상태객체의 인자로 전달하는 것이 혹시 어색한 것인지 궁금합니다!

상태객체가 board를 가지게 하기에는 배보다 배꼽이 더 커지는 느낌이라 일단 배제하긴 했어요.

넵 이 부분 아래 코멘트에 남겨두었는데 서로 분리를 해주는게 좋아보입니다.

JunHoPark93

안녕하세요 페드로~
마지막 단계 구현하느라 고생많으셨습니다.
질문주신 부분들 남겨두었는데 일단 보시고 의견 다시 남겨주세요!

JunHoPark93 · 2024-03-27T14:38:13Z

docs/README.md

+  - 각 팀의 점수는 남아 있는 팀의 기물 평가치의 합으로 계산한다.
+  - 같은 세로줄에 같은 색의 폰이 있는 경우, 각 폰들은 원래 평가치의 절반으로 계산된다.
+
+### ChessGame


docker compose파일을 잘 올려주시긴했는데 README에 실행법도 간략히 정리되어있으면 더 좋을거같아요

반영했습니다!

JunHoPark93 · 2024-03-27T14:48:06Z

src/main/java/domain/game/Board.java

@@ -1,30 +1,36 @@
 package domain.game;

+import domain.position.File;


체스 도메인 특성 상 자주 사용되는 File Rank, Position 객체에 대한 캐싱을 구현하였습니다.
File과 Rank는 Enum이라 static 블록을 활용하여 쉽게 구현할 수 있었지만 레코드에서 캐싱을 진행할 경우 의문점이 들었습니다.
구현할 수 있는 가장 간단한 캐싱을 생각해 보면, 객체 내부에 Map 형태의 인스턴스를 유지하고 있고, 생성 요청이 들어왔을 때 해당 요청에 해당하는 키가 Map에 존재하는지 확인해야 하며, 정적 팩토리 메서드를 사용하면 쉽게 구현할 수 있습니다.
하지만 레코드에 팩토리 메서드가 존재하는 것이 어색하다는 의견도 있었고(저는 상관없다는 입장이긴 합니다), 무엇보다 기본 생성자로 생성했을 때 캐시를 통과하지 않는다는 점이 마음에 걸립니다. 그렇다고 레코드에서 기본 생성자를 private으로 두자니 레코드를 사용하는 의미가 없어지는 것 같구요. 결국 hashCode()와 equals()를 직접 오버라이드하는 비용을 감수하고 클래스로 변환하거나, 레코드의 기본 생성자를 private으로 두는 다소 어색한 구현 중에 선택해야 할 것 같은데 제이의 생각은 어떤지 듣고 싶습니다.

저는 크게 어색해보이지 않는데요, 어떤 점때문에 어색하다고 하신걸까요?
오히려 프로그램이 뜰때 모든 Position객체를 전부 만들어두는것도 개수가 많지 않을거같아서 또하나의 방법이 될거같아요.

사실 'Record 에서는 팩토리 메서드 없이 기본 생성자로 객체를 생성하는 것이 자연스러운 것 같다.' 라는 리뷰를 받은 적이 있어요.
완전히 동의하지는 않아서 팩토리 메서드는 적극적으로 활용하고 있지만.. 그렇다고 레코드에서 기본생성자를 private로 만들어도 되나? 의 의문이 남긴 했습니다.

아마 Record본연에 측면에 중점을 두어서 그렇게 말씀하신것으로 사료되네요.

간결하고 명확하게 데이터를 표현하는게 목적이기때문에(컴파일타임에 자동생성되는 생성자까지 이용해서 더 간결해지는..) 관점의 차이가 있다고 생각합니다. 조금더 도메인적으로 설계를 하냐 데이터 중점적으로 설계를 하냐의 차이로 보이는데 선택해주시면 될거같아요. 헷갈리지않게 record를 아예 쓰지 않는것도 오해의 소지를 줄일수있을거같아요. 일단 저는 상관없다고 생각하는데 같이 일하시는분들과 의견을 잘 맞춰보세요.

JunHoPark93 · 2024-03-27T14:52:18Z

src/main/java/dao/GameDao.java

+        } catch (final SQLException e) {
+            throw new RuntimeException(e);
+        }
+        return -1;


어떤 의미에서의 return일까요?

엇.. 수정했어야 했는데 누락되었었네요😅 예외를 발생시키도록 수정했습니다!

JunHoPark93 · 2024-03-27T14:54:28Z

src/main/java/dao/DBConnector.java

+        } catch (final SQLException e) {
+            System.err.println("DB 연결 오류:" + e.getMessage());
+            e.printStackTrace();
+            return null;


null을 리턴하는것보다 정의된 예외를 던지는게 낫지 않을까요? 실행처에서도 try catch로 묶어서 사용중이신거같아요.

JunHoPark93 · 2024-03-27T14:59:11Z

src/main/java/dao/PieceDao.java

+    public void addAll(List<PieceDto> pieceDtos, int gameId) {
+        pieceDtos.forEach(pieceDto -> addPiece(pieceDto, gameId));
+    }


단건 insert를 반복하기보다 bulk형태의 insert를 시도해보면 좋을거같아요

JunHoPark93 · 2024-03-27T15:39:17Z

src/main/java/domain/game/state/GameState.java

+
+    TeamColor currentTurn();
+
+    MoveResponse move(Board board, Position source, Position destination);


명령-쿼리 분리 원칙
위의 질문과 연관된 질문인데, 제이의 개인적인 생각을 듣고 싶어서 별도로 분리했습니다.
.move()가 해당 이동으로 인해 죽은 상대편 말을 반환하는 것은 CQRS 위반이지만, stack.pop() 처럼 예외적으로 허용 가능한 범위 내에 속한다고 생각하여 현재 구현을 유지하였습니다. 제이는 이에 관해 어떤 생각이신가요?
(사실 Board에서 팀 별 잡힌 말의 리스트를 계속 유지하고 있지 않은 이상 어떻게 분리해야 할지 감이 잘 안 오기도 합니다..ㅎㅎ)

정의하기 나름이지만, 목적지로 움직임에 대한 결과라고 생각하기 때문에 괜찮다고 생각합니다. 움직이고나서 기물을 잡는 행위를 구분을 하신게 아니기 때문에 (움직임과 동시에 기물을 잡는) 굳이 신경안쓰셔도 될거같아요.

JunHoPark93 · 2024-03-27T15:43:39Z

src/main/java/controller/ChessController.java

 import java.util.function.Supplier;
 import view.InputView;
 import view.OutputView;

 public class ChessController {
    private final InputView inputView;
    private final OutputView outputView;
+    private final DBService dbService;


DB 저장을 위한 데이터는 어느 계층까지 전파되어도 괜찮은가?
DB가 도입되기 전(3단계) 까지는 컨트롤러에서 도메인 데이터를 알 일이 크게 없었는데요, DB 저장을 위해 컨트롤러에서 서비스를 호출하다 보니, 서비스에 전달할(DB에 저장할) 데이터를 컨트롤러 단까지 모두 가져와야 하는 경우가 생겼습니다. 데이터가 있는 도메인에서 DAO를 바로 사용하지 않고 컨트롤러 -> 서비스 -> DAO 순으로 흘러가다 보니 발생한 문제인 것 같은데, 각 계층별 책임을 분리하면서 데이터의 영속성을 유지하기 위한 트레이드오프라고 보면 될까요?

제가 질문을 잘 이해를 못한거같은데, db에 저장할 데이터를 컨트롤러단으로 가져오는게 어느부분이죠?
저장을하려면 당연히 필요한 과정같은데 해당부분에 코멘트를 다시남겨주세요

// ChessController.java private void saveCurrentStatus(ChessGame chessGame) { int gameId = dbService.saveGame(chessGame); outputView.printSaveResult(gameId); }

지금은 Service 레이어가 생기면서 chessGame 자체를 전달하도록 변경하여 컨트롤러에서 직접 게임 내부의 데이터를 가져와서 저장하거나 하는 부분은 없어진 것 같아요.

사실 현재의 미션에만 국한된 질문은 아니였습니다. 처음 저 의문이 들었던 배경 설명을 좀 드리자면 3단계까지 구현했을 때는 (DB에 저장할 일이 없다 보니) 객체 간 메시지 보내기 로 충분히 구현 가능했던 부분이 4단계로 오니 DB에 저장하기 위해서 특정 데이터를 직접 반환하는 게터 느낌의 메서드가 추가되는 경우가 있었어요.

대표적인 예로 Piece에서 TeamColor 값 자체를 꺼내올 일은 없었는데 DB 저장 요구사항이 추가되고 나니 직접 가져와서 저장해야 하는 소요가 생기더라구요.

꼭 체스 미션이 아니라 다른 프로젝트들에서도 이렇게 도메인 로직에는 사용되지 않지만 DB 저장을 위해 필요한 게터들이 생기는 경우가 있을 것 같은데, 말씀하신 대로 저장을 위해 당연히 필요한 과정이라고 생각하고 고민 없이 사용하면 되는 부분일까요?

넵 저장에 필요하니 당연히 사용해야죠.

지금 domain과 infra entity가 혼용되어 사용되어 발생하는 의문인데 이걸 나눠서 관리하면 좀 나을거에요. 다만 이것도 중복코드가 많이 발생하기 때문에 트레이드오프가 있어서 프로젝트하실때 직접 경험해보시면 더 좋을거에요.

JunHoPark93 · 2024-03-27T15:49:06Z

src/test/java/dao/GameDaoTest.java

+    final GameDao gameDao = GameDao.getInstance();
+    final PieceDao pieceDao = PieceDao.getInstance();
+
+    @BeforeEach


DAO 객체에 대한 테스트 격리, 테스트 멱등성 유지 + 트랜잭션
JDBC를 잘 사용해 보지 않아 테스트 코드를 작성하는 데 어려움이 있었습니다.
현재는 @beforeeach와 @AfterEach를 통해 모든 테이블을 초기화해 주고 있는데, 별로 좋지 못한 방법인 것 같아요.

트랜잭션을 사용하여 테스트하고, 테스트가 끝나면 롤백하는 기법을 많이 사용하던데, 이러한 구조에서는 만약 하나의 서비스 메서드에서 다수의 DAO를 참조하는 경우 각 DAO의 작업은 공통된 connection에서 행해져야 하고, 다시 말해 DAO에서 connection을 생성하거나 close 하는 책임을 가질 수 없습니다.
그렇다면 Service 없이 DAO 객체를 직접 사용할 때는 외부에서 connection을 직접 생성하여 전달하고, 사용한 후에는 다시 외부에서 close 해 주어야 하는 불편이 발생할 것 같은데, 전체적으로 connection을 어떻게 관리하고 트랜잭션을 어떻게 설정해 주어야 할지 궁금합니다.

혹은, 트랜잭션 롤백을 사용하지 않고 테스트 코드의 멱등성을 유지하고 테스트 대상 코드 간 상호 의존을 없앨 수 있는 방법이 있을까요?
(JDBC 개념이 잘 잡혀있지 않은 것 같아서 제가 질문을 맞게 한 건지도 잘 모르겠네요😅)

체스판을 초기화를 하고나서 조회만 일어나는 테스트라면 굳이 매번 초기화할 필요는 없는것 아닐까요?

아니면 아래와 같은 방식을 말씀하신걸까요?

@BeforeEach public void setUp() throws SQLException { connection = dataSource.getConnection(); ... } @AfterEach public void tearDown() throws SQLException { connection.rollback(); connection.close(); ... }

혹은 h2나 컨테이너 형태로 테스트하는 방법도 있을거에요.

아니면 아래와 같은 방식을 말씀하신걸까요?

네, 저런 형태로 트랜잭션 롤백을 통해 멱등성을 구현해 보고 싶긴 했는데, 저렇게 되면 DAO의 메서드 하나하나 마다 Connection 객체를 외부에서 전달받도록 해야겠더라구요.
외부에서 Connection 생성 -> DAO로 전달 -> DAO에서 해당 connection으로 쿼리 수행 -> 외부에서 Connection 정리 의 과정을 거치게 되는데, DAO 사용 시 마다 커넥션을 전달해 주는 것이 불편하게 느껴졌습니다. DAO에서 커넥션을 가져오고 close 해 주지 않다 보니 외부에서 계속 신경을 써 줘야 하는 부분도 있겠구요.

저러한 불편함은 트랜잭션을 걸어 주기 위해서는 감수해야 하는 사항일까요? 제이라면 테스트 멱등성을 어떻게 가져갈지 궁금합니다. 크루들과도 얘기를 나눠 봤는데, 아직까지 명쾌한 답을 찾은 크루가 없더라구요

(h2는 이번 미션 취지에서 살짝 벗어난 것 같아서 우선 고려하지 않았어요!)

추가로, piece 테이블이 game 테이블의 gameId 필드를 외래키로 참조하고 있어서 Piece를 추가하려면 게임이 있어야 하고, 반대로 게임을 삭제하려면 Piece들이 다 삭제되어야 하는 구조가 되었습니다.

이러한 구조 때문에 GameDaoTest와 PieceDaoTest 두 테스트 클래스 모두에서 GameDao와 PieceDao가 필요한데, 테스트 해야 할 대상끼리 서로 의존해서는 TDD사이클을 준수할 수도 없고, 구조 자체도 이상하다고 생각했어요.

FK로 연결된 테이블간의 DAO 테스트에서 이러한 의존을 끊어낼 수 있나요?
테스트 코드가 아닌 다른 곳에서 piece 정보가 등록되지 않은 game을 초기 데이터로 미리 등록해 둔 상태에서 'piece 등록 테스트'와 'game 삭제 테스트'를 수행하거나 해야 할까요?

그렇다고하면 통합테스트를 하려면 어쩔수 없는 상황인거같아요. test profile로 격리가 된 환경에서 테스트를 진행하더라도 그 안에서 여러 테스트가 무리없이 돌게하려면 위와같은 처리가 필요합니다.

통합테스트를 의도하셨으면 의존을 끊으면 안됩니다.

근데 현재 구현해주신 테스트를 보면 데이터세팅을 위해서만 사용되고있는것처럼보이는데 저런경우에는 보통 테스트시작전에 쿼리파일을 통해서 테스트데이터를 밀어넣고 시작하게 됩니다. 마지막에는 teardown으로 정리를 시켜주고요.

다른 dao로 세팅해주면 간편하기야 하겠지만 의존관계가 걸리신다면 위와같은 방법으로 시도해볼수있을거같아요.

통합테스트를 의도한 것은 아닌데, 실제 DB에 입출력이 일어나는 테스트는 단위테스트보다는 통합테스트의 영역이 맞긴 하겠네요💡

JunHoPark93 · 2024-03-27T15:51:25Z

src/main/java/domain/service/DBService.java

+    public int saveGame(ChessGame chessGame) {
+        int gameId = addGame();
+        saveTurn(gameId, chessGame.currentPlayingTeam());
+
+        List<PieceDto> pieces = chessGame.getPositionsOfPieces().entrySet().stream()
+                .map(entry -> PieceDto.of(entry.getKey(), entry.getValue()))
+                .toList();
+        saveAllPieces(gameId, pieces);
+        return gameId;


메서드 길이를 줄일 수 있는 방법이 있을까요?

List<PieceDto> pieces = chessGame.getPositionsOfPieces().entrySet().stream() .map(entry -> PieceDto.of(entry.getKey(), entry.getValue())) .toList();

이 부분을 PieceoDto 내의 static 메서드로 PieceDto.asList(positionsOfPieces) 와 같이 분리하는 것 이외에는 잘 생각이 나지 않는데 혹시 힌트를 좀 더 받을 수 있을까요..?

private 메서드 추출을 의도했어요. 레벨1에서 극단적으로 연습하는 메서드길이를 줄여보는 차원입니다.

아아 그런 의도셨군요 확인했습니다!

JunHoPark93 · 2024-03-27T15:54:09Z

src/main/java/dao/PieceDao.java

+    public PieceDto findPiece(int fileIndex, int rankIndex, int gameId) {
+        final String query = String.format("SELECT * FROM %s WHERE `file` = ? and `rank` = ? and `gameId` = ?", TABLE_NAME);
+        try (final Connection connection = getConnection();
+             final PreparedStatement preparedStatement = connection.prepareStatement(query)) {
+            preparedStatement.setInt(1, fileIndex);
+            preparedStatement.setInt(2, rankIndex);
+            preparedStatement.setInt(3, gameId);
+            final ResultSet resultSet = preparedStatement.executeQuery();
+            if (resultSet.next()) {
+                String pieceColor = resultSet.getString("pieceColor");
+                String pieceType = resultSet.getString("pieceType");
+                return new PieceDto(fileIndex, rankIndex, pieceColor, pieceType);
+            }
+        } catch (SQLException e) {
+            throw new RuntimeException(e);
+        }
+        return null;


여러 메서드들간에 중복되는 코드들이 보이는데 개선이 가능할까요?

String 형태 쿼리 선언 -> PreparedStatement 생성 -> 쿼리 실행 형태의 중복을 말씀하시는게 맞을까요?

뭔가 JDBC에서 쿼리를 실행하는 템플릿..에 가까운 것 같아서 중복인 상태로 두는 것이 읽기 편할 것 같다고 생각했는데 혹시 어떻게 생각하시나요?

맞습니다. 다만 말씀하신것처럼 중복을 제거해서 구현했을시 단점이 더 크다면 그냥 두셔도 됩니다.

- DAO 의 각 메서드 인자로 Connection 을 전달받도록 변경

hw0603 · 2024-03-29T07:01:30Z

안녕하세요 제이, 위 리뷰에서 말씀하셨던 부분 수정하여 다시 리뷰 요청 드렸습니다!

hw0603 · 2024-03-29T07:18:11Z

src/main/java/service/DBService.java

+    public int saveGame(ChessGame chessGame) {
+        Connection connection = getConnectionOfAutoCommit(false);
+        try (connection) {
+            int gameId = gameDao.addGame(connection);
+            gameDao.updateTurn(connection, gameId, chessGame.currentPlayingTeam());
+            pieceDao.addAll(connection, collectPositionOfPieces(chessGame), gameId);
+            connection.commit();
+            return gameId;
+        } catch (SQLException | DBException e) {
+            try {
+                connection.rollback();
+            } catch (SQLException rollbackException) {
+                throw new IllegalStateException("트랜잭션 롤백 중 오류가 발생했습니다.", rollbackException);
+            }
+            throw new DBException("게임 저장 중 오류가 발생했습니다.", e);
+        }
+    }


Service는 트랜잭션 처리의 단위가 되므로 에서는 서비스 객체는 자신이 사용할 DAO에 대한 인스턴스를 유지하고 있고, 각 DAO를 사용할 때 동일한 Connection으로 작업 요청 후, 커밋과 롤백을 담당해야 한다고 생각했습니다.

Connection 객체는 생성 비용이 꽤 비싼 자원이라고 알고 있습니다.
현재는 ConnectionPool 을 별도로 구현하지는 않았지만, getConnection() 로직을 개선하여 Pool 에서 available 한 Connection 중 하나를 반환한다 와 같이 사용될 수 있을 것 같습니다.

따라서, 서비스는 외부에서 주입된 Connection 하나를 계속 점유하고 있기 보다는 서비스 로직이 실행될 때 서비스 내부에서 getConnection() 하고, 로직이 종료되면 .close() 등을 통해 자원을 해제하는 것이 타당하다는 판단을 내렸습니다.

이때, 서비스 내 DAO간 커넥션을 공유해 주기 위해서 DAO의 모든 메서드에서 Connection 객체를 인자로 전달받아 사용하게 되었습니다. 그러다 보니 테스트를 위한 FakeDAO 등을 만들 때도 테스트에서는 사용하지 않는 Connection 객체를 인자로 받아야 했구요.

Spring과 같은 프레임워크를 사용하지 않을 때, 보통 서비스에서는 DAO 간 Connection 공유를 어떻게 구현하는지 궁금합니다.

이 부분까지 신경써주셨네요. 잘해보셨습니다!

Connection 객체는 생성 비용이 꽤 비싼 자원이라고 알고 있습니다.
현재는 ConnectionPool 을 별도로 구현하지는 않았지만, getConnection() 로직을 개선하여 Pool 에서 available 한 Connection 중 하나를 반환한다 와 같이 사용될 수 있을 것 같습니다.

맞습니다. 미션을 빠르게 구현해주신거에 비해 생각해볼만한 부분들까지 잘 해주셔서 시간이 남는다면 이 부분도 직접 구현해봐도 좋을거같아요.

아래와같이 롤백부분도 재사용가능하게 개선해볼수도있을거같아요.

public int saveGame2(ChessGame chessGame) { try (Connection connection = getConnectionOfAutoCommit(false)) { try { int gameId = gameDao.addGame(connection); gameDao.updateTurn(connection, gameId, chessGame.currentPlayingTeam()); pieceDao.addAll(connection, collectPositionOfPieces(chessGame), gameId); connection.commit(); return gameId; } catch (SQLException | DBException e) { rollbackTransaction(connection); throw new DBException("게임 저장 중 오류가 발생했습니다.", e); } } catch (SQLException e) { throw new IllegalStateException("DB 연결 종료 중 오류가 발생했습니다.", e); } } private void rollbackTransaction(Connection connection) { try { if (connection != null) { connection.rollback(); } } catch (SQLException e) { throw new IllegalStateException("트랜잭션 롤백 중 오류가 발생했습니다.", e); } }

Spring과 같은 프레임워크를 사용하지 않을 때, 보통 서비스에서는 DAO 간 Connection 공유를 어떻게 구현하는지 궁금합니다.

대학생때 모바일앱 개발하면서 커넥션을 직접 다루면서 db연결작업을 한적이 있었는데 자원정리를 빠트리는곳들도 있고 점점 복잡해지면서 누수를 잡을 수 없을정도로 엉망이 된적이 있는데요, 그때 이후로 직접 관리를 한적이 없네요.

또한 업무상 상용서비스를 개발할때 프레임워크의 도움을 받지않고 만든적은 한번도 없어서 이렇게 해봤다라고 말씀을 드리기 힘드네요.
다만 구현한다고한다면 두 가지 측면을 고려할거같습니다.

커넥션 풀

트랜잭션

매번 연결을 만들게되면 성능상 단점이 있기 때문에 적절한 풀 형태를 구현할거같고, 또한 spring에서 제공하는 선언적트랜잭션 관리를 직접 구현하려면 위에 말씀드린것처럼 트랜잭션 처리를 모든 코드에 다 넣어야될겁니다. 자원이 제대로 해제가 됐는지에 대한 부분도 잘 신경써야겠구요.

그래도 우테코하면서 한번정도는 직접 만들어보면 재밌기도하고 성숙한 상용 모델을 경험할때 도움이 많이 될거같아요.

JunHoPark93 · 2024-03-30T07:45:40Z

docs/README.md

+## 실행 방법
+1. MySQL 컨테이너 실행


정리 잘 해주셨네요 👍🏼

JunHoPark93 · 2024-03-30T07:48:06Z

src/main/java/domain/game/state/GameState.java

+import domain.game.TeamColor;
+import domain.position.Position;
+
+public interface GameState {


잘판단해주셨습니다. GameState내 move가 제거되면서 인터페이스의 의미도 좀 더 명확하게 드러나는거같네요!

JunHoPark93 · 2024-03-30T08:10:20Z

src/main/java/service/DBService.java

+    public int saveGame(ChessGame chessGame) {
+        Connection connection = getConnectionOfAutoCommit(false);
+        try (connection) {
+            int gameId = gameDao.addGame(connection);
+            gameDao.updateTurn(connection, gameId, chessGame.currentPlayingTeam());
+            pieceDao.addAll(connection, collectPositionOfPieces(chessGame), gameId);
+            connection.commit();
+            return gameId;
+        } catch (SQLException | DBException e) {
+            try {
+                connection.rollback();
+            } catch (SQLException rollbackException) {
+                throw new IllegalStateException("트랜잭션 롤백 중 오류가 발생했습니다.", rollbackException);
+            }
+            throw new DBException("게임 저장 중 오류가 발생했습니다.", e);
+        }
+    }


이 부분까지 신경써주셨네요. 잘해보셨습니다!

Connection 객체는 생성 비용이 꽤 비싼 자원이라고 알고 있습니다.
현재는 ConnectionPool 을 별도로 구현하지는 않았지만, getConnection() 로직을 개선하여 Pool 에서 available 한 Connection 중 하나를 반환한다 와 같이 사용될 수 있을 것 같습니다.

맞습니다. 미션을 빠르게 구현해주신거에 비해 생각해볼만한 부분들까지 잘 해주셔서 시간이 남는다면 이 부분도 직접 구현해봐도 좋을거같아요.

아래와같이 롤백부분도 재사용가능하게 개선해볼수도있을거같아요.

public int saveGame2(ChessGame chessGame) { try (Connection connection = getConnectionOfAutoCommit(false)) { try { int gameId = gameDao.addGame(connection); gameDao.updateTurn(connection, gameId, chessGame.currentPlayingTeam()); pieceDao.addAll(connection, collectPositionOfPieces(chessGame), gameId); connection.commit(); return gameId; } catch (SQLException | DBException e) { rollbackTransaction(connection); throw new DBException("게임 저장 중 오류가 발생했습니다.", e); } } catch (SQLException e) { throw new IllegalStateException("DB 연결 종료 중 오류가 발생했습니다.", e); } } private void rollbackTransaction(Connection connection) { try { if (connection != null) { connection.rollback(); } } catch (SQLException e) { throw new IllegalStateException("트랜잭션 롤백 중 오류가 발생했습니다.", e); } }

Spring과 같은 프레임워크를 사용하지 않을 때, 보통 서비스에서는 DAO 간 Connection 공유를 어떻게 구현하는지 궁금합니다.

대학생때 모바일앱 개발하면서 커넥션을 직접 다루면서 db연결작업을 한적이 있었는데 자원정리를 빠트리는곳들도 있고 점점 복잡해지면서 누수를 잡을 수 없을정도로 엉망이 된적이 있는데요, 그때 이후로 직접 관리를 한적이 없네요.

또한 업무상 상용서비스를 개발할때 프레임워크의 도움을 받지않고 만든적은 한번도 없어서 이렇게 해봤다라고 말씀을 드리기 힘드네요.
다만 구현한다고한다면 두 가지 측면을 고려할거같습니다.

커넥션 풀

트랜잭션

매번 연결을 만들게되면 성능상 단점이 있기 때문에 적절한 풀 형태를 구현할거같고, 또한 spring에서 제공하는 선언적트랜잭션 관리를 직접 구현하려면 위에 말씀드린것처럼 트랜잭션 처리를 모든 코드에 다 넣어야될겁니다. 자원이 제대로 해제가 됐는지에 대한 부분도 잘 신경써야겠구요.

그래도 우테코하면서 한번정도는 직접 만들어보면 재밌기도하고 성숙한 상용 모델을 경험할때 도움이 많이 될거같아요.

JunHoPark93 · 2024-03-30T08:15:21Z

추가요구사항을 반영하면서(체스 게임방을 만들고 체스 게임방에 입장할 수 있는 기능을 추가한다., 사용자별로 체스 게임 기록을 관리할 수 있다.) 커넥션 풀 형태로 구현해보기

시간이 되실지 모르겠는데 도전해보면 좋을거같아요.

JunHoPark93

안녕하세요 페드로~
리뷰 반영 잘해주셨습니다.
이번 미션 통해서 많은 고민을 해주셨네요. 시간이 남았다면 추가로 도전해볼만한 부분들 남겨놨으니 도전해보셔도 될거같아요.
저희가 많은 대화를 나눴는데 혹시나 누락됐거나 해결이 되지 않은 부분이 있다면 다시 말씀 부탁드려요~

JunHoPark93 · 2024-03-30T08:19:16Z

추가로 구현 하고싶으신부분이나 이야기하고 싶은 부분있으시면 reopen해주셔도되고 따로 브랜치 올려주셔도됩니다

hw0603 · 2024-04-01T05:56:03Z

추가요구사항을 반영하면서(체스 게임방을 만들고 체스 게임방에 입장할 수 있는 기능을 추가한다., 사용자별로 체스 게임 기록을 관리할 수 있다.) 커넥션 풀 형태로 구현해보기

PR 마감기한(금일 18시)까지 리뷰요청을 드리기에는 시간 상 무리인 것 같아 추가 도전사항들은 미션 종료 후 개인적으로 한번 진행해 보려고 합니다.
이번 단계에서 이것저것 질문을 많이 드렸었는데, 상세히 답변해 주셔서 감사합니다! ☺️

hw0603 added 30 commits March 25, 2024 00:24

refactor(continuousMoveStrategy): 불필요한 static 메서드 제거

b3df817

refactor(board): 메서드 위치 변경

224a769

docs(readme): 3단계 추가 기능 요구사항 반영

16d1110

feat(pieceType): 각 기물 타입이 평가치를 가지고 있도록 변경

fd73746

test(boardTest): Board 의 점수 계산 로직에 대한 테스트 코드 추가

950a8b4

feat(board): 현재 보드의 팀 별 점수를 계산하는 기능 구현

db3cd0f

test(boardTest): 폰이 중복된 세로줄에 있는 다양한 경우 추가

b88656c

refactor(board): 중복된 폰의 평가치 패널티를 계산하는 메서드 분리

cc0a1cc

test(boardTest): 각 점수별 Board 를 구성하는 Map 을 테스트 픽스처 클래스로 이동

f68c2b3

docs(readme): ChessGame 요구사항 추가

bec3147

test(chessGameTest): 게임 상태에서의 팀 관리 로직 테스트 추가

b47b964

feat(gameState): 게임 상태를 표현하는 인터페이스 정의

c962c8a

feat(moveResponse): 말 이동 후 응답 객체 정의

1b09fd1

feat(/state): 게임 상태를 나타내는 구체 클래스 구현

725e7f8

- 상태 클래스 패키지 이름 변경(/status -> /state)

feat(chessGame): 상태를 가지고 기물을 이동시키는 체스 게임 구현

41c7bc8

test(chessGameTest): 킹이 잡혔을 때 게임을 종료시키는 테스트 코드 추가

c2419a2

feat(chessGame): 킹이 잡혔을 때 게임을 종료하고 승자를 판단하는 기능 구현

ac5ef1a

test(chessGameTest): 각 팀별 현재 점수를 계산하는 테스트 코드 추가

c33282b

feat(chessGame): 팀 별 현재 점수를 계산하는 로직 구현

6a69d5a

refactor(gameEnd): 게임 종료 메시지 상수 포장

dfc1a95

refactor(gamePlaying): 게임 진행 중 각 말의 차례를 나타내는 상태 객체 추상화

8f06743

- 상태 객체들을 final class 로 변경

style(chessGameTest): 미사용 import 문 삭제

c2bd7bc

remove(turn): 더 이상 사용되지 않는 Turn 클래스와 테스트 코드 삭제

a39a214

refactor(board): 이동한 위치에 상대편의 기물이 있을 때, 해당 기물의 타입을 반환하도록 변경

6a2807e

refactor(boardDto): 정적 팩토리 메서드 인자 타입 변경(Board -> Map<Position, Piece)

801ae77

feat(chessController): 체스 라운드 진행 기능 구현

9bcc335

feat(outputView): 우승자를 출력하는 기능 구현

8d1c564

feat(outputView): 점수 출력 로직 구현

c17d59d

refactor(chessController): 콘솔 출력시 OutputView에서 출력하도록 변경

33665ae

- 게임 종료 시 재시작 가능하도록 변경

refactor(chessController): 게임 계속 진행 여부를 판단하는 표현식의 메서드 분리

f471cfb

hw0603 marked this pull request as draft March 27, 2024 05:39

hw0603 commented Mar 27, 2024

View reviewed changes

hw0603 marked this pull request as ready for review March 27, 2024 08:58

JunHoPark93 requested changes Mar 27, 2024

View reviewed changes

hw0603 added 14 commits March 28, 2024 16:44

refactor(board): Optional 값에 따라 분기하던 로직을 메서드 체이닝으로 변경

fb43fba

docs(readme): 프로그램 사용 방법 추가

fa6eac2

refactor(board): 명시적 형변환 대신 제네릭 타입을 명시하도록 변경

e122c64

refactor(dbException): 데이터베이스 관련 로직에서 예외 발생 시 던질 커스텀 예외 클래스 정의

5d18908

refactor(dbConnector): DB 연결 오류 시 null을 반환하지 않고 예외를 던지도록 변경

fbce762

refactor(pieceDao): allAll() 호출 시 배치 쿼리를 통해 bulk insert 가 가능하도록 변경

944fb76

refactor(chessGame): 기물 이동 시 상태 객체에 위임하지 않고 Board 에 직접 요청하도록 변경

f2311d3

refactor(dbService): 스트림으로 데이터 형태를 변환하는 로직의 메서드 분리

86efdba

refactor(dbService): 게임 저장 시 트랜잭션 처리

24b05c8

- DAO 의 각 메서드 인자로 Connection 을 전달받도록 변경

refactor(gameDaoTest): GameDao 변경에 따른 테스트 코드 수정

c7ed770

refactor(pieceDao): PieceDao 변경에 따른 테스트 코드 수정

f6ab2ca

fix(pieceDao): DAO 가 아닌 Service 에서 트랜잭션을 관리하도록 수정

d2001d7

fix(/service): 잘못 지정된 service 패키지 경로 수정

d5b904a

test(dbService): 게임 저장/로드 로직 테스트 추가

9f390a3

hw0603 commented Mar 29, 2024

View reviewed changes

hw0603 added 2 commits March 29, 2024 18:02

add(/resource): 테이블 생성 SQL 추가

ea4a08f

docs(readme): 프로그램 실행 방법 보충

45fffe5

hw0603 force-pushed the step2 branch from dd6d82a to 45fffe5 Compare March 29, 2024 15:39

JunHoPark93 reviewed Mar 30, 2024

View reviewed changes

JunHoPark93 approved these changes Mar 30, 2024

View reviewed changes

JunHoPark93 merged commit 510af26 into woowacourse:hw0603 Mar 30, 2024

		@@ -1,30 +1,36 @@
		package domain.game;

		import domain.position.File;


		TeamColor currentTurn();

		MoveResponse move(Board board, Position source, Position destination);

[3, 4단계 - 체스] 페드로(류형욱) 미션 제출합니다. #730

[3, 4단계 - 체스] 페드로(류형욱) 미션 제출합니다. #730

Conversation

hw0603 commented Mar 27, 2024 • edited Loading

hw0603 commented Mar 27, 2024 • edited Loading

Record의 캐싱

상태패턴에서 항상 모든 동작을 상태객체로 위임해야 하는가?

상태패턴에서 상태 전이의 책임은 어느 객체에 있는가?

Optional의 올바른 사용

명령-쿼리 분리 원칙

DAO 객체에 대한 테스트 격리, 테스트 멱등성 유지 + 트랜잭션

DB 저장을 위한 데이터는 어느 계층까지 전파되어도 괜찮은가?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunHoPark93 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

hw0603 Mar 28, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Mar 28, 2024 • edited Loading

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

hw0603 commented Mar 29, 2024

hw0603 Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

JunHoPark93 Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunHoPark93 Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

JunHoPark93 commented Mar 30, 2024

JunHoPark93 left a comment

Choose a reason for hiding this comment

JunHoPark93 commented Mar 30, 2024

hw0603 commented Apr 1, 2024

hw0603 commented Mar 27, 2024 •

edited

Loading

hw0603 commented Mar 27, 2024 •

edited

Loading

`Optional`의 올바른 사용

hw0603 Mar 28, 2024 •

edited

Loading

hw0603 Mar 28, 2024 •

edited

Loading

hw0603 Mar 29, 2024 •

edited

Loading

JunHoPark93 Mar 30, 2024 •

edited

Loading

JunHoPark93 Mar 30, 2024 •

edited

Loading