자바 리팩토링

토이 프로젝트로 진행하던 체스게임에 대해서 간단하게 리팩토링을 해보겠습니다.

1. 변수명 의미있게 짓기

아래 코드는 체스기물 중 비숍의 이동 범위를 체크하는 로직에서 필요한 변수를 만드는 과정입니다.

변수명을 n1, n2, n3 이라고 변수명을 지으면 저는 알아보겠지만 다른 사람이 볼때는 다소 난해할 수 있습니다. 물론 시간지나면서 저도 이해하기 힘들 것 같습니다.

그래서 변수명을 목표좌표 x값에서 현재좌표 x값을 뺀 결과인 xDiff, 목표좌표 y값에서 현재좌표 y값을 뺀 결과인 yDiff로 이름을 바꿧습니다. 이 두개의 변수를 합친 결과는 xDiffAndYDiff라고 지었습니다.

• 변경 전 코드

int positionX = position.getX();
int positionY = position.getY();
int targetPositionX = targetPosition.getX();
int targetPositionY = targetPosition.getY();
int n1 = targetPositionX - positionX;
int n2 = targetPositionY - positionY;
int n3 = n1 + n2;

• 수정 코드

int x = position.getX();
int y = position.getY();
int targetX = targetPosition.getX();
int targetY = targetPosition.getY();
int xDiff = targetX - x;
int yDiff = targetY - y;
int xDiffAndYDiff = xDiff + yDiff;

2. 의미없는주석 제거

클린코드에서 주석보다는 코드로 의미를 잘 표현하는 게 중요하다고 합니다. 그래서 아래 코드들은 이름 그자체로 의미가 잘드러나니 주석은 불필요해서 제거 했습니다.

• 변경 전 코드

public enum PieceStatus {
    INVALID_MOVE,       // 잘못된 이동
    ONE_MOVE,           // 1수
    TAKES,              // 기물포획
    CASTLING,           // 캐슬링
    PROMOTION,          // 프로모션
    EN_PASSANT,         // 앙파상
}

// 범위체크
abstract boolean checkPieceRange(ChessBoard board, Position position, Position targetPosition);
// 행마
abstract PieceStatus logic(ChessBoard board, Position position, Position targetPosition);
// 해당위치 공격가능한지 체크
abstract boolean isPossibleAttack(ChessBoard board, Position position, Position targetPosition);

private PieceType pieceType;            // 기물유형
private Position position;              // 기물이전위치
private Position targetPosition;        // 기물다음위치
private final GameStatus gameStatus;    // 게임상태

• 수정 코드

public enum PieceStatus {
    INVALID_MOVE,
    ONE_MOVE,
    TAKES,
    CASTLING,
    PROMOTION,
    EN_PASSANT,
}

abstract boolean checkPieceRange(ChessBoard board, Position position, Position targetPosition);
abstract PieceStatus logic(ChessBoard board, Position position, Position targetPosition);
abstract boolean isPossibleAttack(ChessBoard board, Position position, Position targetPosition);

private PieceType pieceType;
private Position position;
private Position targetPosition;
private final GameStatus gameStatus;

3. 메서드 추출

체스에서는 폰이 상대방의 마지막 랭크에 가면 폰, 킹을 제외한 기물로 승진할 수 있습니다. 아래 코드는 프로모션인지 확인하는 로직이고 단순 조건문으로 체크하는 것 보다는 의미를 알 수 있는 별도의 함수로 만들었습니다.

• 변경 전 코드

if((getPlayerType() == WHITE && targetPosition.getX() == 0)
|| (getPlayerType() == BLACK && targetPosition.getX() == 7)) {
	board.setPiece(this, targetPosition);
   	return PROMOTION;
}

• 수정 코드

if(checkPromotion(board, position, targetPosition)) {
	board.setPiece(this, targetPosition);
	return PROMOTION;
}

private boolean checkPromotion(ChessBoard board, Position position, Position targetPosition) {
	return (getPlayerType() == WHITE && targetPosition.getX() == 0)
            || (getPlayerType() == BLACK && targetPosition.getX() == 7);
}

4. 단일한 테스트

다음은 킹의 이동 중 잘못된 경우를 테스트하는 코드입니다. 하나의 메서드에서 여러 경우의 수를 테스트하고 있는데 만약 하나의 케이스를 수정 시 전체 케이스를 수행해야 하므로 다소 비효율적입니다. 그래서 하나의 케이스 당 하나의 단위테스트 메서드를 만들었습니다.

• 변경 전 코드

@Test
@DisplayName("잘못된이동")
void invalid_move_king() {
    King king = (King)chessBoard.getPiece(Position.of(7, 4));

    // 보드판 범위를 벗어난 경우
    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(8, 4));

    // 가는 길 중간에 기물이 있는 경우
    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(6, 4));

    // 같은 위치로 이동한 경우
    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(7, 4));

    // 동일 플레이어의 기물을 공격한 경우
    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(7, 5));

    assertEquals(king, chessBoard.getPiece(Position.of(7,4)));
}

• 수정 코드

@Test
@DisplayName("보드판_범위를 벗어난_경우")
void out_of_board_range() {
    Piece king = chessBoard.getPiece(Position.of(7, 4));

    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(8, 4));

    assertEquals(king, chessBoard.getPiece(Position.of(7,4)));
}

@Test
@DisplayName("가는길_중간에_기물이_있는_경우")
void if_there_is_piece_on_the_way() {
    Piece king = chessBoard.getPiece(Position.of(7, 4));

    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(6, 4));

    assertEquals(king, chessBoard.getPiece(Position.of(7,4)));
}

@Test
@DisplayName("같은_위치로_이동한_경우")
void moved_to_the_same_location() {
    Piece king = chessBoard.getPiece(Position.of(7, 4));

    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(7, 4));

    assertEquals(king, chessBoard.getPiece(Position.of(7,4)));
}

@Test
@DisplayName("동일플레이어의_기물을_공격한_경우")
void attacking_the_same_player_pieces() {
    Piece king = chessBoard.getPiece(Position.of(7, 4));

    king.move(chessBoard, WHITE, Position.of(7,4), Position.of(7, 5));

    assertEquals(king, chessBoard.getPiece(Position.of(7,4)));
}