[우아한 테크코스 3기] LEVEL 1 회고 - 자동차 경주 게임의 2, 3차 피드백을 받아보다 (7일차)

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

 

오늘은 첫 번째 미션인 자동차 경주 게임의 2, 3차 피드백을 통해 느낀 점을 말씀드리려고 합니다. 사실, 2차 피드백은 일요일날 받았지만, 제가 그날은 신나게 놀아서 오늘이 되어서야 리팩토링을 하였습니다 ㅎㅎ..

 

 

2차 피드백

 

 

첫 번째 수정 사항입니다. fuel 변수를 왜 굳이 명시적으로 0으로 초기화한 것인지 물어보셨습니다. 그리고 저는 처음 자동차의 연료 상태는 0이니까 0으로 초기화해야겠다고 생각하였습니다.

 

 

 

 

하지만, 두 번째 수정 사항을 보고 나서 생각이 바뀌었습니다. 애초에 Car 클래스의 멤버 변수로 fuel이 필요하지 않다는 것이죠. 그래서 fuel을 지우고 아래와 같이 리팩토링하였습니다.

 

 

  public void fillInFulAndRun(final int fuel) {
    if (isRunnable(fuel)) {
      this.position++;
    }
  }

  private boolean isRunnable(final int fuel) {
    return fuel >= MIN_RUNNABLE_FUEL;
  }

 

 

isRunnable()에 인자를 추가하여, 그 값만 단순히 비교하는 방식인 것이죠. 따라서, 기존의 있던 멤버 변수인 fuel을 초기화할 필요도 없어졌습니다.

 

 

 

 

세 번째 수정 사항은 name을 객체로 만드는 것이었습니다. 저는 처음에 굳이 필요한가 생각이 들었지만, name을 객체로 만들게 되면, 그 클래스 안에서 유효성 검사도 가능하고 관련된 상수도 처리할 수 있다는 장점을 깨닫게 되었습니다.

 

 

public class Name {

  private static final int MIN_NAME_LENGTH = 1;
  private static final int MAX_NAME_LENGTH = 5;
  private final String name;

  public Name(final String name) {
    validateName(name);
    this.name = name;
  }

  private void validateName(final String name) {
    if (name.length() < MIN_NAME_LENGTH || name.length() > MAX_NAME_LENGTH) {
      throw new IllegalStateException(RacingCarErrorMessage.CAR_NAME.getMessage());
    }
  }

  public String get() {
    return name;
  }
}

 

 

이런식으로 Name을 객체로 만들면, 관련 상수 처리, 유효성 검사, 문자열 반환이라는 행위를 이 안에서만 시킬 수 있게 됩니다.

 

 

public class Turn {
  private static final int MIN_TURN = 1;
  private final int turn;

  public Turn(final int turn) {
    validateTurn(turn);
    this.turn = turn;
  }

  private void validateTurn(final int turn) {
    if (turn < MIN_TURN) {
      throw new IllegalStateException(RacingCarErrorMessage.TURN.getMessage());
    }
  }

  public int get() {
    return turn;
  }
}

 

 

같은 방식으로 시도할 횟수인 turn도 객체로 바꾸었습니다. 앞으로, 원시 타입을 wrapping하는 방법을 잘 익혀두어야겠습니다.

 

 

 

 

네 번째 수정 사항은 기본 생성자를 제거하는 것입니다. 기본 생성자에서 하는 역할이 단순히 리스트를 초기화하는 것이다보니, 쓸데 없이 코드를 길게하기보다는 cars를 인스턴스 필드 단계에서 초기화하는 것이죠.

 

 

 

 

다섯 번째 수정 사항은 getter를 사용하지 않는 것입니다. 그리고 이것이 가장 리팩토링하기 어려웠습니다.

 

우선, 객체는 독립적이어야하는데 getter는 다른 객체의 상태값을 직접적으로 가져오는 것이므로 의존성을 높이는 단점을 불러일으킵니다. 따라서, getter를 쓰기보다는 다른 객체와 소통할 수 있는 메소드를 정의해야합니다.

 

그래서 34번째 라인에서 car.getPosition()과 maxPosition을 비교하는 대신에 Car 클래스 내의 isSamePosition()을 정의하는 것입니다.

 

비슷한 맥락으로, maxPosition을 구하는 과정도 getter를 쓰지 말아야하는데, 이 부분은 Car 클래스에서 Comparable를 상속받아서 position을 기준으로 내림차순하도록 설정하면 됩니다.

 

 

  public String decideWinner() {
    Collections.sort(cars);
    Car winnerCar = cars.get(0);

    List<Car> winners = cars.stream()
        .filter(car -> car.isSamePosition(winnerCar))
        .collect(Collectors.toList());
    return new Winner(winners).getWinnerName();
  }

 

 

위와 같이 cars를 정렬하면 Car 클래스의 오버라이드된 compareTo() 덕분에 position을 기준으로 내림차순이 됩니다. 이때, 첫 번째 car가 우승자가 될 것입니다. 그리고 나서 동점자까지 고려하여 winners 리스트를 생성하는 것입니다.

 

하지만, 여기서 문제가 하나 있는데, 이것은 3차 피드백에서 설명하겠습니다.

 

 

 

 

