[🚀 사이클2 - 미션 (기물 확장 + DB 적용)] 라텔 미션 제출합니다.

[bin1225]

안녕하세요 에코.
사이클 2 리뷰도 잘 부탁드립니다!

변경 및 추가 사항 관련해서 개요와 간단한 코멘트 남깁니다. 리뷰 과정에서 참고해주세요:)

변경 및 추가 개요

데이터베이스 스키마 구조

‎src/main/resources/schema.sql‎

기물 이동 규칙

궁성 내 대각선 이동에 대해서 "직선 이동 기물의 경우 격자점에 대각선이 있으면 해당 방향으로 이동가능하다" 라고 해석했습니다.

그래서 격자점을 의미하는 Intersection 객체를 추가하고 이동 가능한 방향과 거리를 가지는 Vector 리스트를 가지도록 구현했습니다.

직선 이동 기물(궁,사,차,포,졸)이 공통적으로 StraightRouteProvider를 사용해서 경로를 찾고, 각 기물의 특징에 의한 추가 검증은 기물 내에서 진행하도록 하였습니다.

게임 종료 규칙

• 게임이 끝나는 시점은 다음 두가지로 구성했습니다.
  • 양팀의 기물 점수 합이 모두 30점 이하인 경우 점수를 기준으로 승패 판별
  • 궁이 잡힌 경우

게임 진행 상태에 상태패턴을 도입해보았습니다.

• 관련 패키지 : domain/state

진행중인 상태에서 종료 기준에 따라서 종료 상태로 스스로 변경되는 것이 자연스럽다고 생각했습니다.
또 현재는 궁과 궁이 마주친 상태에서 1턴이 지나는 경우 무승부 처리되는 기능이 존재하지 않지만, 추후 그러한 기능들이 추가되는 경우에도 상태 패턴이 적용되어 있다면 변경이 용이할 것 같다고도 생각하여 적용했습니다.

DB 적용

진행중인 게임이 존재한다면 게임을 이어서 진행하거나, 새로운 게임을 생성하도록 선택할 수 있게 구현했습니다.

게임 진행 도중 종료하는 기능은 따로 구현되어 있지 않아서 강제로 프로그램을 종료시켜야 하는 상태입니다.

리뷰 포인트

데이터베이스

사실 이번 사이클에서 다른 크루들과 다르게 데이터베이스 적용 파트가 가장 난해하다고 느껴졌습니다..

기존 Controller 역할을 하던 JanggiFlow에서 담당해야 하는 역할과 새로 추가한 JanggiService 에서 담당해야 하는 역할에 대한 기준이 명확하지 않았서 그랬던 것 같아요.

또 일반적인 서버는 요청마다 정보를 DB에서 조회하고 처리하고 저장하는 흐름이지만, 이번 사이클에서는 처음에만 Board정보를 조회하고, 이후 이동 시에는 이동 정보만 업데이트 하는 방식이라
메모리 상에 존재하는 Board를 업데이트 한 후 추가로 데이터베이스를 업데이트 하는 구조가 어색하다고 느껴졌던 같습니다.

DB 적용 과정에서 추가한 Service 클래스와 DAO 관련 로직에 결함이 가장 많을 것이라고 예상이 됩니다 ㅎㅎ

테스트 코드

테스트 코드가 원래도 참 어렵다고 생각했는데, 데이터베이스를 추가하고 테스트를 작성하려고 하니 더 복잡해지는 것 같다고 느껴집니다.

더미 데이터 초기화, 트랜잭션 관리로 인해 테스트 코드가 지저분해지는데 개선 방향성이 잘 떠오르지 않는 것 같습니다. 이번 미션이 끝나면 개인적으로 테스트 코드 관련해서 추가 학습이 필요하다고 생각됩니다.

문제가 너무 많을 것 같지만, 치명적이라고 생각되는 부분이 있으면 피드백 부탁드립니다!

학습

이번 장기 미션을 마지막으로 레벨 1이 마무리 되는데, 매 사이클마다 무지함을 새롭게 상기하는 것 같습니다.. 특히 이번 DB 적용은 간단할 거라 생각했는데 복잡하다고 느껴져서 더 그랬던 것 같아요.

