step 34 로빈#19
Open
robinjoon wants to merge 45 commits intowoowacourse-6th-code-review-study:robinjoonfrom
Open
step 34 로빈#19robinjoon wants to merge 45 commits intowoowacourse-6th-code-review-study:robinjoonfrom
robinjoon wants to merge 45 commits intowoowacourse-6th-code-review-study:robinjoonfrom
Conversation
트랜잭션 적용하면서 해당 로직의 필요성이 없어졌다.
3Juhwan
reviewed
Apr 3, 2024
3Juhwan
left a comment
3Juhwan
left a comment
There was a problem hiding this comment.
로빈, 안녕하세요~ 망쵸입니다!
코드 잘 봤어요. JDBC, DB와 관련된 코드가 인상적이었어요 💯
덕분에 제가 학습하지 못한 부분을 학습할 수 있었어요.
레벨1 정말 고생 많았고, 레벨2도 열심히 해보아요 ✊✊
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class PiecesOnChessBoardDAOForMysql implements PiecesOnChessBoardDAO { |
There was a problem hiding this comment.
특정 DB에 매우 종속적인 네이밍이네요! 구현체니까 괜찮은 것 같아요. 다만, 다소 길다는 느낌은 있네요!
Comment on lines
+50
to
+52
| private record DBConnectionParameters(String url, String userName, String password) { | ||
| } | ||
| } |
There was a problem hiding this comment.
내부 클래스를 이렇게 잘 사용할 수 있네요! 사소하지만 대단하다고 느껴지네요 👍
Comment on lines
+31
to
+40
| startTransaction(connection); | ||
| if (isSaveDataExist()) { | ||
| List<Piece> pieces = piecesOnChessBoardDAO.selectAll(connection); | ||
| Team currentTeam = turnDAO.select(connection).get(); | ||
| commitTransaction(connection); | ||
| return new ChessGame(pieces, currentTeam); | ||
| } | ||
| piecesOnChessBoardDAO.deleteAll(connection); | ||
| turnDAO.delete(connection); | ||
| commitTransaction(connection); |
Comment on lines
+71
to
+78
| startTransaction(connection); | ||
| if (canNotSave(connection)) { | ||
| return false; | ||
| } | ||
| boolean saveAllPiecesSuccess = piecesOnChessBoardDAO.saveAll(piecesOnBoard, connection); | ||
| Team currentTeam = chessGame.currentTeam(); | ||
| boolean saveTurnSuccess = turnDAO.save(currentTeam, connection); | ||
| commitTransaction(connection); |
There was a problem hiding this comment.
트랜잭션을 거는 코드가 반복되는데, 개선할 방법은 없을까요? 고민해 보셨다면, 이렇게 작성한 근거가 궁금해요 💭
| import java.sql.SQLException; | ||
| import java.util.List; | ||
|
|
||
| public class ChessPersistence { |
There was a problem hiding this comment.
클래스명에 Persistence를 넣은 의도는 알 것 같은데, 개인적으로 의미가 명확하게 와닿지 않네요!
Comment on lines
+28
to
+33
| private static class FakeConnection implements Connection { | ||
|
|
||
| @Override | ||
| public Statement createStatement() throws SQLException { | ||
| return null; | ||
| } |
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class TurnDAOForMysqlTest { | ||
| private static final String DB_URL = "jdbc:mysql://localhost:13306/chess2?useSSL=false&allowPublicKeyRetrieval=true&serverTimezone=UTC"; |
There was a problem hiding this comment.
프로덕트와 다른 DB를 사용하셨네요! 이 방법을 선택한 이유가 있을까요?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
원본 PR 참고해주세여
woowacourse#749