[🚀 사이클2 - 미션 (기물 확장 + DB 적용)] 라이 미션 제출합니다.#339
[🚀 사이클2 - 미션 (기물 확장 + DB 적용)] 라이 미션 제출합니다.#339dhyepark wants to merge 148 commits intowoowacourse:dhyeparkfrom
Conversation
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
안녕하세요, 라이! 리뷰어 초코칩입니다 🍪
사이클 1에 이어 사이클 2도 잘 구현해주셨네요.
리뷰 남겼으니 확인해보시고, 재요청주세요 😄
|
|
||
| private void continuallyPlay(Long id) { | ||
| Board board = repository.findBoardById(id); | ||
| Players players = repository.findPlayersById(id); |
There was a problem hiding this comment.
한 턴에서 게임을 종료해도, 무조건 초 턴으로 시작되는 버그가 있네요. 해당 케이스는 테스트로 커버가 되고 있나요?
There was a problem hiding this comment.
JanggiRepository에 findTurnById(Long gameId)가 정의되어 있고 DB에도 저장하고 있는데, 복구할 때 전혀 사용하지 않는데, 이를 이용해볼 수 있을까요?
There was a problem hiding this comment.
JanggiRepository의 findPlayersById에서 findTurnById을 통해 턴 정보를 가져올 수 있도록 변경했습니다!
턴 스위치 -> 저장 순서로 진행되어야 하는데, 저장 -> 턴 스위치 순서로 코드를 작성해 두어서 버그가 발생했습니다. playerTurn 내의 보드 정보 업데이트 메서드를 play의 턴 스위치 이후로 옮겨 해결했습니다!
| import java.sql.SQLException; | ||
|
|
||
| public class DBConnection { | ||
| private static final String URL = System.getenv("DB_URL"); |
There was a problem hiding this comment.
JdbcJanggiRepository가 DBConnection.getConnection()을 정적으로 직접 호출하기 때문에, 테스트 시 H2 등의 다른 DB로 교체할 방법이 없네요.
FakeJanggiRepository를 만든 이유가 바로 이것이겠지만, 결국 JDBC 레이어를 전혀 테스트하지 못하는 구조가 되고 있어요. 어떻게 개선해볼 수 있을까요?
There was a problem hiding this comment.
DBConnection 클래스를 제거하고, JdbcJanggiRepository에 DataSource 필드를 추가해 두었습니다! 외부에서 DataSource를 넘겨주도록 DI를 적용시켜 다른 DB로 갈아끼우기 쉽게 코드를 수정했습니다. Application의 main에서 DB 정보를 set 해 주는 건 코드가 너무 복잡해지는 것 같아서, AppConfig를 만들어 조립 책임을 분리했습니다.
| try { | ||
| return DriverManager.getConnection(URL, USER, PASSWORD); | ||
| } catch (SQLException e) { | ||
| System.err.println("접속 시도 URL: " + URL); |
There was a problem hiding this comment.
print는 단순히 콘솔에 메시지를 출력하고, I/O 리소스를 많이 사용합니다. 이와 달리, logger는 비동기 로깅이 가능하고, 로그 레벨에 따라 메시지의 중요를 다르게 활용할 수 있다는 장점이 있습니다.
실제 서비스 환경에서는 로그를 통해 원인을 정확하게 추적하는 것이 필수적이라 생각해, AppConfig의 DB 연결 부분에서 print 대신 로그를 활용하도록 리팩터링 했습니다!
| return DriverManager.getConnection(URL, USER, PASSWORD); | ||
| } catch (SQLException e) { | ||
| System.err.println("접속 시도 URL: " + URL); | ||
| throw new RuntimeException("[ERROR] DB 연결에 실패했습니다. 환경 변수를 확인하세요."); |
There was a problem hiding this comment.
에러 메세지에 DB 에러를 명시해야 할까요? 클라이언트(사용자)는 이것을 보고 무엇을 해야하나요? 보안상으로 문제는 없을까요?
- 참고: 토스의 에러메세지
There was a problem hiding this comment.
처음에는 구현하는 입장에서만 생각하여 상세한 정보를 담으려 했으나, 실제 사용자 입장에서는 DB 에러와 같은 메시지가 거부감을 줄 수 있고, 시스템 내부 정보 노출로 인한 보안 상의 문제가 발생할 수 있다고 이해했습니다.
올려 주신 자료 참고하여 해당 메시지를 친화적인 문구로 변경해 두었습니다. 의견 주셔서 감사합니다! :D
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.atomic.AtomicLong; | ||
|
|
||
| public class FakeJanggiRepository implements JanggiRepository { |
There was a problem hiding this comment.
테스트 더블 활용 👍🏻 테스트 패키지에서만 쓰이는 것 같은데, 옮겨보는건 어떤가요?
There was a problem hiding this comment.
테스트 패키지 내로 옮겨두었습니다. 꼼꼼히 확인해 주셔서 감사합니다!
| this.current = Side.CHO; | ||
| } | ||
|
|
||
| // DB에서 게임 불러온 상태로 복구할 때 사용 |
There was a problem hiding this comment.
주석으로 로직을 제약하는게 꽤 보이네요.
로직을 제약할 수 있는 장치(검증)나 명시적으로 표현할 수 없을까요?
There was a problem hiding this comment.
주석 대신 정적 팩토리 메서드를 만들어 생성자를 명시적으로 사용할 수 있도록 수정해 두었습니다!
| private static final String ERROR_FINISH_GAME = "[ERROR] 게임 종료 처리 실패: "; | ||
|
|
||
| @Override | ||
| public Long save(Players players) { |
| - `DB_URL`: JDBC 연결 주소 (예: jdbc:mysql://localhost:3306/janggi_db) | ||
| - `DB_USER`: MySQL 사용자 이름 | ||
| - `DB_PASSWORD`: MySQL 비밀번호 |
There was a problem hiding this comment.
실행 가이드를 더 자세히 적는 것은 어떤가요? 실행을 위해 로컬에서 DB가 실행되어야하고... 등등이요.
| - [x] [규칙] 저장된 게임 ID를 바탕으로 이전 게임의 플레이어, 턴, 보드 상태를 복원한다. | ||
| - [x] [규칙] 승패 결정 시, 해당 게임의 종료 상태(`is_finished`)를 반영한다. `public void finishGame` | ||
|
|
||
| - [x] **[Database]** 게임 영속화를 위해 데이터 스키마를 설계한다. |
There was a problem hiding this comment.
mermaid, excallidraw같은 툴을 활용하여 ERD 그려주시면 이해가 편할것 같네요.
| @@ -174,13 +174,16 @@ | |||
| - [x] [규칙] 도착 위치에 같은 팀(아군) 기물이 있으면 이동할 수 없다. | |||
| - [ ] [규칙] 이동 이후 자신의 '궁'이 상대에게 잡힐 수 있는 위험한 상태(장군)가 된다면, 그 이동은 허용되지 않는다. | |||
There was a problem hiding this comment.
DTO 변환 로직 위치
현재 구조에서 충분히 잘 동작하고 있어 보여요. JanggiGame이 Controller 역할을 하고 있고, DTO 변환은 View에 넘기기 직전에 하는 것이 자연스럽게 느껴져요. View가 도메인 객체를 직접 알지 못하게 막으려는 의도도 잘 드러나 있어요.
Service 계층을 별도로 두는 것이 의미 있는 시점은 "여러 도메인 객체를 조율하는 복잡한 비즈니스 로직"이 Controller에 쌓이기 시작할 때라고 생각해요. 지금은 DTO 변환 하나를 위해 Service 계층을 도입하면 오버엔지니어링되지 않을까요?
동시성 고려
ConcurrentHashMap과 AtomicLong을 직접 찾아서 적용, 학습해보신 건 💯 다만 이 맥락에서는 과도한 선택으로 느껴져요.(변경의 리뷰는 아닙니다.)
FakeJanggiRepository는 단일 스레드 테스트 환경에서만 사용되는데, 동시성 도구는 필요한 곳에서만 써야 한다고 생각해요. 불필요하게 쓰면 코드를 읽는 사람이 "여기 동시성 이슈가 있나?" 하고 의심하게 되어 오히려 혼란을 줄 수 있어요.
저희 팀에서는 캐싱용 Map을 쓸 때는 ConcurrentHashMap을 사용해서, 여러 스레드가 동시에 접근하는 경우에도 동시성을 보장하고 있어요. 그 상황을 제외하고는 일단 List나 Map을 사용합니다.
동시성 도구를 공부할 때는 도구의 사용법만큼이나 "언제 필요한가"를 파악하는 것도 중요하다고 생각해요 😊
GameManager 클래스
Board.final을 제거하지 않고 객체 생성 주기를 분리한 판단 👍 다만 GameManager라는 이름은 조금 모호하게 느껴지네요 🤔
GameManager의 역할은 게임 실행 진입점 조율에 가깝습니다. GameLauncher나 GameRunner 정도가 역할을 더 명확하게 드러낼 것 같습니다.
이름 짓기에 정답은 없지만, 클래스 이름만 보고도 "이 클래스가 무엇을 하는지"와 {클래스명}이 {메서드명} 행동을 한다의 명제가 자연스럽게 느껴지는 이름을 설정하는 목표로 고민해보시면 좋겠네요.
3 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
초코칩 안녕하세요! 먼저, PR이 예상보다 많이 늦어진 것 같아 죄송하다는 말씀드립니다. 🥲
사이클2도 리뷰 잘 부탁드립니다!
현재
JanggiGame의play메서드에서 도메인을 DTO로 변환하고 있습니다. 도메인 보호와 뷰 의존성 분리를 위해 DTO를 사용했는데, 이 변환 로직이JanggiGame에 있는 것이 적절할지, 아니면 Service 계층을 두어 그 안에서 변환하는 것이 더 나은 설계일지 여쭤보고 싶습니다.DB 실제 적용 전 단계에서,
FakeRepository를 구현하며ConcurrentHashMap과AtomicLong을 처음 알게 되었습니다. 여러 스레드에서 호출이 발생하는 경우, 값 변경을 안전하게 보장할 수 있다고 보았는데, 실제 현업이나 우테코 미션 수준에서 이러한 제어를 어느 정도까지 고려해야 하는지 궁금합니다.이전에는
JanggiGame생성 시점에Board가 함께 초기화되는 구조였는데,JanggiGame내의Board필드를 final로 유지한 상태에서 업데이트 로직을 구현하려다 보니 final을 제거해야 하는 상황이 발생했습니다. 단순히 final을 제거하기보다는 객체 생성 주기를 분리하는 것이 더 적절하다고 판단하여,GameManager클래스를 도입했습니다. 이처럼 도메인과 애플리케이션 흐름 제어를 분리하는 방식이 적절한 설계인지에 대해 의견을 여쭙고 싶습니다. (클래스명도 괜찮은지 조언 부탁드립니다!)