에코가 리뷰하는 과정에서 명확히 보이는 제 부족한 부분이 있다면, 보완하기 위한 방향을 추천받을 수 있을까요?

또는 에코가 생각하시기에 지금 제 단계에서 시간을 투자해볼 만한 요소들이 있다면 공유해주시면 감사하겠습니다.

체크 리스트

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

[bin1225]

현재는 차, 포 같은 직선 이동 기물이 대각선 이동 시 궁성을 벗어날 수 없는 상태이지만, 만약 벗어날 수 있게 되는 경우를 고려해서 현재와 같은 구조를 선택했습니다.

• 궁성 내에서 이동하는 규칙을 새로 만들고, 현재 위치가 궁성일 때 해당 규칙을 사용하게 만드는 것보다 지금처럼 격자점이 방향과 거리를 가지는 것이 변화에 유연하다고 생각했습니다.

[bin1225]

Board에서 Map의 value값으로 Piece 를 가지던 것을 Intersection으로 감싸서 가지도록 했습니다.

이렇게 구조를 변경하니 결국 Intersection 에 있는 Piece, Location을 사용하기 위해서 모두 getter를 정의하게 됐습니다.

Intersection을 격자점 내 정보를 모아두는 바구니같은 존재로 생각하면 적절하다고 생각되는데, 객체지향의 원칙 중 행동을 해당 객체에게 위임하라는 측면에서는 적절하지 않을 것 같다고도 생각됩니다.

혹시 에코의 의견이 있다면 공유해주시면 감사하겠습니다!

[bin1225]

현재 진행중인 게임의 점수와 차례를 가지는 상태입니다.
기물 이동시 도착지에 있던 기물을 board.move()메서드에서 반환하도록 하였고, 해당 반환값을 GameState.update()의 매개변수로 전달합니다.

삭제된 기물을 기준으로 게임 점수를 업데이트 하고, 궁이 잡혔는지 확인하는 등 게임 종료 여부를 판단하도록 구현했습니다.

[bin1225]

컨트롤러와 필요한 데이터를 주고 받기 위한 DTO입니다.

[bin1225]

Service 계층에서 트랜잭션을 시작하고, 해당 커넥션을 DAO에 메서드마다 매개변수로 전달하지 않고 사용하기 위해서 TreadLocal을 활용했습니다.

[bin1225]

각 직선 이동 기물이 자신이 이동 가능한 최대 거리를 가지도록 하였습니다.
RouteProvider를 통해 받은 경로가 이동 가능 거리보다 길다면, 이동할 수 없다고 판단합니다.

[bin1225]

기존 보드 생성 로직을 그대로 활용하기 위해 DB에서 조회한 데이터를 Map<Location, Piece> 구조로 변환하고, DBArrangementStrategy 사용하여 Board를 초기화하도록 구현했습니다.

[echo724]

안녕하세요 라텔!
이번 사이클도 깔끔하게 잘 해주셨네요! DB 사용이 어렵다고 하신 것에 비해서 생각보다 트랜젝션도 깔끔하게 잘 처리해주셨고 공성 부분도 Intersection 활용하셔서 잘 해결 하신 것 같습니다! 변경 사항도 보니 격자점을 위한 변경을 제외하고는 크게 구조가 바뀐 것 같지않구요! 딱봐도 미션 자체가 어려웠을 것 같은데 고생 많았습니다! ㅠ

일단 미션 깔끔하게 잘 짜신 것 같아서 크리티컬한 피드백은 따로 없을 것 같구요, 다만 고민해볼만한 부분, 추가로 학습해보면 좋은 부분들 커멘트로 남겼습니다 확인해보시면 좋을 것 같아요. 추가로 PR 설명에 남기신 질문 답 드리면:

1. 데이터베이스 업데이트 구조

