Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3, 4 단계 - 체스] 이든(최승준) 미션 제출합니다. #815

Merged
merged 55 commits into from
Apr 4, 2024

Conversation

PgmJun
Copy link

@PgmJun PgmJun commented Apr 1, 2024

안녕하세요 알렉스 이든입니다! 🙌

체스 미션에 DB를 적용해서 구현하려니 너무 어지러웠습니다...! 🥲
그래서 구현에 조금 많은 시간이 투자되었네요..!

이번 리뷰도 잘 부탁드립니다!!

주요 구현 사항

DB 적용

  • DB 커넥션풀 구현
    • DAO에서 요청을 보낼 때마다 Connection을 생성하는 것은 큰 비용이 들기 때문에,
      애플리케이션 실행 시에 미리 커넥션을 만들어 사용할 수 있는 커넥션 풀을 공부차원에서 구현해보았습니다.
  • 트랜잭션 적용
    • 데이터 처리 작업을 안전하게 수행하기 위해 트랜잭션을 적용하였습니다!
    • 트랜잭션 공통 로직 분리를 위해 TransactionManager 클래스 구현하였습니다.

관련 코드는 chess/infra/db/transaction 패키지에서 확인하실 수 있습니다!

재입력 로직 구현

  • 실패 시, 게임이 종료되어 다시 실행시켜야 이어할 수 있는 부분이
    유저 입장에서 좋지 않을 것이라고 생각되어 재입력 로직을 구현하였습니다.

하지만 재귀를 사용하여 구현하였기 때문에, 재귀의 깊이가 깊어지면 예외가 발생할 여지가 존재하기 때문에
이 부분은 리팩토링 하며 처리해보겠습니다


고민사항

구현하면서 아래와 같은 고민사항이 있었습니다.

도메인 객체와 DB 테이블의 데이터 구조가 일치하지 않는 문제

이전에 만들었던 도메인 객체와 DB의 테이블을 매핑하여 간단한 CRUD 로직을 만들고 싶었으나,
그렇게 하려면 코드에 수정해야할 사항이 너무 많이 생겨 Entity 클래스를 만들어 해결하였습니다.

엔티티 클래스는 DB 테이블과 완전히 동일한 데이터를 가지도록 하였으며, CRUD 작업에 사용됩니다.

하지만 저는 이렇게 관리하는 방식이
도메인과 DAO 사이에 한 계층을 추가시키는 것 같아 코드의 복잡도를 상승시키는 것 같습니다..
혹시 알렉스는 이 부분에 대해 어떻게 생각하시는 지, 그리고 어떻게 개선했으면 좋겠는 지 의견 주시면 적극 반영해보겠습니다 🥲

게임의 커맨드들은 어떻게 관리해야할까?

이 부분은 Command 패턴을 적용해서 해결하였습니다.
Controller에서 굉장히 복잡한 분기처리를 수행해야했던 이전 커맨드 관련 코드에
커맨드 패턴을 적용하여 각각의 커맨드 구현체에게 책임을 분산시켰습니다.
이를 통해 분기처리를 제거할 수 있었습니다.

하지만 이 로직도 제대로 구현한 것인지 의구심이 들며 만족스럽지 않은 느낌이 계속해서 듭니다..
알렉스가 피드백을 주신다면 이 부분도 적극 반영해보겠습니다!!

- Result 객체 생성
- 무승부 추가
- 왕의 생존 여부로 게임 결과 판단
@PgmJun PgmJun changed the base branch from main to pgmjun April 1, 2024 08:58
@PgmJun PgmJun changed the title [Step3,4] 이든(최승준) 제출합니다. [3, 4 단계 - 체스] 이든(최승준) 미션 제출합니다. Apr 1, 2024
Copy link

@yxxnghwan yxxnghwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 이든! 이번 단계도 잘 구현해주셨네요! 👍🏿
전체적으로 코드가 깔끔해서 코멘트가 많지 않습니다!
확인 부탁드려요!

