[레벨 1 장기] 안 좋은 코드습관 고치기

이번 장기 미션에서 받은 리뷰 중에서 객체지향 설계와 관련된 부분보다는, 코드와 관련된 부분들이 생각보다 많았다. 이전에 알고는 있었지만 실수로 적은 것들도 있었고, 아예 모르고 있었던 것들도 있었다. 이번 기회에 확실히 정리해서 다시는 같은 실수를 반복하지 않도록 하자.

 

1️⃣ while문의 탈출 조건을 생각해라

---

아무 생각 없이 `while(true)`를 사용하는 실수를 저질렀다. 

while (true) {
    moveProcess(game);
}

 

 

물론 지금처럼 단순한 콘솔 프로그램이라면 크게 상관없겠지만, 한번의 실패가 큰 피해로 이어지는 '미션 크리티컬 시스템'이라면 큰 문제가 된다.

 

따라서 아래와 같이 탈출 조건을 while문 안에 명시를 해줬다.

while (!game.isOver()) {
    moveProcess(game);
}

이렇게 하면 코드를 읽는 사람 입장에서도 언제 끝나는지 찾기 쉬워져 코드를 유지보수하기도 좋아진다.

 

여기서 추가적으로 loopCount를 받아서 특정 횟수가 넘으면 break를 하는 코드를 작성한다면 더욱 안전한 코드 작성이 가능해진다.

int loopCount = 0;

while (!game.isOver() && loopCount < 10000) {
    moveProcess(game);
    loopCount++;
}

 

 

2️⃣ 예외를 흐름 제어 용도로 사용하지 말자.

---

`findPositionsByDirection()`메서드는 장기판의 특정 위치좌표에서 특정 방향에 있는 모든 위치좌표를 찾는 메서드이다.

`cur.add()`에서 장기판 좌표를 벗아나면 예외를 발생하게 했고 `findPositionsByDirection()`에서 그 예외가 터질때까지 반복하도록 했다.

public List<Position> findPositionsByDirection(Direction dir) {
    List<Position> positions = new ArrayList<>();
    Position cur = this;
    while (true) {
        try {
            cur = cur.add(dir.row(), dir.column());
            positions.add(cur);
        } catch (IllegalArgumentException e) {
            break;
        }
    }
    return positions;
}

 

이런식으로 특정 메서드에서 발생하는 예외를 다른 메서드에서 잡아 흐름을 제어하는 것은 좋지 않다는 피드백을 들었다.

 

좋지 않은 이유를 정리하면 다음과 같다.

 

1. 성능 문제

예외는 생각보다 엄청 무거운 작업이다.

stack trace를 생성하고 call stack을 탐색하고 객체를 생성하는 과정이 들어가기 때문에 단순히 분기문을 쓸 때보다 훨씬 무겁다

 

2. 버그가 숨겨진다.

리뷰어 분께서 말씀하셨던 것처럼 `IllegalArgumentException` 같은 예외들은 내가 작성한 상황외에 다른 상황에서도 터질 가능성이 높은 예외다. 다른 이유로 예외가 터졌는데 catch로 잡아버리면서 정상흐름으로 처리된다면 그 버그는 숨겨지게 된다.

 

3. 예외 정책이 바뀌면 코드가 깨진다.

현재는 `IllegalArgumentException`을 사용하고 있지만 추후 예외 정책이 바껴서 `DomainException` 같은 커스텀 예외 클래스를 사용하도록 변경된다면 해당 부분도 다시 `DomainException`로 수정해야 하는데 이것을 깜빡해버린다면, 그대로 버그로 이어지게된다. 

 

 

이 문제를 해결하기 위해 특정 행과 열이 장기판에서 벗어나는지 여부를 반환하는 `isOffsetWithinBounds()` 메서드를 만들었고, 

while문 안에 해당 메서드를 넣어주었다.

public List<Position> findAllPositionsByDirection(Direction dir) {
    List<Position> positions = new ArrayList<>();
    Position cur = this;
    while (cur.isOffsetWithinBounds(dir.row(), dir.column())) {
        cur = cur.add(dir.row(), dir.column());
        positions.add(cur);
    }
    return positions;
}

private boolean isOffsetWithinBounds(int row, int column) {
    return this.row.isOffsetWithinBounds(row) &&
            this.column.isOffsetWithinBounds(column);
}

 

3️⃣ enum 순서를 로직으로 사용할 때, 테스트로 순서를 보장해주자

---

현재 `Dynasty` enum 클래스의 `next()` 메서드에서는 enum의 순서를 기반으로 순환하도록 구현했다.

public enum Dynasty {

    CHO(Direction.SOUTH, false),
    HAN(Direction.NORTH, true);

    // 순서를 활용하는 메서드
    public Dynasty next() {
        Dynasty[] values = Dynasty.values();
        return values[(ordinal() + 1) % values.length];
    }
    
}

 

사실 이렇게 코드를 작성하면서도 '나중에 새로운 타입이 추가되거나, 순서가 실수로 바뀌게 되면 어떡하지?' 라는 고민이 있었다.

찜찜했지만 그냥 넘어갔었는데 리뷰어 분께서 테스트를 통해 순서를 보장하면 된다는 이야기를 해주셨다.

사실 어찌보면 당연한 부분인데 착각하고 있었던 것 같다.

 

다음과 같이 테스트 코드를 작성해주었다.

@ParameterizedTest
@CsvSource(value = {
        "CHO, HAN",
        "HAN, CHO",
})
@DisplayName("다음 Dynasty를 반환한다.")
public void next_success(Dynasty before, Dynasty after) {

    // when
    Dynasty result = before.next();

    // then
    assertThat(result).isEqualTo(after);
}

 