• 일단 라텔이 이미 갖고 있는 고민처럼 현재 제가 느끼기로는 비지니스 로직을 담당하는 도메인과 영속화를 위한 엔티티가 너무 따로 노는 느낌이 들었습니다. 인메모리 상태로 움직임을 확인 한 다음에 움직임이 성공하면 게임 상태와 커밋을 하더라구요. 해당 구조가 아무래도 DB추가 요구사항에 맞춰서 설계가 그렇게 진행된 것 같아요. 말씀하신 대로 해당 로직이 나쁜 것?은 아니지만 그래도 장애 처리 부분과 도메인 결합도로 봤을때는 많이 떨어지는 것 같습니다. 어떤 정보가 진실원인지 (DB에 있는 피스 정보가 진실인지, 아니면 인메모리상의 정보가 진실인지), 장애 났을 경우에 DB와 인메모리 상태 차이는 어떻게 날 수 있으며 어떻게 해결할 수 있을지 등 현재 구조로 부터 오는 문제점들은 한 번 고민해보시면 도움이 될 것 같습니다.|

2. 테스트 코드
   마찬가지로 현재 DB가 추가되면서 테스트 범위나 방식에 대한 고민이 좀 생기셨을 것 같아요. 두 가지 키워드로 추가 학습 해보시면 도움이 될 것 같은데 1. 관심사 분리: TransactionManager처럼 비지니스 로직과 인프라나 트랜젝션을 위한 로직을 분리해두셨는데, 테스트도 그에 따라 각각의 관심사를 제대로 테스트 해주면 좋을 것 같습니다. (이미 잘 해두신 것 같지만요) 2. 테스트 환경 분리: 현재 프로덕션 환경이 따로 없기 때문에 In-memory db로만 테스트와 실제 프로그램 환경 모두 동일하게 해두셨더라구요. 이 부분 미션에서 살짝 벗어날 수 있지만 간단하게 어떻게 하면 테스트 환경과 프로덕션 환경을 분리할 수 있을까 고민해보시면 좋을 것 같아요.

이정도가 답이 될 것 같습니다! 커멘트 고민해보시고 리뷰 요청 다시 주세요. 화이팅입니다!

[echo724]

DB 사용이 어렵다 하신 것 치고는 관심사 분리(트랜젝션은 트랜젝션 매니저가 비지니스/도메인 로직은 도메인이나 서비스가)를 매우 깔끔하게 잘 하셨습니다?! 혹시 전에 사용하셨거나 따로 경험이 있으신가요?

[echo724]

다만, 확인해보니 executeInTransaction 메서드들 모두 transactionManager에 public 메서드로 만들고 나머지 isNotActive, commit, rollback, close 메서드들 내부 접근자로 바꿀 수 있을 것 같은데 어떻게 생각하세요? 딱히 메서드에서 다른 객체를 의존하지 않아서 transactionManager로 넣어도 좋을 것 같습니다. 의미상으로도 DefaultJanggiService보단 transactionManager가 담당하는게 나을 것 같아서요!

[bin1225]

DB 사용이 어렵다 하신 것 치고는 관심사 분리(트랜젝션은 트랜젝션 매니저가 비지니스/도메인 로직은 도메인이나 서비스가)를 매우 깔끔하게 잘 하셨습니다?! 혹시 전에 사용하셨거나 따로 경험이 있으신가요?

너그럽게 평가해주셔서 감사합니다 ㅎㅎ
스프링 프레임워크에서 @Transactional 어노테이션으로 서비스 레이어에서 트랜잭션 관리를 분리하는 걸로 알고 있었고, 그렇게 하는 것이 책임 분리 관점에서 옳다고 생각했습니다.
구현은 ai를 많이 참고 했습니다..

다만, 확인해보니 executeInTransaction 메서드들 모두 transactionManager에 public 메서드로 만들고 나머지 isNotActive, commit, rollback, close 메서드들 내부 접근자로 바꿀 수 있을 것 같은데 어떻게 생각하세요?

에코 의견에 동의합니다.
트랜잭션의 실행 흐름 관리도 transactionManager에서 수행하는 것이 옳은 것 같습니다. 또 Service 계층 구현체가 추가되는 경우 재사용이 가능하다는 면에서도 이동 시키는 것이 정답이라고 생각해요.

commit, rollback 과 같은 메서드도 노출시켜두는 것이 바람직해보이지 않는 것 같습니다. 동시에 여러 로직이 실행될 때 (이미 이런 부분에서 문제가 되는 지점이 100가지 정도 될 것 같지만요), 해당 메서드들이 호출되는 경우 예상하지 못한 동작이 발생할 수 있을 것 같습니다.

결론

