[체스 - 3, 4 단계] 리비(이근희) 미션 제출합니다. #17
Conversation
| public void printChessBoardMessage(ChessBoard chessBoard) { | ||
| System.out.println(resolveChessBoardMessage(chessBoard)); | ||
| public void printChessBoardMessage(BoardDto boardSnapshotDto) { | ||
| System.out.println(resolveBoardSnapshotMessage(boardSnapshotDto)); | ||
| } |
There was a problem hiding this comment.
도메인 객체를 사용하다가, DTO를 사용하게 된 계기가 있을까?
There was a problem hiding this comment.
체스는 특히 단계에 걸친 확장 요구사항이 많은 미션이었던 것 같아
확장 요구사항을 경험할 일이 적었던 이전 미션들과 달리 이제는 사이드 이펙트를 고려해야 될 시기라고 생각했으 👍
| public ChessGame loadChessGame() { | ||
| PiecesDto pieces = pieceRepository.findPieces().get(); | ||
| Team currentTurn = turnRepository.findCurrentTurn().get(); | ||
| ChessBoard chessBoard = DomainMapper.mapToBoard(pieces); | ||
| return new ChessGame(chessBoard, currentTurn); | ||
| } |
There was a problem hiding this comment.
매 명령마다 DB로부터 정보를 불러와서 체스게임을 하나씩 만든 후에 작업을 하는데,
지금과 같이 설계한 이유가 있어? (처음으로 한 번 불러와서 계속 같은 ChessGame을 이용해도 되지 않을까라는 생각)
There was a problem hiding this comment.
우선 멀티쓰레드 요청 환경을 예상할 때 상태를 가지는 것은 좋지 않다고 생각했어
(실제로 웹요청도 요청자가 누구인지 기억하고 있지는 않으니까)
하지만 지금은 커찬이 말한대로 불필요한 작업일지는 모르겠네 🤔
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
| private final ChessGameService chessGameService; |
There was a problem hiding this comment.
나도 Service를 이용했는데, 이 부분에서 필드를 세 개 사용한다는 지적을 받았어.
어떻게 이야기 해야 될까?
There was a problem hiding this comment.
나도 어떻게 해야 할지 속으로만 생각했는데 컨트롤러를 호출하는 Application 클래스를 요청자로써 새로 정의할 것 같아
Application을 요청자로 정의하여 InputView와 OutputView를 할당하고 Application이 컨트롤러에 요청할 때는 입력받은 String 혹은 Command로 요청하도록 하는 거지.
이렇게 하면 컨트롤러는 들어오는 요청과 요청을 위임할 서비스 레이어만 알게되고 어플리케이션 클래스가 InputView와 OutputView를 아는 요청자가 된다는 측면에서 하나의 해결책이 될 수 있지 않을까 생각함 👍
| @Override | ||
| public Score score() { | ||
| return new Score(SCORE_VALUE); | ||
| } |
There was a problem hiding this comment.
스코어 객체를 매번 만들지 말고, 만들어진 객체를 들고 있는 건 어때?
private static final Score SCORE = new Score(3.0);
@Override
public Score score() {
return SCORE;
}| import chess.domain.position.Position; | ||
| import chess.domain.position.Rank; | ||
|
|
||
| public class ValueMapper { |
There was a problem hiding this comment.
위와 같은 Mapper 클래스를 생성하는 대신에, 이것을 사용하는 곳에 해당 로직을 놓는게 좋다고 생각해.
나는 Mapper를 보면서, 해당 텍스트들이 DB에 어떻게 저장되는지를 결정하는데, 이 정보가 Repository와 떨어져 있다는 생각이 들었어.
리비의 생각은 어때?
There was a problem hiding this comment.
확실히 두가지 방법이 있는 것 같아
나의 현행처럼 클래스를 분리하거나 이를 사용하는 클래스의 메서드로 배치하거나.
트레이드 오프지점이라고 생각해 클래스로 분리할 경우에 클래스 복잡도가 올라가고 작성해야 하는 코드양이 많아지지만 책임 분리 관점에서 좋지 않을까?
반대의 경우에는 책임분리 측면에서 단점이 있고 이를 높게 평가하여 나는 이렇게 구현했음을 말해주고 싶음! 👍
| void should_ReturnDirectionNorth() { | ||
| assertThat(of(A1, A2)).isEqualTo(N); | ||
| assertThat(Direction.of(A1, A2)).isEqualTo(Direction.N); | ||
| } |
There was a problem hiding this comment.
그냥 of()라고 쓰는 것보다 Direction.of()로 쓰니까 훨씬 가독성이 좋아진 것 같아!
| class ConnectionManagerTest { | ||
| private final ConnectionManager connectionManager = new ConnectionManager(); | ||
|
|
||
| @DisplayName("DB 커넥션을 성공적으로 얻어올 수 있는지 테스트") | ||
| @Test | ||
| void should_ConnectionManagerCouldGetConnection() { | ||
| try (final var connection = connectionManager.getConnection()) { | ||
| assertThat(connection).isNotNull(); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 테스트는 도커가 정상적으로 실행하는 상태에서만 통과될 것 같아.
외부 상황에 따라서 통과하는 테스트는 지양하는 편이 좋을 것 같아.
There was a problem hiding this comment.
커넥션 정도는 테스트 하기 위화감 없었다고 생각했는데 이또한 도커라는 외부 리소스의 영향을 받는 테스트 였네 🤔
짚어줘서 감사 커찬 👍
There was a problem hiding this comment.
데이터베이스와 통신하는 부분도 분명히 테스트 되어야 하는 로직입니다. 따라서, 이런 테스트가 있는 것이 오히려 더 좋다고 생각합니다. 다만, 이건 단위 테스트가 아니라 통합테스트가 될 뿐이죠.
| private ChessGameService chessGameService; | ||
| @Mock | ||
| private PieceRepository pieceRepository; | ||
| @Mock | ||
| private TurnRepository turnRepository; |
There was a problem hiding this comment.
외부 라이브러리 추가 없이 Mocking 하는 방법 배워갑니다~ 굳.
There was a problem hiding this comment.
실은 외부라이브러리 추가가 있었다는 👀
27c723b
There was a problem hiding this comment.
외부 라이브러리를 통한 Mock 보다는 인터페이스를 사용하는 것이 더 좋을 것 같아요.
| public class StatusCommand implements Command { | ||
|
|
||
| @Override | ||
| public ExecuteResult execute(ChessGameService chessGameService, OutputView outputView) { | ||
| ScoreStatusDto scoreStatusDto = chessGameService.calculateScoreStatus(); | ||
| outputView.printStatusMessage(scoreStatusDto); | ||
| return new ExecuteResult(true, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
나도 이런 형식의 Command 도입을 고려했었는데 각 커멘드마다 로직이 여러 파일로 분리되는 것 같아 꺼려했었어
리비는 일종의 상태 패턴을 도입했을 때, 어떤 장점을 얻었어?
There was a problem hiding this comment.
나만의 커맨드 패턴에 대한 소감있지 ㅋㅋ
우선 나는 만약에 그냥 명령의 타입만 확인하고 컨트롤러 코드에서 다음과 같이 분기한다면 그건 책임이 잘못된 것이라고 생각해.
// 컨트롤러 내부 코드
if(command == Command.START) {
// do someting
}
if(command == Command.END) {
// do someting
}
if(command == Command.STATUS) {
// do someting
}
if(command == Command.MOVE) {
// do someting
}명령이라는 도메인을 정의했을 때는 응당 해당 명령에 대한 동작을 응집할 수 있어야 한다고 생각했어
그리고 이러한 것을 가능하게 한 것이 커맨드 패턴이었네
커맨드 패턴을 통해서 결국 기능 배치에 있어서 책임에 위화감 없는 개선 할 수 있었다고 생각함 👍
| private ChessGameService chessGameService; | ||
| @Mock | ||
| private PieceRepository pieceRepository; | ||
| @Mock | ||
| private TurnRepository turnRepository; |
There was a problem hiding this comment.
외부 라이브러리를 통한 Mock 보다는 인터페이스를 사용하는 것이 더 좋을 것 같아요.
| class ConnectionManagerTest { | ||
| private final ConnectionManager connectionManager = new ConnectionManager(); | ||
|
|
||
| @DisplayName("DB 커넥션을 성공적으로 얻어올 수 있는지 테스트") | ||
| @Test | ||
| void should_ConnectionManagerCouldGetConnection() { | ||
| try (final var connection = connectionManager.getConnection()) { | ||
| assertThat(connection).isNotNull(); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
데이터베이스와 통신하는 부분도 분명히 테스트 되어야 하는 로직입니다. 따라서, 이런 테스트가 있는 것이 오히려 더 좋다고 생각합니다. 다만, 이건 단위 테스트가 아니라 통합테스트가 될 뿐이죠.
| Piece blackPiece = new King(Team.BLACK); | ||
| Piece whitePiece = new King(Team.WHITE); | ||
| Map<Position, Piece> board = new HashMap<>(); | ||
| board.put(PositionFixtures.A1, whitePiece); | ||
| ChessBoard chessBoard = new ChessBoard(board); | ||
| boolean canMoveSamePosition = whitePiece.canMove(PositionFixtures.A1, PositionFixtures.A1, chessBoard); | ||
|
|
||
| assertThat(canMoveSamePosition).isFalse(); |
There was a problem hiding this comment.
개행을 어떤 테스트에서는 하고, 어떤 테스트에서는 안했는데, 그 이유가 뭔가요?
| if (piece instanceof Pawn) { | ||
| return "pawn"; | ||
| } | ||
| if (piece instanceof Rook) { | ||
| return "rook"; | ||
| } | ||
| if (piece instanceof Knight) { | ||
| return "knight"; | ||
| } | ||
| if (piece instanceof Bishop) { | ||
| return "bishop"; | ||
| } | ||
| if (piece instanceof Queen) { | ||
| return "queen"; | ||
| } | ||
| if (piece instanceof King) { | ||
| return "king"; | ||
| } | ||
| throw new IllegalArgumentException("알 수 없는 피스 타입입니다"); |
There was a problem hiding this comment.
이렇게 할거면 스위치 문을 쓰는 것이 차라리 더 가독성이 좋은 것 같아요.
| public Connection getConnection() { | ||
| try { | ||
| return DriverManager.getConnection("jdbc:mysql://" + SERVER + "/" + DATABASE + OPTION, USERNAME, PASSWORD); | ||
| } catch (final SQLException e) { | ||
| throw new RuntimeException("DB 연결 오류"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Connection은 비싼 자원이기 때문에, 재사용을 고려해야 할 것 같아요.
There was a problem hiding this comment.
커넥션 풀로 재사용성을 고려한 크루들이 있던데 한번 물어봐야겠네요 👀
| public ExecuteResult(boolean success, boolean needNextCommand) { | ||
| this.success = success; | ||
| this.needNextCommand = needNextCommand; | ||
| } |
There was a problem hiding this comment.
boolean 타입 변수 두개가 연속으로 나와서, 아주 헷갈릴 것 같아요. 래핑 하는 것이 나을 것 같아요.
| public abstract boolean isPawn(); | ||
|
|
||
| public abstract boolean isKing(); |
There was a problem hiding this comment.
이 두 메서드는 각각 Pawn 과 King만 내부 구현이 다를 것으로 판단되는데, 이러면 추상 메서드가 아니라 바로 정의하고, Pawn과 King에서만 재정의 하는 것이 코드가 더 깔끔할 것 같아요!
| public Team getTeam() { | ||
| return team; | ||
| } |
| import chess.repository.TurnRepository; | ||
| import chess.repository.mapper.DomainMapper; | ||
|
|
||
| public class ChessGameService { |
| try (Connection connection = connectionManager.getConnection(); | ||
| PreparedStatement pstmt = connection.prepareStatement(query); | ||
| ResultSet resultSet = pstmt.executeQuery()) { | ||
|
|
||
| while (resultSet.next()) { | ||
| String position = resultSet.getString("position"); | ||
| String team = resultSet.getString("team"); | ||
| String type = resultSet.getString("type"); | ||
|
|
||
| result.add(new PieceDto(position, team, type)); | ||
| } | ||
| if (!result.isEmpty()) { | ||
| return Optional.of(new PiecesDto(result)); | ||
| } | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException("기물 조회 과정 중 오류 발생"); | ||
| } |
No description provided.