[4, 5단계 - 체스] 이프(송인봉) 미션 제출합니다.#399
Conversation
hongbin-dev
left a comment
hongbin-dev
left a comment
There was a problem hiding this comment.
안녕하세요 이프! 리뷰어 앨런입니다.
4, 5단계 미션 구현하시느라 고생많으셨습니다.
리뷰는 웹, DB 위주로 진행했어요. 코멘트 확인부탁드려요~
| final ChessGame chessGame = ChessGame.initializeChessGame(); | ||
| chessGame.start(); |
There was a problem hiding this comment.
체스게임의 초기화하는 부분을 나누신 이유가 있을까요? 객체의 흐름이 순서에 의존적인 것 같아요.
만약 ChessGame코드를 사용하는 다른 클라이언트 입장에서는 '사용이 어려울 수 있겠다'라는 생각이 드네요
There was a problem hiding this comment.
기존의 도메인을 최소한의 수정으로 재사용하던 과정에서 이런 로직이 나왔네요. 수정하도록 하겠습니다~!
|
|
||
| get("/", gameController.index()); | ||
|
|
||
| path("/room", () -> { |
There was a problem hiding this comment.
자원이 game인데요. games라는 prefix는 어떨까요?
또 아래 다른 path의 식별자들이 gameId인데요. url을 보고( /games/:id) 어떤 식별자를 줄 수 있을지 이해하기 좋을 것 같습니다.
| get("/start", gameController.createNewGame()); | ||
| get("/:gameId", gameController.loadGame()); | ||
| post("/:gameId/move", gameController.movePiece()); | ||
| post("/:gameId/promotion", gameController.promotion()); | ||
| get("/:gameId/status", gameController.calculatePlayerScores(), gson::toJson); | ||
| get("/:gameId/end", gameController.endGame()); |
| public GameDao(final MysqlConnector mysqlConnector) { | ||
| this.mysqlConnector = mysqlConnector; | ||
| } |
| public GameDto findById(final Long id) { | ||
| final Connection connection = mysqlConnector.getConnection(); | ||
|
|
||
| GameDto gameDto = null; |
| } catch (SQLException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return gameDto; |
There was a problem hiding this comment.
쿼리 중 예외가 생기면 catch부분으로 빠질 것 같은데요.
그럼 스택트레이스 출력하고 별다른 액션이 없네요. 그럼 null이 반환되는걸까요?
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
이 부분도 null반환을 피했으면 좋겠습니다!
catch 부분에서 예외를 내거나, 대체 값을 반환하면 return문을 제거할 수 있을 것 같아요.
| } catch (final SQLException e) { | ||
| throw new IllegalStateException(e.getMessage()); | ||
| } |
| try { | ||
| resultSet.close(); | ||
| preparedStatement.close(); | ||
| connection.close(); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
지금 이 부분에 문제가 발생할 수 있어요. 만약 resultSet.close();를 진행 중 문제가 생기면, ps와 connection은 안 닫히게됩니다. 각각의 실패 했을 때 액션처리가 안된 것 같아요.
try-with-resources를 사용해서 자원 닫는것을 위임해볼 수 있을 것 같아요.
https://tecoble.techcourse.co.kr/post/2021-04-26-try-with-resource/ 테코블의 참고하면 좋을 것 같아요~
There was a problem hiding this comment.
아! close 과정에서 문제가 발생하는 상황을 고려하지 못했네요. 수정하겠습니다!!
| import chess.dao.dto.GameDto; | ||
| import chess.dao.dto.GameUpdateDto; | ||
|
|
||
| class GameDaoTest { |
There was a problem hiding this comment.
웹 서버와 동일한 데이터베이스를 사용하고있군요~
이 부분은 어떻게하면 유연하게 테스트할 수 있을까요?
|
DB와 관련하여 지적해주셨던 Null 사용과 예외처리 미흡 등을 해결하기 위해 GameRepository가 PlayerRepository를 인스턴스로 지니면서 Player에 대한 DB 로직을 수행하는 것이 과연 옳은가 생각해봤어요. |
일단 계층구조가 controller - service - repository - dao - data인 것 같은데요. 또 repository같은 경우 도메인 패키지 내에 있는데, 정말 도메인이 맞는가? 도 고민되네요. |
|
DAO는 DB 테이블 중심의 객체이므로 도메인 객체와의 의존성이 없어야 한다고 생각했습니다. Repository와 DAO에 대한 각각의 책임과 역할을 말씀드리자면, Repository를 도메인 패키지에 위치한 이유는 Repository가 도메인을 다루는 컬렉션이기 때문입니다. 제 생각에 잘못된 부분이 있으면 과감히 지적해주시기 바랍니다. |
설명 감사합니다! DAO는 도메인의 컬렉션이 될 수 없는건가요? 비슷한 역할을 하는 것 같은데, 계층이 더 추가되어서 코드파악하기가 더 어려운 부분이 있는 것 같습니다.
DAO가 도메인인가? 는 아니라고 말씀드리고싶네요. 도메인객체가 데이터베이스에 저장되고, 파일시스템에 저장되는건 세부사항이라고 생각되네요 |
hongbin-dev
left a comment
hongbin-dev
left a comment
There was a problem hiding this comment.
안녕하세요 이프!
피드백 반영해주신부분 확인했고, 쿼리사이드의 중복코드를 잡아보셨군요 💯
고민해보면 좋을포인트 몇개 남겨봤어요. 레벨1 고생많으셨어요
| private Connection createConnection() { | ||
| try { | ||
| return DriverManager.getConnection(URL, USERNAME, PASSWORD); | ||
| } catch (final SQLException e) { | ||
| throw new IllegalStateException(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
매번 커넥션을 만들어서 사용하고있는데, 효율적인 방법이 있을지 고민해보셔도 좋을 것 같아요~!
| private static final String URL = "jdbc:mysql://localhost:13307/chess"; | ||
| private static final String USERNAME = "user"; | ||
| private static final String PASSWORD = "password"; |
There was a problem hiding this comment.
임베디드 db를 사용하면, 별도의 db준비없이도 테스트할 수 있을 것 같아요.
이건 반영하지 않으셔도 됩니다!
| public static ChessGame initializeAndStartChessGame() { | ||
| final ChessGame chessGame = initializeChessGame(); | ||
| chessGame.start(); | ||
| return chessGame; | ||
| } |
There was a problem hiding this comment.
콘솔 어플리케이션의 로직 때문에 ChessGame의 생성자와 start()가 분리되어 있습니다.
콘솔 어플리케이션에서는 프로그램 시작과 동시에 ChessGame의 생성자를 호출하여 게임 준비 상태에 접어들고
"start" 명령어 입력 시에 ChessGame#start 메서드를 호출하여 게임 진행 상태에 접합니다.
그에 반해, 웹 어플리케이션에서는 별도의 게임 준비 상태 없이,
"새로 시작하기" 버튼을 통해 ChessGame 생성과 동시에 게임 진행 상태로 빠져듭니다.
두 어플리케이션 간의 차이로 인해 현재, initializeAndStartChessGame과 같은 정적 팩토리 메서드가 탄생했습니다..
문제를 이와 같이 해결한 부분에 대해서는 달리 드릴 말씀이 없습니다.
해당 메서드가 과연 옳은가 하는 질문에는 그렇지 않다고 답변드리고 싶습니다.
허나 어떻게 보완하는 것이 옳은가하는 질문에는 제대로 답변드리기가 어렵습니다.
계속해서 방법을 생각 중이며, 필요에 따라 ChessGame 도메인의 변경에 따른 각 Contoller의 변경도 고려하고 있습니다.
There was a problem hiding this comment.
도메인이 콘솔, 웹 따라서 생성되는방법이 다르다고 이해했는데요. 콘솔구조에 맞춰진 도메인 생성자로 만들어진거 아닌가? 라는 생각이 들었습니다.
두 컨트롤러에 제너럴한 생성자로 변경을 고려해봐야할 것 같네요 🤔
이 부분 답변을 못드렸네요 😅 GameRepository에서 playerRepository를 의존하고 있군요. 가볍게 든 생각은 해당 비즈니스가 레포지토리역할이 아닌, 서비스의 역할이 아닌가 의심이 되네요. 해당 책임을 상위로 올려보는 시도를 해볼 수 있을 것 같아요. |
DAO(Data AccessObject)는 이름 그대로 DB와 관련된 책임을 수행해야 한다고 생각했기 때문에, SRP에 따라
서비스에서 행할까 생각했지만, ChessGame과 Player의 repository 로직을 억지로 분리하려 하니 도메인 영역까지 영향을 미치게 됩니다. 그래서 이와 같은 구조가 별 수 없다고 생각했었습니다. 하지만 이번에 DAO와 레파지토리를 조사하던 과정에서 알게 된 DAO와 REPOSITORY 논쟁에 나오는 현재 저의 코드는 레파지토리가 다른 레파지토리를 소유하면서 문제가 발생했습니다. 하지만 좀 더 근본적으로 바라보면, 왜 레파지토리를 분리해야 하는가가 문제였습니다. 예전 질문에서도 말씀드렸듯 ChessGame과 Player는 끊을래야 끊을 수 없는 관계입니다. 이를 굳이 두 레파지토리로 분리했기 때문에 현 상황이 도래한 것입니다. 제가 도달한 해결책은 PlayerRepository를 제거하고 GameRepository에서 GameDao와 PlayerDao를 이용하는 구조를 취하는 것입니다. 이번 방학 기간 동안 |
|
이야기하다보니 DDD 이야기까지 간 것 같은데요. 이 부분을 잘 몰라서, 제 의견 요점만 정리해놓겠습니다.
말씀드리고 싶은부분은 DAO나 Repository한 쪽의 네이밍만 사용하면 좋겠습니다. 현재 Repository가 DDD에서 말하는 Repository인지는 모르겠으나, Repository를 인터페이스로 분리하고, 현재의 DAO를 JdbcRepository로 변경하면 의존성의 흐름이 자연스럽긴 하겠네요. 이번 미션에서는 Web와 DB연동이 목적이라서 이 이야기는 여기서 마치겠습니다. 더 이상은 웹/DB미션과 벗어나는 것 같아요~ 고생하셨어요 |
해당 부분에 대해 찾아본 바로는, 여러 자료에서 Separated Interface를 적용해 domain에 repository 인터페이스를 두고 infrastructure에 repository의 구현체를 두는 형태를 취하곤 하네요. (결국 DDD네요...)
DAO를 처음 접하면서 기존에 스프링에서 사용했던 Repository와 차이점을 조사하고 제 나름대로의 정의를 내리다보니 이번 미션을 진행하면서 그동안의 코드 방식과 달리, 여러 가지 생각을 갖고서 각종 시도를 해봤습니다. 레벨1 초기에는 클래스/메서드 명명을 잘하고 내부구현로직을 깔끔하게 작성해서 이런 상황에서 자연스레 코드를 바라보는 관점이 지금 굉장히 방황하고 있습니다. 레벨1이 마무리되었고 이제 레벨2가 시작하는데 적다보니 사설이 많아졌네요. 많은 부분에 있어서 코드가 마음에 들지 않으셨을텐데도 |
|
이프 긴 기간 고생많으셨습니다.
아닙니다! 죄송할게 있나요?? 제가 말씀드린 건 '코드보기 어려웠다 -> 이 부분의 계층을 없애면 이해하기 좋을 것 같다'의 뉘앙스였습니다. 제가 불편한 건 아니였어요 😅 많은시도와 이프의 생각이 있는 코드 잘 보았고, 불편하지 않았습니다! 고생 많으셨고 방학기간 잘 보내세요~ 🙂 |
2 participants
안녕하세요! 이프입니다. 예상과 달리 4,5단계 체스 미션이 오래 걸렸네요ㅠ
다음은 이전 PR 코드로부터의 변경 사항과, 아직 미흡한 부분을 정리한 내용입니다!!
변경 사항
ChessGame 상태 패턴 수정
chess.domain.game.ChessGame에서 사용되던 command 패키지를 제거했고
state 패키지의 구성은 RunningState와 FinishedState 두가지로만 이뤄지도록 수정했습니다.
CommandExecutor을 통해 처리하던 분기문 수정
이전 PR에서 피드백 주신대로, application.console 패키지의 CommandExecutor를 통해
처리되던 ChessApplication의 분기문 로직을 수정했습니다.
현재 ChessApplication#play에서 각 명령어에 따른 조건문을 통해 로직을 수행하고 있습니다.
미흡한 부분(진행 예정)
Dao의 jdbc 코드에서 반복되어지는 코드를 메서드 분리할 예정입니다.
chess.dao.mysql 패키지의 jdbc 코드를 인터페이스로 추상화할 예정입니다.
chess.domain.game.repository.GameRepository가 현재 PlayerRepository를 인스턴스로 지니고 있습니다.
현재는 ChessGame과 Player의 의존관계 때문에 GameRepository가 PlayerRepository를 소유하도록 했지만,
GameRepository로부터 PlayerRepository를 분리하여 상위 계층에서 두 Repository를 사용하도록 할 예정입니다.