• TransactionManager 내부로 executeInTransaction() 메서드를 이동 시켰습니다.
• 테스트 코드에서 더미 데이터를 초기화하여 테스트간 종속성을 없애기 위해 begin, rollback, close 메서드를 @BeforeEach, @AfterEach 에서 호출했는데, 이 로직을 유지하기 위해서 TransactionManger 를 인터페이스화 하고, 테스트에서는 TestTransactionManager 를 사용하도록 했습니다. 사실 이렇게 구현한 게 적절한지 확신이 서지 않는 것 같네요..

[echo724]

장기말 옮기기 -> 옮긴 장기말 업데이트 -> 개임상태 업데이트 이런 식으로 진행하는 걸로 보이네요? 일단은 깔끔하게 로직이 잘 보이긴 하는데 몇 가지 고민하실 부분이 있어 보이네요! 만약 인메모리 상(영속화 제외한 부분)에서는 성공적으로 진행이 됐는데, pieceDao에서 피스를 찾거나 업데이트 하는 과정에서 에러가 발생하면 어떻게 될까요? Timeout이 발생할 수 도 있고, 커넥션에 문제가 생겼을 수 도 있구요. 다양한 이유로 비지니스 로직이 영속화가 안될 수 있는데 이런 장애 상황 등을 어떻게 대비할 수 있을까요? 또한 만약 인메모리 상에서는 보드(교차점)에 해당 피스가 있는데, pieceDao에서는 못찾는 경우도 있을까요? 이렇게 엔티티와 도메인 객체가 분리되었을 경우에는 검증해줘야할 부분들이 있습니다. 이 부분 1. 현재 구조가 장애 상황 혹은 상태 불일치 상황에 괜찮은지 2. 아니라면 어떤 방식으로 처리할 수 있을지 -> 도메인 엔티티로 설계 변경? 혹은 재시도? 등등 고민해보시고 적용해보시거나 커멘트로 알려주시면 좋겠네요!

[bin1225]

먼저 데이터베이스 관련 예외가 발생했을 때도 재시도 로직에 포함되도록 JanggiException을 상속받은 커스텀 예외를 정의하고 적용했습니다.

1. 데이터 베이스 연결 관련 문제도 프로그램을 종료시키는 것보다는 사용자에게 알리고, 다시 재시도 할 수 있도록 처리하는 것이 좋다고 생각했습니다.

2. 프로그램 시작 자체가 불가능한 경우, 즉 시작 시점에 필요한 데이터를 로딩할 수 없는 경우는 따로 재시도 로직 없이 실행이 실패하는 구조입니다.

이렇게 수정하니 에코가 말씀하신대로 불일치 문제가 발생하는 것을 확인했습니다.

아래 코드와 같이 메모리에 있는 보드를 업데이트 한 후 예외를 발생시키는 경우, 데이터베이스 쿼리를 롤백이 되지만, 메모리 데이터는 롤백 되지 않습니다. 여기서 메모리 데이터와 DB 데이터의 일관성이 깨집니다.

        transactionManager.executeInTransaction(() -> {
            Long gameId = gameInformation.getGameId();
            Board board = gameInformation.getBoard();
            Piece removedPiece = board.move(from, to);

            Optional<PieceEntity> fromPiece = pieceDao.findByGameIdAndLocation(gameId, from);
            Optional<PieceEntity> toPiece = pieceDao.findByGameIdAndLocation(gameId, to);
            fromPiece.ifPresent(pieceEntity -> pieceDao.updatePosition(pieceEntity.getId(), to));
            toPiece.ifPresent(pieceEntity -> pieceDao.deleteById(pieceEntity.getId()));

            gameInformation.update(removedPiece);

            if (gameInformation.isInProgress()) {
                gameDao.updateTurn(gameId, gameInformation.getCurrentSide().name());
                throw new JanggiException("임의로 예외 발생!");
            }
        });

또 이 경우 재시도 로직이 실행되기 때문에 view 레벨에서 캐싱해둔 보드가 출력됩니다.

public class ApplicationView {

    private final Output outputWriter;
    private final Input inputReader;