이런 테스트 코드가 있으면 실수로 순서가 변경되거나 타입이 추가되더라도 결국 이 테스트 코드가 깨지기 때문에 배포 전에 버그를 방지할 수 있다.

 

 

4️⃣ 의미없는 주석을 사용하지 마라

---

나한테 코드를 작성하는데 있어 고질적인 습관이 있다면, 바로 주석을 많이 적는다는 것이다. 

// 장기판 상차림 입력
Map<Dynasty, HorseElephantPosition> horseElephantPositions = readDynastyHorseElephantPositionMap();

 

하지만 클린코드에서도 그랬듯이, 좋은 주석은 “무엇을”을 작성하는 것이 아니라 “왜”를 작성하는 것이다. 즉, 주석은 ‘코드로는 설명이 어려운 부분들’을 적어야 한다.

// ex. 260328 기준 해당 메서드는 Deprecated 되었지만 A 사용처에서는 하위호환 유지를 위해 남겨둠
Map<Dynasty, HorseElephantPosition> horseElephantPositions = readDynastyHorseElephantPositionMap();

 

그전까지는 그냥 ‘한글로 적어두면 읽기 편할 것 같은데?’라고 생각했었는데 이것이 오히려 코드의 가독성을 낮추고 나중에 코드를 수정할 때, 주석까지 수정해야 한다는 것을 깨달았다.

 

 

5️⃣ 에러 메시지를 공통 클래스로 빼지마라

---

장기 프로그램에서 발생하는 예외들의 에러 메시지들을 하나의 enum 클래스에서 관리했었다.

public enum ErrorMessage {

    INVALID_HORSE_ELEPHANT_POSITION_INPUT_FORMAT("상차림 법을 숫자로 입력해주세요."),
    INVALID_HORSE_ELEPHANT_POSITION_INPUT_RANGE("1, 2, 3, 4 중 하나의 숫자를 입력해주세요."),
    INVALID_POSITION_FORMAT("위치를 콤마로 구분된 두 개의 숫자로 올바르게 입력해주세요."),
    NO_AVAILABLE_MOVES("선택된 기물이 이동할 수 있는 위치가 없습니다."),
    PIECE_NOT_FOUND("해당 위치에 기물이 존재하지 않습니다."),
    INVALID_PIECE_OWNER("본인 팀의 기물만 옮길 수 있습니다."),
    INVALID_PIECE_MOVE("해당 위치에 해당 기물을 옮길 수 없습니다.")
    ;
}

 

이 부분에 대해 다음과 같은 리뷰를 받았다.

 

사실 이전 블랙잭 미션에서도 상수를 공통 enum클래스로 빼두었다가 피드백을 받았던 적이 있었는데 이번에도 그와 비슷한 피드백이었던 것 같다. 

 

오류 메시지를 오류가 발생시키는 곳으로 이동시켰다.

public class Board {

    private final Map<Position, Piece> board;

    public static final String PIECE_NOT_FOUND_MESSAGE = "해당 위치에 기물이 존재하지 않습니다.";
    public static final String INVALID_PIECE_OWNER_MESSAGE = "해당 위치에 기물이 존재하지 않습니다.";
    public static final String INVALID_PIECE_MOVE_MESSAGE = "해당 위치에 해당 기물을 옮길 수 없습니다.";
    
    ...
}

 

사실 이렇게 하니 클래스에 보일러 플레이트 코드가 늘어나 클래스 코드가 지저분해지는 느낌이 들었다. 

이를 피하기 위해 도메인 별로 메시지를 묶어서 관리하는식으로 구현했다.

public enum BoardError {
    PIECE_NOT_FOUND("해당 위치에 기물이 존재하지 않습니다."),
    INVALID_PIECE_OWNER("해당 기물의 소유자가 올바르지 않습니다."),
    INVALID_PIECE_MOVE("해당 위치로 기물을 이동할 수 없습니다.");

    private final String message;

    BoardError(String message) {
        this.message = message;
    }

    public String message() {
        return message;
    }
}

 

 

6️⃣ 에러 메시지에 입력값을 넣으면 좋다.

---

이것보다는

throw new IllegalArgumentException(String.format("행은 %d부터 %d사이의 숫자입니다.", MIN_ROW, MAX_ROW));

 

이것처럼 입력값도 에러 메시지에 넣으면  디버깅 속도가 빨라져 버그를 빠르게 대응할 수 있게 된다.

throw new IllegalArgumentException(String.format("행은 %d부터 %d사이의 숫자입니다. 입력 값 : %d", MIN_ROW, MAX_ROW, row));

 

 

7️⃣ 시스템 내부 에러메시지가 view로 넘어가게 하지마라

---

현재는 모든 Exception에 대한 에러 메시지가 view로 넘어가게끔 되어 있다.

private <T> T getUntilValid(Supplier<T> readOperation) {
    while (true) {
        try {
            return readOperation.get();
        } catch (Exception e) {
            outputView.printErrorMessage(e.getMessage());
        }
    }
}

이렇게되면, 시스템에서 노출되면 안되는 정보가 사용자한테 노출되는 보안이슈가 발생할 수 있다.

 

이를 해결하기위해 커스텀 예외인 `DomainException`과 `ViewException`을 정의하고 해당 에러의 메시지만 사용자한테 노출하도록 하였다.

private <T> T getUntilValid(Supplier<T> readOperation) {
    while (true) {
        try {
            return readOperation.get();
        } catch (DomainException | ViewException e) {
            outputView.printErrorMessage(e.getMessage());
        } catch (Exception e) {
            outputView.printErrorMessage("알 수 없는 에러가 발생했습니다.");
        }
    }
}