마지막 수정 사항은 맞지 않는 멤버 변수 삭제입니다. 저는 RacingResult 클래스 내의 멤버 변수로 Participants를 정의하였는데, 이 피드백을 보고 다시 생각해 보니까 아무래도 결과 클래스 안에 참가자가 있는 것은 바람직하지 않다고 판단하였습니다. 그래서 Participants를 제거하고, RacingResult 내의 메소드의 인자로 Participants를 추가하였습니다.

 

 

public class RacingResult {

  private static final String ENTER = System.lineSeparator();
  private static final String LOG_FORM = "%s : %s" + ENTER;
  private static final String DISTANCE_SIGN = "-";
  private final StringBuilder log = new StringBuilder();

  public void appendLog(final Participants participants) {
    participants.getCars()
        .forEach(car -> log.append(
            String.format(LOG_FORM, car.getName(), convertToDistanceSign(car.getPosition()))
        ));
    log.append(ENTER);
  }

  private String convertToDistanceSign(final int position) {
    StringBuilder sign = new StringBuilder();
    for (int i = 0; i < position; i++) {
      sign.append(DISTANCE_SIGN);
    }
    return sign.toString();
  }

  public String getLog() {
    return log.toString();
  }

  public String decideWinner(Participants participants) {
    return participants.decideWinner();
  }
}

 

 

다음과 같이 필요한 인자에 Partcipants를 추가함으로써 문제를 해결하였습니다.

 

 

3차 피드백

이렇게 2차 피드백까지 수정하고 난뒤, 다시 리뷰어 님께 3차 피드백을 요청하였습니다.

 

 

 

 

이렇게 격려도 해 주시고, 잘한 부분에 대해서는 칭찬도 해 주셔서 코드 리뷰를 신청할 때마다 설렜습니다 ㅎㅎ. 그리고 나머지 피드백은 저 혼자서도 수정이 가능하다고 판단하셔서 미션을 마무리하신듯 합니다.

 

 

 

 

첫 번째 수정 사항은 Null 관련 익셉션입니다. 사실, 저는 여기서 try ~ catch를 통해 익셉션을 처리할까 고민하였는데, 애초에 자동차의 이름은 1글자 이상 5글자 이하이고, 0글자는 들어올 수 없으므로 참가자는 반드시 1명 이상이 됩니다. 따라서, null 검사 없이 get()을 취해도 문제가 없다고 판단하였습니다.

 

또한, 이것을 지금 단계에서 처리한다고하더라도 프로그램을 바로 종료시켜야할지, "우승자가 없습니다."라고 에러 문구를 띄워서 중간 중간 boolean 처리를 적절히 할지 고민하는 것은 적절하지 않다고 생각하였습니다. 그래서 이 부분은 우선 이대로 두기로 하였습니다. 하지만, 다음 번에 로직을 설계할 때는 이러한 null 체크에 유의해야겠다고 다짐하였습니다.

 

 

 

 

두 번째 수정 사항은 Winner 클래스 내의 우승자의 이름을 반환하는 메소드를 OutputView로 옮기는 것입니다. 이 부분도 처음에는 getWinnerName()을 OutputView로 옮겨서 처리하려고 하였으나... 테스트 코드에서 문제가 생겼습니다.

 

 

    RacingResult racingResult = racingManager.start();
    assertThat(racingResult.decideWinner(participants)).isEqualTo(expectedLog);

 

 

여기서 decideWinner()의 반환형은 String인데, getWinnerName()을 OutputView로 옮기면 수정 과정에서 이 부분의 반환형이 Winner가 됩니다. 그렇다면, OutputView에서 getWinnerName()은 static이니까 isEqualTo()와 바로 비교하면 되지 않겠냐고 생각하실 수 있는데, 저는 OutputView가 출력하는 부분만 public이 되길 원했습니다. 그러니까 getWinnerName()은 printWinner() 위한 메소드라고 생각하여 private 처리를 하고 싶었습니다.

 

이것저것 방법을 시도해 보았는데, 원래 기존의 선언된 방식의 뾰족한 문제점을 찾지는 못해서 이 부분도 피드백을 반영하지 않았습니다. 추후에 바뀔 이유가 생기면 고치려고 합니다.

 

 

 

 

마지막 수정 사항은 옳지 않는 주석을 삭제하는 것입니다. 사실, 이것은 자동차 경주 게임의 코드는 아니고 단위 테스트 예제 학습할 때 적어둔 것인데, 제가 다른 거랑 혼용해서 쓰다가 실수로 주석을 안 지운 것 같습니다. 그래서 바로 지웠습니다 ㅎㅎ

 

 

정리

저는 프리코스 때부터 개발을 하고 나서 늘 구조가 찜찜하고, 제 코드가 항상 불만족스러웠습니다. 객체의 역할을 분리하는 것보다는 하나의 객체가 여러 개의 일을 분담하도록 설계하였고, 어떻게든 프로그램이 돌아가는 것을 1순위로 정하곤 하였습니다. 하지만, 본 코스 첫 번째 미션에서 페어프로그래밍을 하면서 단일책임원칙과 추상화 방법을 익혔고, 어느 정도 제 스스로 마음에 드는 코드가 나왔습니다. 물론, 아직도 부족하고 배워야할 것은 많지만, 프리코스 때보다 한 단계 성장한 것은 뿌듯하게 생각합니다.

 

위 미션의 코드가 궁금하신 분은 이곳을 참고하시길 바랍니다.