Comment on lines 8 to 33
public class DBConnectionPool {
private static final int MAX_CONNECTION_SIZE = 3;
private static final Deque<Connection> CONNECTION_POOL;

static {
CONNECTION_POOL = new ArrayDeque<>();
for (int i = 0; i < MAX_CONNECTION_SIZE; i++) {
try {
CONNECTION_POOL.add(DBConnectionGenerator.generate());
} catch (SQLException e) {
throw new RuntimeException(e);
}
}
}

public static Connection getConnection() {
if (!CONNECTION_POOL.isEmpty()) {
return CONNECTION_POOL.pop();
}
throw new RuntimeException("현재 남아있는 커넥션이 없습니다.");
}

public static void releaseConnection(final Connection connection) {
CONNECTION_POOL.add(connection);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커넥션 풀은 멀티 스레드 환경을 고려해서 사용하게 될 것 같은데요! 이 클래스 스레드세이프한 클래스일까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 고려한 부분에 대해서 너무 좋은 리뷰인 것 같습니다!!
스레드 세이프하도록 변경해보겠습니다! 감사합니다:)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syncronized 키워드를 적용해주셨네요! 추후 자바에서 동시성을 제어하기 위해 제공하는 기능들에 대해서도 공부해보시면 좋을 것 같아요! 👍🏿

@@ -30,3 +30,4 @@ out/

### VS Code ###
.vscode/
/docker/db/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실행해보려면 init.sql이 필요할 것 같아서 docker/db/mysql/data 만 ignore처리 부탁드려요!

Copy link
Author

@PgmJun PgmJun Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗,,! 제가 설명을 달아놓지 않았네요,,!
docker-compose up 수행하신 뒤에 resources/schema.sql 디렉토리에 있는 쿼리 전부 복사하셔서 실행시켜주시면 됩니다!

import chess.domain.position.ChessRank;
import chess.domain.position.Position;

public class PieceEntity {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

분명 존재하는 방식이긴 하지만, 개인적으론 생산성 저하가 단점으로 다가오곤 합니다! 상황에 따라 다른 것 같네요!

DB의 엔티티와 도메인엔티티를 같은 클래스로 사용할 경우 도메인이 DB에 의존하게 구현되어야하는 단점이 분명 존재합니다.
코어한 도메인 로직을 보호하기 위해 DB접근을 위한 엔티티를 따로 만들어서 사용할 경우엔 생산성의 단점이 존재하구요.

제 경험상 DB를 걷어내거나 하는 경우는 흔치 않았던 것 같아서 저는 그냥 같은 클래스로 묶어서 사용하곤 합니다 :)

Copy link
Author

@PgmJun PgmJun Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 코드의 복잡성이 증가하고 생산성이 저하되는 문제가 더 크다고 생각해서 알렉스 방식에 동의하지만
DB 설계를 먼저한 뒤에 개발을 시작한 것이 아니라, 개발 도중 DB를 설계하고 연결하려고 하니
도메인 객체의 필드와, 데이터베이스 테이블의 컬럼이 일치하지 않아 둘을 연결시키는 것이 굉장히 복잡하네요..


스크린샷 2024-04-04 00 05 49

DB에서는 위와 같이 기물이 위치 정보를 가지고 있지만,
자바 코드에선 Rank,File 을 포함한 기물의 위치 정보를 ChessBoard 객체가 가지고 있어서
DB 테이블과 객체를 매핑하기가 굉장히 어려웠습니다..

분명 두 방식의 장단점이 존재하지만, 저는 현재 설계에는 Entity 클래스를 활용하는 방식이 적합한 것 같다고 생각이 되어 그대로 채택하겠습니다! 🙂

각 방식의 장단점을 비교해주신 좋은 답변 감사드립니다 알렉스!!

Comment on lines 13 to 16
private PieceType type;
private PieceColor color;
private ChessRank rank;
private ChessFile file;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB접근을 위한 클래스니까 DB자료형에 맞는 원시 타입들은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그게 맞는 것 같습니다!
변경해보겠습니다:)

