[체스 - 3, 4단계] 아토(이혜린) 미션 제출합니다. #24
Open
Conversation
This file contains 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
* docs: 기능 요구 사항 정리 * feat(King): 검정 팀 여부 확인 Co-authored-by: leegwichan <[email protected]> * docs: '팀 결정 여부 확인' 문서에 반영 Co-authored-by: leegwichan <[email protected]> * feat(Position): 각 위치 구분 - 가로 위치와 세로 위치 저장 - 가로 위치와 세로 위치가 같으면, 같은 객체로 판단 Co-authored-by: leegwichan <[email protected]> * feat(Board): 해당 위치의 말 확인 Co-authored-by: leegwichan <[email protected]> * feat(BoardFactory): 체스 말 위치 초기화 Co-authored-by: leegwichan <[email protected]> * refactor(domain): 패키지 분리 Co-authored-by: leegwichan <[email protected]> * feat(ChessGame): 초기 체스 보드 출력 - 대문자는 검정말, 소문자는 흰말로 출력 Co-authored-by: leegwichan <[email protected]> * refactor(OutputView): 메서드 분리 Co-authored-by: leegwichan <[email protected]> * refactor(ChessGame): 메서드 분리 Co-authored-by: leegwichan <[email protected]> * style(OutputView): 불필요한 공백 제거 Co-authored-by: leegwichan <[email protected]> * feat(InputView): 커멘드 입력 Co-authored-by: leegwichan <[email protected]> * feat(ChessGame): 종료 커멘드 반영 Co-authored-by: leegwichan <[email protected]> * style(Team): 개행 추가 Co-authored-by: leegwichan <[email protected]> * feat(Row): 남북 이동 Co-authored-by: leegwichan <[email protected]> * feat(Column): 동서 이동 Co-authored-by: leegwichan <[email protected]> * feat(Position): 다음 위치 확인 Co-authored-by: leegwichan <[email protected]> * refactor(Rank, File): 클래스명 변경 Co-authored-by: leegwichan <[email protected]> * feat(MovementRule): 연속적인 움직임 - 이동 가능 여부 판단 - 해당 경로 파악 Co-authored-by: leegwichan <[email protected]> * feat(KnightMovement): 나이트 움직임 - 이동 가능 여부 판단 - 해당 경로 파악 Co-authored-by: leegwichan <[email protected]> * feat(KingMovement): 킹 움직임 - 이동 가능 여부 판단 - 해당 경로 파악 Co-authored-by: leegwichan <[email protected]> * feat(Piece): 각 기물별 가능한 경로 파악 Co-authored-by: leegwichan <[email protected]> * feat(Board): 시작 위치의 말을 도착 위치로 이동 Co-authored-by: leegwichan <[email protected]> * feat(ChessGame): 이동 커멘드 반영 Co-authored-by: leegwichan <[email protected]> * feat(PawnDefaultMovement): 폰 기본 움직임 - 이동 가능 여부 판단 - 해당 경로 파악 Co-authored-by: leegwichan <[email protected]> * fix(Rook): 남쪽으로 이동할 수 있도록 수정 Co-authored-by: leegwichan <[email protected]> * feat(PawnFirstMovement): 폰 처음 움직임 - 이동 가능 여부 판단 - 해당 경로 파악 Co-authored-by: leegwichan <[email protected]> * style: 컨벤션에 맞추어 공백 추가 Co-authored-by: leegwichan <[email protected]> * style(KingTest): 불필요한 구문 제거 Co-authored-by: leegwichan <[email protected]> * feat(Pawn): 가능한 경로 파악 Co-authored-by: leegwichan <[email protected]> * refactor(Piece): stream을 이용하여 가독성 개선 Co-authored-by: leegwichan <[email protected]> * refactor(ContinuousMovementRule): 패키지 분리 Co-authored-by: leegwichan <[email protected]> * refactor(PieceTest): 팀 확인 테스트 이동 Co-authored-by: leegwichan <[email protected]> * style(Piece): 불필요한 구문 제거 Co-authored-by: leegwichan <[email protected]> * test(Piece): 가능한 경로 파악 Co-authored-by: leegwichan <[email protected]> * feat(Piece): 에러 메시지 추가 Co-authored-by: leegwichan <[email protected]> * test(Position): 파일과 랭크 차이 계산 Co-authored-by: leegwichan <[email protected]> * refactor(ChessGame): 메서드 분리 Co-authored-by: leegwichan <[email protected]> * feat(ChessApplication): 사용자에게 에러메시지 출력 후 종료 Co-authored-by: leegwichan <[email protected]> * feat(DiscreteMovementRule): 공통 로직 추상 클래스로 분리 Co-authored-by: leegwichan <[email protected]> * style(ContinuousMovementRule): 불필요한 개행 제거 Co-authored-by: leegwichan <[email protected]> * feat(DiscreteMovementRule): 에러 메시지 추가 Co-authored-by: leegwichan <[email protected]> * feat(PawnFirstMovement): 잘못된 파라미터 입력 시 에러 발생 Co-authored-by: leegwichan <[email protected]> * refactor: 가독성을 위해 상수 추출 Co-authored-by: leegwichan <[email protected]> * refactor(GameCommand): 불필요한 메서드 삭제 Co-authored-by: leegwichan <[email protected]> * feat(BoardFactory): 객체 생성을 막기 위해 생성자 추가 Co-authored-by: leegwichan <[email protected]> * refactor(NorthWestMovementTest): 불필요한 접근제어자 제거 Co-authored-by: leegwichan <[email protected]> * style: 불필요한 개행 삭제 Co-authored-by: leegwichan <[email protected]> * refactor: 디미터 규칙 준수 Co-authored-by: leegwichan <[email protected]> * style: 가독성을 위해 개행 추가 Co-authored-by: leegwichan <[email protected]> * refactor(Position): 불필요한 메서드 삭제 Co-authored-by: leegwichan <[email protected]> * docs: 개선 필요 사항 분리 Co-authored-by: leegwichan <[email protected]> * style(Position): 가독성을 위해 this 추가 Co-authored-by: leegwichan <[email protected]> * refactor(BoardFactory): 메서드명 변경 * refactor(PieceDto): 클래스로 변경 - 주 생성자를 private으로 변경 * feat(BoardDto): 체스 보드 출력을 위한 Dto 생성 - OutputView에서 null 사용 제거 - ChessGame의 역할 분리 * feat(Board): 팀 규칙 추가 - 흰 말부터 차례로 이동 - 같은 팀 말이 있는 위치로 이동 불가 * refactor(MovementRule): 상속되지 않을 클래스에 final 추가 * refactor(Position): 파라미터 변수명 변경 * docs: 기능 요구 사항 추가 * feat(MovementRule): 적 여부 파라미터에 추가 - 폰 이동 시 적 여부 판단해서 이동 가능 여부 반환 - 폰 대각선 이동 방법 추가 * feat(MovementRule): 도착 위치는 이동 경로에 포함되지 않도록 변경 - 이동 경로란 출발 위치에서 도착 위치까지 갈 때 거쳐가는 칸들 - 출발 위치에서 시작해 이동 경로를 지나 도착 위치에 도달 - 출발 위치에서 도착 위치 사이에 거쳐가는 칸이 없는 경우 빈 리스트 반환 * feat(ChessGame): 잘못된 입력 시 오류 메시지 출력 후 재입력 * style(ChessGame): 컨벤션에 맞춰 공백 추가 * refactor(File): 오타 수정 * refactor(Board): 메서드 간략화 --------- Co-authored-by: leegwichan <[email protected]>
Libienz
reviewed
Apr 3, 2024
Comment on lines
+5
to
+16
| public final class EastMovement extends ContinuousMovementRule { | ||
|
|
||
| @Override | ||
| protected boolean isMovable(int rankDifference, int fileDifference) { | ||
| return rankDifference == 0 && fileDifference > 0; | ||
| } | ||
|
|
||
| @Override | ||
| protected Position next(Position position) { | ||
| return position.moveToEast(); | ||
| } | ||
| } |
There was a problem hiding this comment.
1,2 단계와 관련된 얘기긴 한데 움직임이 지나치게 추상화 된 부분인 것 같음 🤔
isMovable이라는 메서드가 상위 클래스에서 같은 이름으로 오버로딩 되고 있는 것도 지나친 추상화로 가독성이 떨어지는 부분일 수 있을 것 같아
계층구조가 나한테는 한번에 와닿지 않았어
아토의 의도를 드러내기에 간단한 구조를 택하는 것도 한번 고민해보라는 의미에서 코멘트 남김 👍
Comment on lines
+36
to
+38
| public Piece createPiece(Team team) { | ||
| return pieceFactory.apply(team); | ||
| } |
There was a problem hiding this comment.
Piece의 생성 로직이 PieceType enum 내부에 있네
생성로직이 퍼져있는 부분이라고 생각해
물론 생성로직이 여러개일 수 있다고 생각하지만 숨겨져 있는 생성로직은 유지보수를 방해하는 요인이 될 수도 있다고 개인적으로 생각!
Comment on lines
+17
to
+21
| public Position(File file, Rank rank) { | ||
| this.file = Objects.requireNonNull(file); | ||
| this.rank = Objects.requireNonNull(rank); | ||
| } | ||
|
|
Comment on lines
+22
to
+28
| public static BoardDto of(Board board) { | ||
| List<Position> positions = Position.ALL_POSITIONS; | ||
| Map<Position, PieceDto> boardDto = new HashMap<>(); | ||
| positions.forEach(position -> addPiece(board, position, boardDto)); | ||
| Team turn = board.getTurn(); | ||
| return new BoardDto(boardDto, turn); | ||
| } |
There was a problem hiding this comment.
of라는 이름은 보통 정적 팩토리 메서드에서 파라미터가 2개 이상인 경우에 붙이는 이름인 것 같아
이미 알고 있을 수도 있지만 링크 투척 👀
Comment on lines
+30
to
+40
| private static void addPiece(Board board, Position position, Map<Position, PieceDto> boardDto) { | ||
| Optional<Piece> optionalPiece = board.find(position); | ||
|
|
||
| if (optionalPiece.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| Piece piece = optionalPiece.get(); | ||
| PieceDto pieceDto = PieceDto.from(piece); | ||
| boardDto.put(position, pieceDto); | ||
| } |
There was a problem hiding this comment.
기물을 추가한다는 너무 도메인 스러운 이름인 것 같아
처음에 해당 메서드를 보고는 DTO에 로직이 있으면 안되는데! 했는데 자세히 읽어보니 DTO 생성시에 변환을 위환 과정 중 하나라는 것을 알게되었어
메서드 시그니쳐 한번 수정해보는 것도 괜찮을 듯!
Comment on lines
+9
to
+11
| private final PieceType type; | ||
| private final boolean isBlack; | ||
|
|
There was a problem hiding this comment.
DTO는 전달객체로 전달에 집중해야 하는 레이어라고 생각!
그런데 그 전달의 대상은 도메인의 전달은 아닐 것 같아.
받는 쪽에서 필요로 하는 데이터가 무엇인지 살펴보고 그에 맞추어 도메인 의존성을 개선 해보면 어떨까
Comment on lines
+25
to
+27
| public static boolean isImpossibleBeforeStartGame(GameCommand command) { | ||
| return command == MOVE || command == STATUS; | ||
| } |
Comment on lines
+37
to
+42
| if (command == GameCommand.START) { | ||
| Board board = getBoard(); | ||
| showBoard(board); | ||
| play(board); | ||
| return; | ||
| } |
There was a problem hiding this comment.
커맨드에 따라서 분기를 하게 된다면 Command 객체가 하는일이 너무 없지 않을까?
나는 Command 객체가 있으면 해당 커맨드에서 수행되어야 하는 로직 또한 Command 클래스 안으로 응집되어야 한다고 생각했어
그런 생각을 적용하려고 하다보니 커맨드 패턴과 비슷한 형태라는 것도 알게되었지
아토도 꼭 적용이 아니더라도 관련해서 고민해보면 좋을 듯!
Comment on lines
+107
to
+112
| public Board loadBoard(int gameId) { | ||
| Team turn = getGameTurn(gameId); | ||
| Map<Position, Piece> pieces = getBoardPieces(gameId); | ||
| return new Board(pieces, turn); | ||
| } | ||
|
|
There was a problem hiding this comment.
우리가 DAO 계층을 이용하는 이유는 계층 외부에서 봤을 때는 다음의 메시지 때문이라고 생각해
- Board 넣어줘
- Board 꺼내줘
이러한 측면에서 봤을 때 해당 메서드에서 도메인인 Board를 반환하도록 하는 것이 나는 위화감 없다고 생각해!
다만 다른 크루들은 데이터 베이스에서 꺼내온 값을 변환하는 과정을 이를 이용하는 서비스 레이어에 두거나 entity계층을 운용하던데 아토는 어떻게 생각해!?
Comment on lines
+48
to
+54
| private Board getBoard() { | ||
| BoardDao boardDao = new BoardDao(); | ||
| if (boardDao.isExistBoard(ONE_GAME)) { | ||
| return boardDao.loadBoard(ONE_GAME); | ||
| } | ||
| return BoardFactory.createInitialBoard(); | ||
| } |
There was a problem hiding this comment.
BoardDao 는 하나의 계층으로 관리하는 것이 어떨까
다른 여러 기능들도 Dao를 이용할 텐데 그럴 때마다 이렇게 생성하도록 하면 불편한 점이 있을 것 같아
예로 BoardDao를 MySql이 아닌 다른 dialect를 사용하는 Dao로 바꾼다면 new를 통해 생성하고 있는 부분을 모두 찾아가야 할 것 같아
BoardDao를 속성으로 관리하고 의존성을 주입하도록 하면 위와 같은 문제상황에 유연할 수 있을 듯!
(속성이 너무 많아진다고 생각하면 계층 구조를 나누어볼수는 있을 것 같아!)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
구조가 크게 달라지지는 않았습니다!
다만 게임이 끝날 때
를 구분해야겠다는 생각이 들어 GameStatus를 추가했습니다!
점수 계산은 폰을 모두 1점으로 계산한 후, 0.5점이 되는 폰의 수를 세는 방식입니다.
이 방식이 다른 사람이 이해할만한지 궁금합니다.
+db에 언제 저장하고 어떻게 불러오는지 등에 관한 정보는 README.md에 함께 적어두었습니다!
혹시 이해 안가시는 부분이 있으시면 DM 주셔도 됩니다.
보시고 편하게 의견 주시길 바랍니다!
좋은 하루 되세요~~