    private List<List<String>> lastBoard;
    private String lastSide;

결과적으로 사용자 입장에서는 제대로 작동하지 않는 프로그램을 경험하게 되는 것을 확인했습니다.

---

해결 방법을 생각해봤을 때,

1. DB 먼저 업데이트 후 성공 시 메모리 업데이트
2. 보드를 메모리에서 유지하지 않고 항상 DB에서 조회

지금 상황에서는 1번 방식이 가장 간편하게 적용할 수 있는 방법인 것 같습니다. 하지만 이 부분은 코드 작성하는 과정에서 개발자가 매번 신경써야 한다는 점에서 문제인 것 같습니다. 또 로직이 복잡해지는 경우 완벽한 통제가 불가능 하네요.

2번 방식을 적용하기 위해서는 스키마, DAO 로직에 많은 수정이 필요할 것 같습니다.

• 보드 생성 시 궁성 위치 초기화 전략도 지정이 필요하기 때문에, 해당 전략에 대한 정보도 DB에 저장 필요합니다.
• 도메인 로직에 필요한 모든 데이터를 매번 DB에서 조회해서 사용하고 출력해야 할 것 같습니다.

---

결론

DB 관련 예외 발생 시, 초기 로딩 시점이 아니라면 재시도 로직이 적용되도록 커스텀 예외 도입하였습니다.

장애 발생 시 메모리와 DB의 동기화를 보장하기 위해, DB 관련 로직을 수행하고 메모리 데이터를 업데이트 하도록 하였습니다.

완벽하지 않은 상태이고 메모리 상에서 데이터를 유지함으로써 얻는 조회 비용 감소의 이점보다 데이터 동기화 관련해서 안정성이 떨어지는 것이 더 크다고 생각됩니다.
또 현재 GameInformation DTO가 객체를 내부적으로 가지고 controller와 service 계층을 왔다 갔다 하는 것도 메모리 내부에서 정보를 유지하기 때문이라고 생각됩니다.

완벽하게 해결하기 위해서는 매 로직마다 필요한 정보를 데이터베이스에서 조회하는 것이 적절하다고 생각합니다.

구조 변경이 많이 필요할 것 같아서 조금 더 고민해볼 시간이 필요할 것 같습니다.

너무 좋은 리뷰 감사드립니다!

[echo724]

캬 공성 짜기 쉽지 않은데 그래도 잘 표현해내셨네요 👍

[echo724]

제 생각입니다만, 현재 GameInformation, GameContext 생성 시점, 사용되는 위치 전부 비슷하게 사용되고 있는데 이 부분 합치는거 고려해봐도 좋을 것 같아요. 한쪽이 DTO이긴 하지만 결국에 둘이 같은 라이프사이클로 같이 움직인다면 충분히 같은 객체에 둬도 되지 않을까 생각합니다.

[bin1225]

에코가 말하신대로 생성 시점도 동일하고, Service 계층 메서드 호출 시에도 GameContext와 GameInformation이 함께 전달되는 구조인 것을 확인했습니다.

그래서 GameInformation에 GameContext 필드를 추가했습니다.

이렇게 수정하니 새로운 문제를 발견했습니다.
GameInformation을 DTO 목적으로 정의했는데, 내부에 GameContext, Board 와 같은 객체를 가지게 되니, DTO인지 도메인인지 경계가 모호해진다고 느낍니다.

단순 getter로 객체를 제공하고 DTO로써 역할을 명확히 하면 디미터의 법칙을 위반하고 호출부에서 다음과 같은 형태로 호출이 필요합니다.

gameInformation.getGameContext().update();

그래서 현재는 GameInformation 내부에 gameContext를 호출해주는 서빙 메서드를 두었습니다.

public void update(Piece removedPiece) {
    gameContext.update(removedPiece);
}

하지만 이 구조는 DTO가 도메인처럼 행동하기에 DTO를 정의했던 목표가 사라지고 컨트롤러와 서비스가 결국 결합된다고 생각됩니다.

결론
작동 구조 설계가 잘못되었나 생각이 드는 상태입니다.
매 턴마다 보드 정보를 조회하는 비용을 아끼고자 메모리에서 보드, 게임 턴 상태를 유지하고 업데이트 쿼리만 호출하도록 하였습니다. 이러한 구조때문에 Controller와 Service가 도메인 객체를 주고 받게되고, 그래서 DTO에 객체가 그대로 들어가며 역할이 모호해지는 것 같습니다.

이 부분에 대해서 지금 떠오르는 방법은 다음 두가지입니다.

1. 조회 비용을 아끼기 위해서 Service 계층에서 보드 및 턴 정보를 필드로 가지고 캐싱해서 사용한다.
2. 매 실행 시마다 DB에서 보드 정보를 조회한다.

단일 호스트에서 실행되는 지금 구조라면 1번도 문제가 되지 않을 것 같은데,
에코가 보시기에는 어떤 방향이 더 적절해보이시나요?

[echo724]

resolveRoute라는 이름이 이해가 잘 안되는데요, calculate에서 변경하신 이유가 따로 있을까요?

[bin1225]

기물 경로 계산에서 중복되는 예외 처리 로직을 제거하기 위해 템플릿 메서드 패턴을 적용했습니다!

---

기존에는 기물의 이동 경로 계산 과정에서 예외 발생 시 기물이 무엇인지 정보를 주기 위해 기존 코드에서 다음과 같이 예외를 잡아서 구체화했습니다.