import java.util.List;
import java.util.Optional;

public class GameDAO implements GameRepository {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DAO 라고 작성해주신 클래스의 거의 모든 메소드가 다음과 같은 패턴인 것 같아요.

  1. 커넥션 맺기
  2. prepareStatement 세팅
  3. 쿼리실행 or 쿼리 실행결과 매핑 및 반환

이러한 패턴인데 이 중복되는 패턴을 어떻게하면 줄일 수 있을까요? 🤔

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작성해주신 TransactionManager라는 클래스에 힌트가 있는 것 같네요! 잘 부탁드립니다 :)

Copy link
Author

@PgmJun PgmJun Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패턴은 비슷하지만 중복되는 코드는 생각보다 적은 DAO로직을
TransactionManager와 비슷한 방식으로 처리하기에는 제 지식이 많이 부족했던 것 같습니다 🥲

대신 Fluent API 방식을 사용해서 중복되는 코드를 메서드 체이닝 형태로 풀어내보았습니다.
혹시 이 방식은 어떠신지, 그리고 개선할 점은 뭐가 있을 지 남겨주시면 잘 반영해보겠습니다!

infra/db/query 패키지 내부에 QueryManager라는 클래스입니다!

Before

@Override
    public Long add(Connection conn, GameEntity game) throws SQLException {
        try {
            PreparedStatement preparedStatement = conn.prepareStatement(
                    "INSERT INTO game (turn) VALUES(?)",
                    Statement.RETURN_GENERATED_KEYS
            );
            preparedStatement.setString(1, game.getTurn().now().name());
            preparedStatement.executeUpdate();

            ResultSet generatedKeys = preparedStatement.getGeneratedKeys();
            generatedKeys.next();

            return generatedKeys.getLong(1);
        } catch (SQLException e) {
            throw new RuntimeException(e);
        }
    }

After

public Long add(Connection conn, GameEntity game) throws SQLException {
        ResultSet generatedKeys = QueryManager
                .setConnection(conn)
                .insert("INSERT INTO game (turn) VALUES(?)")
                .setString(1, game.getTurn().now().name())
                .execute()
                .getGeneratedKeys();

        generatedKeys.next();
        return generatedKeys.getLong(1);
    }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 단계에서 할 수 있는 충분한 고민을 해주신 것 같아요!
추후 미션에서 스프링을 통해 DB를 접근하는 코드들을 짜게 될텐데 그때 스프링에서 제공하는 JdbcTemplate이라는 클래스의 코드를 참고해보시면 도움이 될듯 합니다!

@PgmJun
Copy link
Author

PgmJun commented Apr 3, 2024

안녕하세요 알렉스! 이든입니다
벌써 두 번째 리뷰요청 드리게 되었네요!
남겨주신 리뷰들 하나하나 소중하게 읽어보고 반영해보았습니다!

변경사항

1. Fluent API 방식 적용

#815 (comment)

2. DBConnectionPool 동시성 문제 해결을 위해 syncronized 키워드 적용

동시성 문제가 발생할 수도 있던 DBConnectionPool의 connection 관련 로직을
syncronized 키워드 적용을 통한 동기 처리를 통해 해결책을 제시해보았습니다!

제가 아는 선에서는 이 방법이 최선이었는데 혹시 이 보다 더 간결하고 좋은 방법이 있는지 궁금합니다!

3. Entity 클래스의 필드 자료형을 DB 테이블 컬럼 자료형과 최대한 매칭되도록 변경

Entity 클래스는, DB 테이블의 데이터를 담을 클래스이기 때문에 알렉스의 의견대로 동일한 형태의 자료형을 사용하는 것이 바람직하다고 생각되어 변경하였습니다!
그리고 도메인 객체로 매핑하는 작업은 Service 객체에서 수행하도록 변경하였습니다.

