[우아한 테크코스 3기] LEVEL 1 회고 - 체스 1, 2, 3단계 미션 1차 피드백을 받아보다(49일차)

 안녕하세요? 제이온입니다.

 

오늘은 체스 1, 2, 3단계 미션 1차 피드백을 열심히 반영해 보았습니다.

 

 

1차 피드백

이번 리뷰어님은 김고래였습니다. 어제 오후 즈음에 1차 피드백이 완료되었다는 메시지를 받고, 바로 깃허브에 들어가 확인해 보았습니다. 수정 사항이 17개나 돼서 좀 불안했는데, 가볍게 훑어보니까 다행히 체스 미션의 근본적인 로직에 문제는 없었습니다. 정말 사소한 것은 제외하고, 제가 시간을 들여서 리팩토링하였던 부분만 따로 가져와서 말씀드리겠습니다.

 

 

 

첫 번째 수정 사항은 객체가 할 일을 분산하는 것입니다. 위에는 Horizontal 열거형 클래스에서 symbol 이라는 문자열 변수만을 모아서 출력하는 코드입니다. 이를 BoardInitializer에서 하지 말고, Horizontal 열거형 클래스 내부에서 하라는 것입니다. 따라서 아래와 같이 수정하였습니다.

 

 

    public static List<String> horizontalSymbols() {
        return Arrays.stream(values())
            .map(Horizontal::symbol)
            .collect(Collectors.toList());
    }

 

 

비슷한 방식으로 Vertical도 정의하였습니다.

 

 

    private static final List<LocationInitializer> locationInitializers;
    private static final List<String> HORIZONTAL_RANGE = Horizontal.horizontalSymbols();
    private static final List<String> VERTICAL_RANGE = Vertical.verticalSymbols();

 

 

그래서 BoardInitializer 클래스의 상수가 상당히 깔끔해졌습니다.

 

 

 

 

두 번째 수정 사항은 불필요한 인자를 제거하는 것입니다. ChessGame 객체는 내부에 turn을 갖고 있는데, 이를 컨트롤러에서 호출하여 따로 인자를 넘기는 것이 아니라, 내부에서 처리하는 것이죠.

 

 

public class ChessGame {

    private final Board board;
    private final Turn turn;
    private final GameOver gameOver;

    public ChessGame() {
        board = new Board();
        turn = new Turn();
        gameOver = new GameOver();
    }

    public void changeGameOver() {
        gameOver.changeGameOver();
    }

    public boolean isGameOver() {
        return gameOver.isGameOver();
    }

    public void nextTurn() {
        turn.next();
    }

    public boolean isKingDead() {
        return board.isKingDead();
    }

    public void move(final Position start, final Position end) {
        board.move(start, end, turn.now());
    }

    public Board board() {
        return board;
    }
}

 

 

위와 같이 내부에서 move 메소드의 turn 인자를 제거하고, 필드 인스턴스 변수를 사용하였습니다.

 

 

 

 

세 번째 수정 사항은 instanceof 키워드를 사용하지 말라는 것입니다. 위에 조언에서 나와있는대로, 이것은 다형성을 깨뜨리기 때문에 차라리 상위 클래스에서 추상 메소드를 정의하는 것이 낫다고 합니다.

 

 

public abstract class Piece {

    private final Team team;
    private final String name;

    public Piece(final Team team, final String initialName) {
        this.team = team;
        if (team.isBlackTeam()) {
            name = initialName.toUpperCase();
            return;
        }
        name = initialName.toLowerCase(Locale.ROOT);
    }

    protected boolean isOpponent(final Piece piece) {
        return this.team.isOppositeTeam(piece.team);
    }

    public boolean isSameTeam(final Team team) {
        return this.team.isSameTeam(team);
    }

    public String name() {
        return name;
    }

    public Team team() {
        return this.team;
    }

    public abstract boolean canMove(final Position source, final Position target,
        final Piece piece);

    public abstract boolean hasMiddlePath();

    public abstract double score();

    public abstract boolean isPawn();

    public abstract boolean isBlank();

    public abstract boolean isKing();

    @Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (o == null || getClass() != o.getClass()) {
            return false;
        }
        Piece piece = (Piece) o;
        return Objects.equals(name, piece.name);
    }

    @Override
    public int hashCode() {
        return Objects.hash(name);
    }
}

 

 

위와 같이 isPawn(), isBlank(), isKing() 추상 메소드를 정의한 다음에 하위 클래스에서 true, false를 정해주면 됩니다. 예를 들어, Blank 클래스의 경우 다음과 같을 것입니다.

 

 

public final class Blank extends Piece {

    private static final Piece BLANK = new Blank();
    private static final String INITIAL_NAME = ".";

    private Blank() {
        super(Team.NOTHING, INITIAL_NAME);
    }

    public static Piece getInstance() {
        return BLANK;
    }

    @Override
    public boolean canMove(final Position source, final Position target, final Piece piece) {
        throw new UnsupportedOperationException("비어 있는 칸입니다.");
    }

    @Override
    public boolean hasMiddlePath() {
        return false;
    }

    @Override
    public double score() {
        return 0;
    }

    @Override
    public boolean isPawn() {
        return false;
    }

    @Override
    public boolean isBlank() {
        return true;
    }

    @Override
    public boolean isKing() {
        return false;
    }
}

 

 

그런데 이것도 instanceof를 쓰는 것과 근본적으로 역할을 비슷한 것 같다는 의문은 듭니다.

 

 

 

 

네 번째 수정 사항은 객체에게 메시지를 던지라는 것입니다. 매 피드백마다 나왔던 이야기인데, 특정 객체의 상태를 가져와서 비교하는 것이 아니라, 그 객체에게 메시지를 던지라는 것이죠. 이렇게 객체지향적이지 않은 코드를 싹 수정해 주었습니다.

 

 

 

 

다섯 번째 수정 사항은 매직 넘버를 상수로 만드는 것입니다. 위 메소드는 위치 좌표 2개를 뺀 (x, y) 중 x와 y의 절댓값이 같은지 물어보는 코드입니다. result가 (x, y)에 속하는 것이고, 절댓값이 같되, 두 값이 모두 0이면 안 된다를 의미합니다.

 

이를 처음에는 의미 있는 상수로 빼려고 하였으나, isLinear() 메소드에도 매직 넘버가 존재하여 차라리 Difference 라는 객체를 생성하기로 하였습니다.

 

 

public class Difference {

    private final int horizontalDegree;
    private final int verticalDegree;

    public Difference(final int horizontalDegree, final int verticalDegree) {
        this.horizontalDegree = horizontalDegree;
        this.verticalDegree = verticalDegree;
    }

    public Difference(final List<Integer> positionDifference) {
        horizontalDegree = positionDifference.get(0);
        verticalDegree = positionDifference.get(1);
    }

    public boolean isSameAbsoluteValue() {
        return Math.abs(horizontalDegree) == Math.abs(verticalDegree);
    }

    public boolean hasZeroValue() {
        return horizontalDegree == 0 || verticalDegree == 0;
    }

    public boolean allZeroValue() {
        return horizontalDegree == 0 && verticalDegree == 0;
    }

    public int biggerAbsoluteValue() {
        return Math.max(Math.abs(horizontalDegree), Math.abs(verticalDegree));
    }

    public Difference makeUnitLength() {
        final int absoluteValue = biggerAbsoluteValue();
        return new Difference(horizontalDegree / biggerAbsoluteValue(), verticalDegree / biggerAbsoluteValue());
    }

    public int horizontalDegree() {
        return horizontalDegree;
    }

    public int verticalDegree() {
        return verticalDegree;
    }
}

 

 

그래서 위와 같이 매직 넘버를 빼기 보다는 Position 객체의 책임을 위임하였습니다. 해당 객체는 다음과 같이 사용합니다.

 

 

    private boolean isLinear(final Position target) {
        final List<Integer> result = target.subtract(this);
        final Difference difference = new Difference(result);
        return difference.hasZeroValue() && !difference.allZeroValue();
    }

    private boolean isDiagonal(final Position target) {
        final List<Integer> result = target.subtract(this);
        final Difference difference = new Difference(result);
        return difference.isSameAbsoluteValue() && !difference.hasZeroValue();
    }

    public Direction decideDirection(final Position target) {
        if (hasMiddlePath(target)) {
            final Difference difference = directionMatcher(target);
            return Direction.matchedDirection(difference.horizontalDegree(), difference.verticalDegree());
        }
        throw new IllegalArgumentException("유효하지 않은 방향입니다.");
    }

    private Difference directionMatcher(final Position target) {
        final List<Integer> result = target.subtract(this);
        final Difference difference = new Difference(result);
        return difference.makeUnitLength();
    }

 

 

어떤가요, 훨씬 더 직관적이지 않나요? 이것도 리뷰어님이 어떻게 생각하실지는 모르겠으나, 저는 나름 만족하였습니다.

 

 

 

 

여섯 번째 수정 사항은 메소드의 이름을 잘 지으라는 것입니다. 해당 메소드가 calculateScore에서 온 것이므로 get보다는 calculate 단어를 사용하여 수정하였습니다. 이 외에도 다른 메소드를 엘레강트 오브젝트 시간에 배운 방식으로 수정해 보았습니다. 모든 메소드명을 그렇게 지은 것은 아니고, 'void' 반환형은 동사, 그 외 반환형은 명사로, boolean 반환형은 형용사로 지었습니다. 그래서 우리가 흔히 말하는 'getName'과 같은 메소드명은 단순히 'name'이 되는 것입니다.

 

 

 

 

일곱 번째 수정 사항은 불필요한 상속을 막으라는 것입니다. 개인적으로는 우선 클래스에는 final을 다 붙여 놓고 상속이 필요한 경우에는 final을 떼거나 추상 클래스로 만드는 것이 바람직할 것 같습니다. 크루들에게는 미안하게도 새로운 강박증이 생겨버렸습니다 ㅎㅎ

 

 

정리

로직 상의 문제는 없었지만, 김고래 리뷰어님의 도움을 받아서 더욱 가독성이 좋은 코드가 되었다고 생각합니다. 2차 피드백이 오는대로 바로 공유하도록 하겠습니다.