[라빈] 체스 미션 1주차 제출합니다 #71
Conversation
refactor: final 키워드 추가
- 게임을 진행하는 ChessRunner 클래스 구현
| chessRunner.update(source, target); | ||
| printBoard(chessRunner.getBoard()); | ||
| if (findWinner(chessRunner)) { | ||
| runChess(chessRunner); |
There was a problem hiding this comment.
재귀에 맡기는 방법이 아닌 반복을 통한 방법으로 수정해보면 어떨까요?
재귀를 통한 방법으로 진행되면 게임이 오래 진행될 수록 스택 오버 플로가 발생할 위험이 있겠네요. :)
There was a problem hiding this comment.
메모리 관리에 대해서 제대로 생각하지 않고 작성을 했었네요 ㅠㅠㅠ
반복을 통해 수행하도록 메서드를 수정했습니다 :)
| Team winner = chessRunner.findWinner(); | ||
| if (winner != null) { | ||
| outputView.printWinner(winner); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
승자가 있는지에 대한 체크 조차 사용하는측에서 null을 통한 확인보다는 ChessRunner가 제공하는게 있는게 좋지않을까요? :)
There was a problem hiding this comment.
ChessRunner의 책임을 더 명확히 하고 컨트롤러에서는 ChessRunner 만 이용하도록 변경했습니다!
| printBoard(chessRunner.getBoard()); | ||
| if (findWinner(chessRunner)) { |
There was a problem hiding this comment.
runByCommand에서 END시에 return 시키고
이렇게 모든 커맨드에 반복되는 행위에 대해서는 runByCommand에 포함시키는 방법도 있을 것 같네요. :)
왜냐하면 현재 로직 상 새로운 커멘드가 추가될 때마다 이러한 공통 로직(현재로서는 보드 출력, 혹은 승자 도출)을 중복으로 넣어줘야하는 번거로움이 있을 것 같아서요.
| } | ||
|
|
||
| private void changeTeam() { | ||
| this.currentTeam = Team.changeTeam(this.currentTeam); |
There was a problem hiding this comment.
static factory method가 아닌 현재 enum 자체에서 메서드로 제공해줄수도 있었을 것 같네요. :)
this.currentTeam = currentTeam.changeTeam()
| Position targetPosition = Position.of(target); | ||
| Piece selectedPiece = this.board.getPiece(sourcePosition); | ||
|
|
||
| if (!(currentTeam.isSameTeamWith(selectedPiece.getTeam()))) { |
There was a problem hiding this comment.
selectedPiece.getTeam()으로 getter를 통해 내부 구현을 밖으로 가져오지 말고,
메서드의 방향을 반대로하여 구현을 숨겨보면 어떨까요?
Piece에게 팀을 전달하여 확인하도록이요. :)
There was a problem hiding this comment.
getter를 사용하여 내부 구현을 꺼내서 확인하는 습관이 아직 제대로 고쳐지지 않은것 같습니다! 지적 감사합니다!
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum File { |
There was a problem hiding this comment.
체스에 관한 규칙을 찾아보던 중 체스 보드의 행은 파일, 열은 랭크라는 말로 불러서 적용했습니다 :)
| public boolean movable(final Position source, final Position target, final Board board) { | ||
| return this.pieceType.movable(source, target, board); | ||
| } |
There was a problem hiding this comment.
로직을 한번 살펴보니 Piece에 너무 많은 책임을 가지고 있네요. 게임 전반의 모든 로직을 Piece가 가지고 있어보여요.
그 때문에 Piece로부터 따라가는 모든 로직들이 Board에 대한 의존을 양방향으로 계속 이어가지고 있네요. Board에 대한 의존을 분리해보면 어떨까요?
There was a problem hiding this comment.
다시 저와 페어가 설계한 도메인을 천천히 살펴보니 말씀해주신대로 양방향으로 의존을 계속 하고 있어서 Board 와 Piece 의 책임도 명확해지지 못했습니다.
PawnMoveStrategy에서 리뷰 주신 내용 반영하면서 각 클래스별로 책임 분리를 명확히 하는 과정에서 양방향 의존을 없앴습니다 :)
| pieces.put(Position.of("a7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("b7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("c7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("d7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("e7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("f7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("g7"), new Piece(PieceType.PAWN, Team.BLACK)); | ||
| pieces.put(Position.of("h7"), new Piece(PieceType.PAWN, Team.BLACK)); |
There was a problem hiding this comment.
하드코딩 보다는 이미 작성해둔 enum을 통해 초기화해주시는게 좋을 것 같아요. :)
이 경우 수정이 일어나도 런타임에서나 체크가 가능할 것 같네요.
There was a problem hiding this comment.
각 Initializer 클래스 내부에 위치를 담고있는 enum 클래스를 private 로 선언하여서 그 값들을 불러서 초기화 해주도록 바꾸었습니다.
| if (pawnTeam.isWhite()) { | ||
| if (fileGap == 0 && rankGap == -1) { | ||
| return board.isEmpty(target); | ||
| } | ||
| if (fileGap == 0 && rankGap == -2) { | ||
| return source.getRank() == WHITE_POSITION; | ||
| } | ||
| if (Math.abs(fileGap) == 1 && rankGap == -1) { | ||
| return !board.isEmpty(target) && sourcePiece.isEnemy(targetPiece); | ||
| } | ||
| } | ||
| if (!pawnTeam.isWhite()) { | ||
| if (fileGap == 0 && rankGap == 1) { | ||
| return board.isEmpty(target); | ||
| } | ||
| if (fileGap == 0 && rankGap == 2) { | ||
| return source.getRank() == BLACK_POSITION; | ||
| } | ||
| if (Math.abs(fileGap) == 1 && rankGap == 1) { | ||
| return !board.isEmpty(target) && sourcePiece.isEnemy(targetPiece); | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
각 Piece를 enum이 아닌 상속구로조 푸는것도 하나의 방법이었을 것 같네요. :)
Board에 의존하지 않고 Pawn 구현 + Team의 포함관계로 백 Pawn인지 흑 Pawn인지에 따라 스스로 처리가 가능했을 것 같구요.
PawnMoveStrategy자체가 실제 Piece와는 관계가 없다보니 로직이 복잡해지는 부분도 있어보여요.
Piece sourcePiece = board.getPiece(source);
Piece targetPiece = board.getPiece(target);
해당 객체가 실제로 Pawn 외 해당 피스에 해당하는 객체인지 확인조차 없어서 실수할경우 오작동할 위험도 있네요. :)
There was a problem hiding this comment.
말씀해주신 내용을 생각해 보고 Piece 를 상속 구조로 바꿔서 구현을 했더니 Piece 와 Board 의 양방향 의존성도 사라지고 Piece 내부에서 본인에 맞는 이동 전략을 구현하는 설계를 얻을 수 있었습니다 :)
| int fileGap = Math.abs(source.calculateFileGap(target)); | ||
| int rankGap = Math.abs(source.calculateRankGap(target)); | ||
|
|
||
| return (fileGap == 2 && rankGap == 1) || (fileGap == 1 && rankGap == 2) |
There was a problem hiding this comment.
하드코딩들은 다 상수로 처리해주세요. :)
혹은 체스말이 이동 가능한 위치는 한정적이니 Direction같이 enum을 활용하는것도 방법일 것 같네요.
…/ 각 체스 말을 초기화 할때의 정보를 enum 으로 구현
|
화투 안녕하세요! |
| return !this.board.containsKey(position); | ||
| } | ||
|
|
||
| public Optional<Piece> getPiece(final Position position) { |
|
|
||
| public String getWinner() { | ||
| Optional<Team> winner = this.board.getWinner(); | ||
| return winner.map(Enum::name).orElseThrow(AssertionError::new); |
There was a problem hiding this comment.
아이템 75. 예외의 상세 메시지에 실패 관련 정보를 담으라
아이템 69. 예외는 진짜 예외 상황에만 사용하라
승자가 결정되지 않는게 예외상황인지도 고려해볼 필요가 있을 것 같아요.
| Piece whitePawn = new Pawn(PieceType.PAWN, Team.WHITE); | ||
| Position initialSource = Position.of("a2"); | ||
| Position directOne = Position.of("a3"); | ||
| Position directTwo = Position.of("a4"); | ||
| Position nonMovableInitial = Position.of("a5"); | ||
|
|
||
| Position nonInitialSource = Position.of("b3"); | ||
| Position diagonalTarget = Position.of("c4"); | ||
| Position directTarget = Position.of("b4"); | ||
| Position nonMovableTarget = Position.of("b5"); | ||
|
|
||
| Assertions.assertThat(whitePawn.movable(initialSource, directOne)).isTrue(); | ||
| Assertions.assertThat(whitePawn.movable(initialSource, directTwo)).isTrue(); | ||
| Assertions.assertThat(whitePawn.movable(initialSource, nonMovableInitial)).isFalse(); | ||
|
|
||
| Assertions.assertThat(whitePawn.movable(nonInitialSource, diagonalTarget)).isTrue(); | ||
| Assertions.assertThat(whitePawn.movable(nonInitialSource, directTarget)).isTrue(); | ||
| Assertions.assertThat(whitePawn.movable(nonInitialSource, nonMovableTarget)).isFalse(); |
There was a problem hiding this comment.
단위 테스트 당 하나만 테스트 할 수 있도록 해주세요. :)
중간에 테스트가 실패할 경우 이후 테스트 성공 여부 확인이 어려워요.
| return Command.of(inputView.askChessRun()); | ||
| } catch (IllegalArgumentException e) { | ||
| System.out.println(e.getMessage()); | ||
| return getCommand(); |
안녕하세요!
체스 1단계 - 체스판 초기화
체스 2단계 - 말 이동
체스 3단계 - 승패 및 점수 구현해서 제출합니다.
기능 구현은 끝났고 코드에 개선할 부분이 아직 남아서 빠르게 작성해서 다시 말씀드리겠습니다.
감사합니다!