리뷰 요청 시점까지는 우선 이렇게 구현하였는데,
Service 로직이 비즈니스 로직 이외에도 너무 많은 책임을 가지게 되는 것 같아서
Mapper라는 것을 구현하여 Domain 객체Entity 객체 매핑을 담당시킬 계획입니다


질문 💭

커맨드 패턴 적용을 잘못한 걸까요?

커맨드 패턴을 적용하여 최대한 start, end, status, move 등의 로직을 수행하도록 구현해보고자 하였습니다.
이 커맨드들은, 커맨드 기능을 실행하는 execute 메서드의 return Type이 void이기 때문에 이때까지는 괜찮았습니다.

하지만 game 커맨드를 추가하고, 이에 따른 커맨드 패턴 구현체를 생성하려고 하니
game 커맨드는 execute 메서드에 반환값이 필요하였습니다.

이러한 상황에서 저에게는 2가지 선택지가 있었습니다.

1. 커맨드 execute 메서드의 return Type을 제네릭으로 가지는 새로운 커맨드 Interface 생성

이 방식은 이전 커맨드들과 호환이 되지 않아, 코드의 굉장히 많은 부분을 뜯어고쳐야하는 문제가 있어서 적용이 꺼려졌습니다.

2. 기존 커맨드 Interface에 return Type을 제네릭으로 가지는 새로운 execute 메서드 생성

이 방식은 execute 시에, return값이 필요하지 않은 커맨드들도 이 메서드를 구현해주어야 하기 때문에 좋지 않다고 느껴졌습니다.


스크린샷 2024-04-04 00 34 06

결국 game 커맨드에 대한 처리는 기존 코드처럼 분기처리를 할 수 밖에 없었는데요,,

제가 느끼기에는 이 부분이 처리가 어려운 건 설계 미스인 것 같다고 생각합니다..
때문에 어떻게 개선하면 좋을 지, 알렉스의 의견을 얻어보고자 질문을 드리게 되었습니다!

궁금한 사항에 대해 적다보니 글이 길어져서, 제가 설명을 제대로 못드린건 아닌지 걱정이 되네요..
혹시 제대로 이해가 안되신 부분이나 궁금한 점 남겨주시면 DM으로라도 답변 빠르게 드리겠습니다 감사합니다!

Copy link

@yxxnghwan yxxnghwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 이든! 레벨1 고생 많으셨습니다! 이번 단계에서 고민할만한 부분들을 충분히 고민해주신 것 같아 PR은 이만 머지하겠습니다! 추가적으로 추후 레벨에서 고려해봄직한 것들을 코멘트로 남겨두었는데요! 여유가 되신다면 한번 살펴보아도 좋을 것 같습니다! (굳이 살펴보지 않아도 이후 레벨에서 결국 만나게 되긴 할 것 같네요..😅) 다음 레벨도 화이팅이에요! 💪

질문 답변

결국 game 커맨드에 대한 처리는 기존 코드처럼 분기처리를 할 수 밖에 없었는데요,,
제가 느끼기에는 이 부분이 처리가 어려운 건 설계 미스인 것 같다고 생각합니다..
때문에 어떻게 개선하면 좋을 지, 알렉스의 의견을 얻어보고자 질문을 드리게 되었습니다!

현재 작성해주신 코드 구조상 단순한 분기처리 코드와 딱히 차별점이 느껴지진 않습니다! 근데 결국 이러한 코드를 고민해주시는 이유는 코드의 가독성과 유지보수성이 이유일 것 같은데, 현재 요구사항과 시스템 구조에서 꼭 분기문을 없애야하는 건지 의문이 드네요..! 단순한 분기문 나열이 가독성이 더 좋고 유지보수하기 좋은 경우도 있다고 생각하거든요!

@yxxnghwan yxxnghwan merged commit cc6da04 into woowacourse:pgmjun Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants