[🚀 사이클2 - 미션 (기물 확장 + DB 적용)] 이프 미션 제출합니다.#347
[🚀 사이클2 - 미션 (기물 확장 + DB 적용)] 이프 미션 제출합니다.#347verus-j merged 36 commits intowoowacourse:jihwankim128from
Conversation
* 교차점에서 수평/수직 이동 실패하는 문제 해결
verus-j
left a comment
There was a problem hiding this comment.
안녕하세요 이프~
2단계 진행하시느라 고생했습니다. 개인적으로 장기 미션에 비해 코드의 구조가 복잡한 것 같습니다. 트랜잭션도 고려하면서 기술적으로 다양한 부분을 적용하려고 노력한 점이 보이나 원하는 기술을 적용하면서 더 심플하게 구성할 수 있을 것 같아요. 우선적으로는 불필요한 상속 구조를 제거해보면 좋을 것 같습니다. 관련해서 리뷰 남겨놨으니 확인부탁드립니다~
| import model.GameStatus; | ||
| import model.JanggiGame; | ||
|
|
||
| public class InMemoryJanggiRepository implements JanggiRepository { |
There was a problem hiding this comment.
아.. DB 접근 실패 시 서킷 브레이커 패턴처럼 InMemory Repository로 제공하기 위함이었습니다!
그리고 cycle 2 - 2단계에서 DB Layer를 추가한다는 것을 인지했기 때문에, InMemory DB로 확장 가능한 구조로 먼저 개선한 후 DB만 갈아끼우기 위해 InMemoryRepository를 사용했었습니다.
현재는 미사용 중이긴 한데, 당장에 사용하지 않는다면 제거하는게 더 나을까요?
|
|
||
| public class ConnectionManager { | ||
|
|
||
| private static final ThreadLocal<Connection> THREAD_LOCAL_CONNECTION = new ThreadLocal<>(); |
There was a problem hiding this comment.
기존에 매번 새로운 Connection으로 각 변경 상태를 저장했습니다.
여기서 한가지 문제가 있다고 판단했습니다. Connection의 Auto Commit으로 인해 각 Insert 작업이 모두 독립적으로 발생합니다. 결국 하나의 비즈니스 로직 단위에서 독립적인 커밋으로 원자성을 잃는 문제가 발견했습니다.
대표적인 예시로 기물 이동에서 장기 상태 변경 -> 기물 위치 변경이 있는 경우, 장기 상태만 변경되고 기물 위치가 변경되지 않는 문제가 있습니다.
그래서 트랜잭션이 보장되지 않기 떄문에 트랜잭션을 보장하는 로직을 도입했습니다. 이 과정에서 MySQL의 커맨드성 작업은 트랜잭션을 갖고 있기 때문에, 장기의 상태 변경과 기물 위치 변경과 같이 두 개의 변경이 묶인 로직은 하나의 Connection으로 처리해야 한다고 판단했습니다.
그래서 Connection을 관리하기 위한 방법을 생각하면서 과거 학습한 Spring Tx를 참고해 적용했습니다. 이 과정에서 비즈니스 관심사(도메인 모델 로직)와 흐름 제어 관심사(어플리케이션 서비스 로직) 를 분리하게 됐습니다. 여기서 트랜잭션의 단위는 어플리케이션 레이어가 알고 있기 때문에, TxService를 도입했습니다.
최종적으로 TxService는 트랜잭션 관리자의 역할을 하므로 트랜잭션 단위에 대한 Connection을 생성하게 됐습니다. 그럼 DAO에서는 TxService가 생성한 Connection을 필요로 했기 때문에, 스레드 단위로 생성한 독립적인 Connection을 사용하도록 ThreadLocal을 활용해 문제를 해결했습니다.
ThreadLocal의 문제점으로는 Thread의 Connection 자원을 해제하지 않게 되면, ThreadPool에서 미리 할당된 Thread로 인해 다음 작업에서 Connection이 공유 될 수 있다는 문제가 있습니다. 그래서 트랜잭션이 끝나면 finally를 통해 ThreadLocal에 생성했던 커넥션을 Remove하도록 명시했습니다.
베루스의 입장은 단순 콘솔 프로그램에서 과하다는 관점인가요?
만약 이런 관점이라면 동의합니다!.. 하지만 비즈니스 로직에는 집중 했고 남는 시간동안 Thrid party에 대해서는 복잡하더라도 다양한 도전을 해보고 싶었습니다. 8기 키워드가 도전인 만큼 Common Case는 복잡해도 된다는 기술 블로그의 내용을 보고 사용해봤는데... 혹시 다른 관점이나 다른 문제점 때문에 해당 코멘트를 남겨주신걸까요?!
There was a problem hiding this comment.
기술적으로 다양한 시도를 해보려고 한 점은 좋았지만, 단순 콘솔 프로그램에서는 과한 구조인 것 같습니다. Spring의 tx를 참고해서 적용하셨다고 했는데, 현재 개발중인 장기 콘솔 애플리케이션에서도 최고의 구조인지 고민해볼 필요도 있을 것 같아요. 그리고 트랜잭션과 관련된 기술적 도전들은 레벨2에서 많이 이뤄질거라 개인적으로는 레벨1에 맞는 도전들(OOP, 클린코드, 테스트 등)을 더 찾아봤으면 좋았을 것 같긴합니다.
| import model.piece.Piece; | ||
| import repository.JanggiRepository; | ||
|
|
||
| public class JanggiQueryService { |
There was a problem hiding this comment.
Query용과 Command 용을 분리할 필요가 있을까요? 그리고 대부분 QueryService의 로직은 불필요한 것 같습니다. Controller에서 JanggiGame을 사용하면 어떨까요?
There was a problem hiding this comment.
좋은 리뷰 감사합니다 베루스..!
Query용과 Command 용을 분리할 필요가 있을까요?
이전 코멘트에서 언급했듯, 서비스 레이어에 트랜잭션을 도입하면서 수동 프록시(TxService)를 사용하게 되었습니다. 이때 모든 기능을 하나의 서비스에 넣으면 트랜잭션이 필요 없는 단순 조회 로직까지 오버라이딩으로 인한 프록시 오버헤드 문제가 있었습니다.
그래서 변경이 발생하는 기능(Command)은 JanggiService로, 단순 조회(Query)는 프록시를 타지 않는 JanggiQueryService로 분리하여 효율성을 높이고자 했습니다.
그리고 대부분 QueryService의 로직은 불필요한 것 같습니다. Controller에서 JanggiGame을 사용하면 어떨까요?
이것 또한 고민했던 부분입니다. Service Layer의 도입으로 애플리케이션의 유스케이스 보호 관점에서 고민했습니다.
컨트롤러를 논리적으로 ui 계층으로 분리하면서 컨트롤러가 도메인을 직접 알게 되면, 비즈니스 로직의 흐름을 제어하는 책임이 컨트롤러로 새어 나갈 위험이 있다고 판단했습니다.
하지만 현재 불편하신 부분도 충분히 공감이 됩니다..! janggiGame을 직접 가져오는 방식으로 개선해보겠습니다.
There was a problem hiding this comment.
컨트롤러를 논리적으로 ui 계층으로 분리하면서 컨트롤러가 도메인을 직접 알게 되면, 비즈니스 로직의 흐름을 제어하는 책임이 컨트롤러로 새어 나갈 위험이 있다고 판단했습니다.
컨트롤러의 역할이 뭔가요? 현재 적용중인 MVC 패턴에서 Service 계층이 있나요? 레이어드 아키텍처와 MVC 패턴에 대해 어떤 차이가 있는지 생각해보면 좋을 것 같아요. 그리고 과연 현재 애플리케이션에서 레이어드 아키텍처가 필요한지도 같이 고민해보면 좋을 것 같습니다.
| import model.JanggiGame; | ||
| import model.Team; | ||
|
|
||
| public record JanggiResultDto( |
There was a problem hiding this comment.
게임 결과에서 3가지 정보를 동시에 요구합니다.
Team winner,
boolean bigJangDone,
Map<Team, Double> finalScore베루스가 남겨준 앞 코멘트처럼 JanggiGame을 직접 의존한다면, Dto가 필요없겠지만 현재는 비즈니스 레이어를 보호하자는 명목에서 Dto의 필요성을 느꼈습니다. 혹시 Dto의 본질적인 네이밍이 문제라면 Application Layer의 Result 객체로 네이밍을 수정하겠습니다!
혹시 제가 답변한 내용이 베루스 의도에 맞는 답변일까요?!
There was a problem hiding this comment.
DTO를 도입한게 계층을 보호하기 위해 도입한거라는게 느껴졌는데, 이전 피드백들을 읽어보셔서 아시겠지만 현재 구조에서 레이어드 아키텍처가 필요한가라는 의문이 있어 남긴 질문이였습니다.
| import model.board.TeamFormation; | ||
| import repository.db.TransactionTemplate; | ||
|
|
||
| public class JanggiTxService implements JanggiService { |
There was a problem hiding this comment.
트랜잭션 적용을 위해 JanggiServiceImpl과 구분해야할까요? 장기 미션에서는 심플하게 트랜잭션을 적용한 JanggiService만 제공하는 것도 좋아보입니다.
There was a problem hiding this comment.
그렇네요... 좋은 피드백 감사합니다!
어차피 트랜잭션 관심사에 대한 보일러 플레이트 코드를 TransactionTemplate으로 분리했고,
TxService는 단순히 Template을 통해 데코레이트 할 뿐이네요... ㅎㅎㅎ
transactionTemplate.execute(() -> logic()); 이런 코드도 거슬렸나봅니다...
이건 합치는게 훨씬 더 좋아보이는 것 같아요! 감사합니다.
| import ui.view.InputView; | ||
| import ui.view.OutputView; | ||
|
|
||
| public abstract class JanggiController { |
There was a problem hiding this comment.
사용자 입력에 따른 기능 제공으로 Controller를 분리한 건 좋았으나 상속을 사용하면서까지 구성이 필요한가 싶습니다. 상속 없이 분리하는 방법은 없을까요?
There was a problem hiding this comment.
사실 is-a와 has-a 관계에 트레이드 오프에 대해 크게 와닿지 못했습니다.
그런데 베루스가 남겨주신 리뷰를 보고 계속 고민해봤는데요...
앞으로 추가되는 기능들이 부모 클래스에 강하게 결합되겠네요...
has-a 관계로 분리하도록 해보겠습니다 :)
감사합니다 😊
| private void play(Long janggiId) { | ||
| if (janggiQueryService.canPlaying(janggiId)) { | ||
| retry(() -> playByTurn(janggiId), OutputView::displayError); | ||
| } | ||
| OutputView.displayBoard(janggiQueryService.getBoardResponse(janggiId)); | ||
| } |
There was a problem hiding this comment.
janggiQueryService를 호출하는 로직이 모두 Game 조회를 위해서 DB에 매번 접근할 것 같은데 비효율적인 것 같네요. 어떻게 개선하면 좋을지 고민해보면 좋을 것 같아요~
There was a problem hiding this comment.
베루스, 성능적인 비효율을 짚어주셔서 감사합니다!
말씀하신 대로 한 루프 내에서 동일한 데이터를 위해 여러 번 DB에 접근하는 것은 분명 비효율적인 구조입니다.
다만, 도날드 크누스의 말처럼 현재 규모에서 캐싱을 도입하는 것은 섣부른 최적화가 될 수 있다고 판단했습니다.
그렇다고 단순히 조회를 줄이기 위해 도메인 객체(JanggiGame)를 컨트롤러에 직접 노출하는 것은 제가 지키고자 했던 서비스 계층의 캡슐화를 저해하는 일이라고 생각했습니다.
그래서 저는 조회 전용 DTO를 도입하여, 한 번의 DB 접근으로 플레이에 필요한 데이터를 묶어서 반환하는 방식으로 개선했습니다. 이렇게 하면 계층 분리의 원칙을 지키면서도 기술적인 비효율을 합리적으로 해결할 수 있다고 생각했는데, 더 나은 방식이 있을까요??
JanggiGame을 사용하는 방식에 대한 반박보단 단순 궁금증입니다!!
There was a problem hiding this comment.
캐싱을 의도하여 리뷰를 남긴건 아니고, 조회한 도메인 객체를 재활용하길 바랬습니다. 컨트롤러에 도메인 객체의 접근을 막은 이유가 조금 이해가 안되네요 ㅠㅠ 지금 같은 상황은 오히려 잘못된 구조로 인해 불필요한 DB 접근이 생기는 상황같습니다... 컨트롤러에서 도메인 객체를 사용하게되면 심각한 문제가 생길까요...? 그리고 도메인 객체가 변경돼서 서비스 계층의 로직이 변경되는건 괜찮은걸까요?
| } | ||
|
|
||
| private void checkBigJang(Long janggiId) { | ||
| if (!janggiQueryService.isBigJang(janggiId)) { |
There was a problem hiding this comment.
README에 간략히 작성되어 있습니다!
왕이 잡히지 않은 경우 점수로 승패를 결정한다. (빅장)
미션 요구사항에서 참고자료로 제공된 장기 위키백과에 작성된 내용입니다.
장기-위키
두 왕이 마주보고 있을 때, 그 사이에 기물이 없는 경우 빅장을 통해 종료할 수 있습니다.
요구사항 중 점수계산을 고려했을 때, 빅장이 없다면 무의미한 점수라고 생각해서... 요구사항에 제시된 자료를 최대한 참고해 적용해봤습니다.
There was a problem hiding this comment.
원래 영어로도 BigJang인가요? 처음 들어본 단어여서 생소해서 여쭤봤습니다. 딱히 용어가 없어서 BigJang으로 하신거군요
src/main/java/model/JanggiState.java
Outdated
| import model.state.Finished; | ||
| import model.state.Running; | ||
|
|
||
| public abstract class JanggiState { |
There was a problem hiding this comment.
너무 과하게 상속을 사용한 것 같습니다... 모든 구조에 상속을 필수적으로 넣은느낌인데 Piece 외의 객체들에서는 상속을 제거해보면 어떨까요? 지금 상속 구조 때문에 컨트롤러에서부터 기물 이동 관련 로직을 보는데까지 너무 복잡합니다. 불필요한 상속을 제거하고 흩어져 있는 로직들을 모아서 한번에 이해할 수 있도록 만들어보면 좋을 것 같아요
There was a problem hiding this comment.
베루스! 단순히 진행/종료만 있다면 다른 방식을 선택 했을 것 같습니다.
하지만 제 설계에서는 '빅장(Big-Jang)' 이라는 특수 상태가 핵심이라고 생각합니다.
- 진행 중: 일반적인 이동 로직
- 빅장: 이동 후 빅장 여부를 확인하고 사용자에게 선택권을 주는 상태
- 빅장 종료: 빅장 선언으로 인한 즉시 종료
- 일반 종료: 왕이 잡혀서 끝나는 상태
이 4가지 상태는 이동(move) 가능 여부와 체크 로직이 물리적으로 다르게 작동합니다. 이걸 JanggiGame 하나에서 if-else로 풀면 메서드 하나가 너무 거대해지고 가독성이 떨어지는 것을 관측했습니다. 상태 패턴을 통해 현재 게임의 따른 책임을 명확히 분리하고 싶었는데, 이런 경우에도 과한 선택이었을까요..?
우선은 리뷰어가 함께 협업하는 동료라고 생각했을 때, 불편함을 느꼈으니 JanggiGame으로 다시 로직을 모아보겠습니다!
베루스의 말씀과 동일한 방향으로 개선했는지 모르겠는데,,, 장기 상태를 Interface로 분리하고 4가지 상태를 2가지 클래스에서 진행중(일반, 진행중)과 종료(일반, 빅장)으로 구분했습니다. 원하시는 방향으로 개선됐는지 궁금합니다!
There was a problem hiding this comment.
public void movePiece(Position current, Position next) {
Piece piece = selectPiece(current);
List<Position> path = piece.pathTo(current, next);
List<Piece> pieces = board.extractPiecesByPath(path);
piece.validatePathCondition(pieces);
board.move(current, next);
this.state = state.next(board);
}우선 위의 로직만 봤을 때 state의 변경을 쉽게 알아차릴 수 있는지 고민해보면 좋을 것 같습니다. 코드를 처음보는 입장에서는 state의 변경 흐름을 따라가기 좀 어려운 것 같네요. 거기다 status가 변하는 조건까지 있어서 더 어려운 것 같네요 ㅠㅠ
public class Finished implements JanggiState {
...
@Override
public JanggiState next(Board board) {
throw new IllegalStateException("종료된 게임은 더이상 진행할 수 없습니다.");
}JanggiState 중에서 Finished의 next 구현이 예외여서 더욱이 상태패턴을 적용해야하는지 의문이 드네요 ㅠㅠ
verus-j
left a comment
There was a problem hiding this comment.
안녕하세요 이프~
2단계 진행하시느라 고생하셨습니다. 이번 미션에서 기술적으로 다양한 도전을 하려는 모습이 보여서 좋았네요. 조금 아쉬운 점은 너무 과하게 기술적 도전과 레이어드 아키텍처에 빠지셨던 것 같습니다 ㅠㅠ 몇가지 관련된 피드백을 남겨뒀으니 한번 보시고 고민해보면 좋을 것 같아요. 레벨 1 진행하는 동안 너무 고생많으셨고 남은 우테코 생활 화이팅입니다~
| import model.GameStatus; | ||
| import model.JanggiGame; | ||
|
|
||
| public class InMemoryJanggiRepository implements JanggiRepository { |
|
|
||
| public class ConnectionManager { | ||
|
|
||
| private static final ThreadLocal<Connection> THREAD_LOCAL_CONNECTION = new ThreadLocal<>(); |
There was a problem hiding this comment.
기술적으로 다양한 시도를 해보려고 한 점은 좋았지만, 단순 콘솔 프로그램에서는 과한 구조인 것 같습니다. Spring의 tx를 참고해서 적용하셨다고 했는데, 현재 개발중인 장기 콘솔 애플리케이션에서도 최고의 구조인지 고민해볼 필요도 있을 것 같아요. 그리고 트랜잭션과 관련된 기술적 도전들은 레벨2에서 많이 이뤄질거라 개인적으로는 레벨1에 맞는 도전들(OOP, 클린코드, 테스트 등)을 더 찾아봤으면 좋았을 것 같긴합니다.
| import model.piece.Piece; | ||
| import repository.JanggiRepository; | ||
|
|
||
| public class JanggiQueryService { |
There was a problem hiding this comment.
컨트롤러를 논리적으로 ui 계층으로 분리하면서 컨트롤러가 도메인을 직접 알게 되면, 비즈니스 로직의 흐름을 제어하는 책임이 컨트롤러로 새어 나갈 위험이 있다고 판단했습니다.
컨트롤러의 역할이 뭔가요? 현재 적용중인 MVC 패턴에서 Service 계층이 있나요? 레이어드 아키텍처와 MVC 패턴에 대해 어떤 차이가 있는지 생각해보면 좋을 것 같아요. 그리고 과연 현재 애플리케이션에서 레이어드 아키텍처가 필요한지도 같이 고민해보면 좋을 것 같습니다.
| private void play(Long janggiId) { | ||
| if (janggiQueryService.canPlaying(janggiId)) { | ||
| retry(() -> playByTurn(janggiId), OutputView::displayError); | ||
| } | ||
| OutputView.displayBoard(janggiQueryService.getBoardResponse(janggiId)); | ||
| } |
There was a problem hiding this comment.
캐싱을 의도하여 리뷰를 남긴건 아니고, 조회한 도메인 객체를 재활용하길 바랬습니다. 컨트롤러에 도메인 객체의 접근을 막은 이유가 조금 이해가 안되네요 ㅠㅠ 지금 같은 상황은 오히려 잘못된 구조로 인해 불필요한 DB 접근이 생기는 상황같습니다... 컨트롤러에서 도메인 객체를 사용하게되면 심각한 문제가 생길까요...? 그리고 도메인 객체가 변경돼서 서비스 계층의 로직이 변경되는건 괜찮은걸까요?
| } | ||
|
|
||
| private void checkBigJang(Long janggiId) { | ||
| if (!janggiQueryService.isBigJang(janggiId)) { |
There was a problem hiding this comment.
원래 영어로도 BigJang인가요? 처음 들어본 단어여서 생소해서 여쭤봤습니다. 딱히 용어가 없어서 BigJang으로 하신거군요
| import model.JanggiGame; | ||
| import model.Team; | ||
|
|
||
| public record JanggiResultDto( |
There was a problem hiding this comment.
DTO를 도입한게 계층을 보호하기 위해 도입한거라는게 느껴졌는데, 이전 피드백들을 읽어보셔서 아시겠지만 현재 구조에서 레이어드 아키텍처가 필요한가라는 의문이 있어 남긴 질문이였습니다.
src/main/java/model/JanggiState.java
Outdated
| import model.state.Finished; | ||
| import model.state.Running; | ||
|
|
||
| public abstract class JanggiState { |
There was a problem hiding this comment.
public void movePiece(Position current, Position next) {
Piece piece = selectPiece(current);
List<Position> path = piece.pathTo(current, next);
List<Piece> pieces = board.extractPiecesByPath(path);
piece.validatePathCondition(pieces);
board.move(current, next);
this.state = state.next(board);
}우선 위의 로직만 봤을 때 state의 변경을 쉽게 알아차릴 수 있는지 고민해보면 좋을 것 같습니다. 코드를 처음보는 입장에서는 state의 변경 흐름을 따라가기 좀 어려운 것 같네요. 거기다 status가 변하는 조건까지 있어서 더 어려운 것 같네요 ㅠㅠ
public class Finished implements JanggiState {
...
@Override
public JanggiState next(Board board) {
throw new IllegalStateException("종료된 게임은 더이상 진행할 수 없습니다.");
}JanggiState 중에서 Finished의 next 구현이 예외여서 더욱이 상태패턴을 적용해야하는지 의문이 드네요 ㅠㅠ
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요. 베루스 정말 늦었습니다 ㅎㅎ..
cycle1에서 부진했던 탓에 cycle2 를 너무 늦게 진행했네요..
저는 cycle1에서 빠른 PR을 목표로 진행하다가 오히려 다시 보니 이해하기 힘든 코드가 됐습니다...
그래서 이번 cycle2에서는 제가 해볼 수 있는 만큼 분리와 책임에 집중해봤습니다.
이어서 변경 사항과 함께 피드백 바라는 부분 작성하겠습니다.
1. TransactionTemplate과 Proxy 패턴을 통한 관심사 분리
비즈니스 로직에 DB 기술인 트랜잭션 코드가 침범하는 것을 방지하기 위해 JanggiTxService를 도입했습니다.
매번 Connection을 생성하는 방식은 2가지 문제가 있었습니다.
그래서 한 트랜잭션에서 Connection을 공유하고 공유된 Connection은 트랜잭션이 끝났을 때 반환하는 식으로 개선했습니다.
트랜잭션은 부가 기능인데 이를 명시적으로 프록시 객체로 만든 것이 과한 설계는 아닐지, 혹은 서비스 계층의 순수성을 지키기 위한 합리적인 선택이었을지에 대한 의견이 궁금합니다.
2. 상태 패턴(State Pattern)을 이용한 장기 게임 흐름 제어
장기 게임의 복잡한 상태(진행 중, 빅장 합의, 종료 등)를 JanggiState 추상 클래스와 하위 상태 객체들로 관리했습니다.
Running, BigJang, Finished, BigJangDone 등의 상태를 정의하고,
state.next(board)를 통해 상태 전이가 일어나도록 설계했습니다.특히 '빅장'이라는 특수한 도메인 규칙을 상태로 녹여내어 JanggiGame 클래스의 많은 책임을 피하게 설계 했습니다.
이렇게 양 측의 생각이 오가는데 베루스는 어떻게 생각하시나요?
3. 이전 리뷰
cycle1에서 해결되지 않은 두 리뷰에 대해 적용해봤습니다.
더 나은 방법이 있을까요? (여기가 제일 어렵습니다..)
4. 그 외
그 외는 third party 관련 작업이라, 아직은 비즈니스 및 객체지향에 집중하고 싶습니다.
여유가 되신다면 그 외 사항에 대해서도 자세하게 리뷰해주시면 성장에 큰 도움이 될 것 같습니다!
이번 미션도 리뷰 잘부탁드리겠습니다! 🔥