    @Override
    public List<Location> calculateRoute(Location from, Location to) {
        try {
            return ROUTE_PROVIDER.calculateRoute(from, to);
        } catch (RouteResolveException e) {
            throw new RouteResolveException(pieceType, from, to);
        }
    }

이렇게 try-catch로 감싸는 로직이 모든 기물에 중복된다고 생각되어서 템플릿 메서드 패턴을 적용해보았습니다.

실제 이동 가능한 기물의 부모인 ActivePiece 에 다음과 같이 정의하여 예외 처리를 공통 처리하고, 자식 클래스에서 resolveRoute() 를 구현하도록 강제했습니다.

    @Override
    public final List<Location> calculateRoute(Intersection from, Intersection to) {
        try {
            return resolveRoute(from, to);
        } catch (RouteResolveException e) {
            throw new RouteResolveException(pieceType, from.getLocation(), to.getLocation());
        }
    }

    protected abstract List<Location> resolveRoute(Intersection from, Intersection to);

사실 이 부분 관련해서도 적절한 코드인지 확신이 서지 않습니다.
코드를 보는 입장에서 Piece 인터페이스의 calculateRoute가 자식 클래스에 구현이 되어 있지 않기 때문에 직접 부모 클래스를 확인하면서 작동을 이해할 필요가 있다는 점이 문제가 되는 것 같습니다.

반대로 부모 클래스에서 예외 처리 템플릿 메서드를 protected로 제공할 수 있지만, 이 경우에는 자식 클래스를 구현할 때 해당 메서드를 사용해야 하는 것에 대해서 알 수 없기 때문에, 구현의 일관성이 깨질 수 있을 것 같습니다.

// 부모 클래스 (ActivePiece)
    protected List<Location> calculateWithExceptionHandling(Intersection from, Intersection to, Supplier<List<Location>> routeSupplier) {
        try {
            return routeSupplier.get(); // 자식 클래스에서 전달한 람다(실제 계산 로직) 실행
        } catch (RouteResolveException e) {
            // 공통 예외 처리 및 구체화
            throw new RouteResolveException(pieceType, from.getLocation(), to.getLocation());
        }
    }

    // 자식 클래스 (예: Cha, Sang 등)
    @Override
    public List<Location> calculateRoute(Intersection from, Intersection to) {
        // 부모의 protected 메서드를 명시적으로 호출하며 람다로 실제 로직 전달
        return calculateWithExceptionHandling(from, to, () -> {
            return ROUTE_PROVIDER.calculateRoute(from, to); 
        });
    }

혹시 에코가 보시기에 적절하지 않다고 판단되시면 의견 공유 부탁드립니다 :)

[bin1225]

에코 안녕하세요 🌵
리뷰 감사합니다! 덕분에 필요한 고민들 많이 해볼 수 있었어요. 도움 많이 됐습니다.
피드백 관련해서 수정하고 고민해본 내용들 코멘트에 정리해두었습니다.
여유 되실 때 확인 해보시고, 답변이 필요하다고 생각되시면 답변해주시면 감사하겠습니다!
바쁘신 일정에 시간 내어 리뷰해주셔서 감사드려요.