[1, 2단계 - 체스] 테오(최우성) 미션 제출합니다. #460
Conversation
일급 컬렉션 생성을 통한 조회, 변경, 검사 분리
There was a problem hiding this comment.
안녕하세요 테오, 리뷰어 카프카입니다.
저번에 드린 피드백에 대해 논의를 많이 진행했는데, 코드를 확인해보니 논의한 내용대로 잘 반영이 되어 있네요. 💯
코드 퀄리티가 많이 좋아져서 현재 미션의 요구사항을 잘 충족하고 있다고 생각됩니다.
다만 구조를 조금 더 개선했으면 싶은 부분들이 있어서 코멘트를 남겨두었는데요,
이 부분 참고해보시고, 다음 스탭 진행하면서 반영해주시면 될 것으로 생각됩니다.
- [1, 2단계 - 체스] 테오(최우성) 미션 제출합니다. #460 (comment) 이 코멘트도 참고 바랍니다.
작업하시느라 고생 많으셨습니다.
이번 스탭 approve 하겠습니다. 👍
| @@ -5,3 +5,76 @@ | |||
| ## 우아한테크코스 코드리뷰 | |||
|
|
|||
| - [온라인 코드 리뷰 과정](https://github.com/woowacourse/woowacourse-docs/blob/master/maincourse/README.md) | |||
|
|
|||
There was a problem hiding this comment.
move 부분에 대해 예외 설정된 것 확인했습니다.
턴 부분과 기물 잡기는 아직 구현되지 않은 것 확인하였습니다. 이부분은 다음 단계에서 함께 확인하겠습니다.
| import domain.square.Square; | ||
| import domain.piece.Coordinate; | ||
|
|
||
| public final class ChessGame { |
There was a problem hiding this comment.
이번에 구조개선을 요청드리면서 ChessGame 클래스가 만들어졌는데, 내부 구조를 잘 만들어 주셨네요.
게임에서 수행하는 move 동작과 해당 동작에 대한 검증을 책임지는데, 코드를 명확하게 잘 작성해 주셨다고 생각합니다 💯
| "> 게임 이동 : move source위치 target위치 - 예. move b2 b3"; | ||
| public static final String GAME_END_MESSAGE = "게임을 종료합니다."; | ||
|
|
||
| public void printBoard(final ChessGame chessGame) { |
There was a problem hiding this comment.
OutputView에 ChessGame을 보내주는 것은 좋지 않은 방법이라고 생각합니다.
OutputView에서는 List < String > (라인별로 String에 넣어서 보낼 경우) 혹은 String(전체 체스판 정보를 개행문자 포함하여 저장한 경우) 을 받아서 출력해주는 것이 도메인에 대해 모르는 구조가 되지 않을까요?
저라면 Piece를 상속한 클래스들이 자신의 알파뱃 값을 가지게 하고, Square는 해당 값 + camp 값을 바탕으로 해당 칸이 어떻게 출력될지의 정보를 반환하도록 구조를 짤 것 같아요. 그리고 Board는 이를 바탕으로 전체 칸의 출력 형태를 만들고, 이를 View에 전달하게 되는 것이지요.
There was a problem hiding this comment.
다만 현재 구조를 보면 PieceTypeMapper 를 통해 뷰에서 체스말의 출력 형태를 정하도록 해주고 있네요.
만약 해당 작업이 뷰의 책임이라고 본다면 현재 구조를 유지하는 것이 옳을 수도 있습니다.
다만 이 구조 역시 Piece class에 대한 정보를 View 레이어에서 가지게 되는 점이 아쉽긴 합니다.
출력 작업에 대해 어디에서 어떤 책임을 져야 할지 명확히 정리하고, 그에 맞게 리팩토링을 수행해야겠네요.
이부분은 다음 스탭에서 개선 방향을 정해보면 좋을 것으로 생각됩니다.
- 만약 현재 구조가 옳다고 생각하신다면, 유지하시되 ChessGame을 넘겨주는 대신 DTO를 넘겨주는 방향으로 개선이 가능할지 고려해보기를 바랍니다.
There was a problem hiding this comment.
감사합니다!
아직 명확한 구조 개선 방향이 떠오르지는 않지만, 3단계에서 반영해 제출해보겠습니다.
카프카가 말씀해주신대로, View에서는 Domain Layer에 대해 직접적으로 의존하지 않도록 변경해볼게요.
그때 가서 또 한번 짚어주시면 감사하겠습니다!
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Y축 좌표가 알파벳 소문자가 맞는지를 검증한다") |
There was a problem hiding this comment.
해당 테스트명을 보면 Y축 좌표값이 대문자로 들어오는 경우에 대해 테스트해야 할 것 같네요.
그리고 정상 동작에 대한 테스트를 하나 추가해서, 올바르게 들어온 경우 convert를 수행한 Coordinate 클래스의 row,col 값이 올바른지 검증해보기를 바랍니다.
| @Test | ||
| @DisplayName("킹은 정상 생성이 된다") | ||
| void constructorTest() { | ||
| assertThatCode(King::new) | ||
| .doesNotThrowAnyException(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("킹은 디폴트 상태를 가진다") | ||
| void propertyTest() { | ||
| Piece king = new King(); | ||
|
|
||
| assertThat(king.canReap()).isFalse(); | ||
| assertThat(king.isPawn()).isFalse(); | ||
| } |
There was a problem hiding this comment.
제가 정상 테스트에 대해 코멘트드린 부분의 의미는, 아래의 디폴트 상태를 가진다 테스트처럼 정상 테스트를 수행하라는 의미였습니다.
- 클래스가 정상 생성될 경우, 기대되는 값도 올바르게 들어갔는지 검증했다면 그것이 정상 테스트라고 생각합니다.
그래서 위의정상 생성이 된다테스트는 없어도 될 것으로 보입니다.
만약 생성 과정에서 예외가 발생한다면, 아래의 테스트도 같이 실패할 테니까요.
There was a problem hiding this comment.
이부분은 저번에 코멘트를 드리면서 제가 명확하게 설명드리지 못했던 것 같네요.
그래도 피드백을 드린 방향에 맞게 잘 작성해주셔서, 충분히 잘 이해하고 계시다고 생각됩니다 😄
There was a problem hiding this comment.
감사합니다!
확실히 아래의 디폴트 상태를 가진다를 체크하는 테스트가 생성 그 자체까지 체크하고 있는 것 같습니다.
중복 테스트는 제거하도록 하겠습니다! 👍
안녕하세요 카프카 반갑습니다!!
리뷰 잘 부탁드립니다 👍 바쁘신데 감사해요!!
스스로 고민해볼 수 있는 부분들은 제외하고, 카프카에게 한 가지 의견을 여쭙고 싶은게 있는데요,
이번에 Pawn이나 Knight를 구현하는 과정에서,
다형성과복잡한 기능사이의 충돌이 있었던 것 같습니다.다형성을 사용하기는 해야 하는데, Pawn이나 Knight의 움직임은 다형성만으로 다룰 수가 없었기에 고민을 많이 했던 것 같습니다.
추상화 레벨을 깨더라도 위 타입의 객체가 아래 타입의 객체를 알게 할 것인지.. 고민을 많이 했던 것 같습니다.
복잡한 도메인에서는 이런 충돌은 불가피한 부분일까요?
아니면 모두를 수용할 수 있는 좋은 설계가 존재하는지.. 궁금합니다.
이외에도 해주시는 모든 리뷰 꼼꼼히 생각해보며 흡수하겠습니다. 감사합니다!! 😊