[우아한테크코스] 1주차 과제 피드백 후기

우아한테크코스 1주차 피드백

매주 1대1로 피드백을 받을 수 있는 너무나 좋은 기회가 있다.

이번 분기에 배정된 리뷰어는 자바지기님 이다. 자바지기님의 리뷰를 1:1로 받는 다는 것은 테니스로 따지면 페더러와 공을 쳐볼 수 있는 기회라고 생각하면 된다. 페더러와 공을 치고 포핸드를 교정받는 느낌이랄까.

1주차 과제에 대한 피드백 내용이다.

리뷰는 깃허브를 통해 다음과 같이 이루어진다.

메서드 반환형에 대해

기존 나의 코드

public List<CarDto> getWinners() {
    gameResult = new GameResult(cars);
    return RacingCarUtil.convertCarToCarDto(gameResult.getWinnerCars());
}

Game이라는 클래스에서 GameResult라는 게임의 결과를 반환하는 메서드이다. Game클래스안에 GameResult란 인스턴스 변수까지 가지고 있던 상황이었다.

굳이 인스턴스 변수로 구현하지 않고 이 메소드의 반환 값으로 GameResult를 반환하는 것은 어떨까?

피드백 이후

public GameResult getGameResult() {
    return new GameResult(cars.getCarList());
}

반환형을 GameResult로 수정하니 클래스의 인스턴스 변수가 하나 줄었다. 대단한 수확이다. 되짚어보면 GameResult 인스턴스변수는 저 메서드에서만 쓰이는 필드였다. 다른 메서드에서 쓰이지 않는 필드가 있다는 것은 클래스의 응집도가 낮아지는 것이다. 저 필드를 없앰으로써 클래스의 응집도를 높일 수 있었다.

경계값에 대해

기존 나의 코드

public class Game {
    private static final int STOP_BOUND = 3;
    private static final int MOVE_BOUND = 4;

    ...
}

3 이하일 경우 움직이지 않는 기능을 구현했다. 사실 경계값 하나만 있어도 되는데 4라는 상수까지 구현해놓은 상태 였다.

피드백 이후

STOP_BOUND를 삭제했다. 삭제하고 나니 메서드 로직도 한결 간결해졌다.

public void attemptMove(int number) {
    if (number >= MOVE_BOUND) {
        moveForward();
    }
}

클래스 필드의 의미

지정한 횟수만큼 game을 play하는 로직이 필요했다.

기존 나의 코드

// 게임 진행
Game game = new Game(numberGenerator, cars);
for (int i = 0; i < tries; i++) {
    userInterface.printResult(game.play());
}

tries를 받고 메인 함수에서 반복을 돌려주었다.

tries를 game에 전달하고 game.isEnd()와 같은 메소드 추가해 구현하는 것은 어떨까?

라는 피드백을 받고 Game 클래스에 대해 생각했다. Game이라는 개념은 게임 시도 횟수라는 것을 가지고 있어야 그 역할을 다하는 것 같았다.

피드백 이후

// 게임 진행
Game game = new Game(numberGenerator, cars, tries);
while (!game.isEnd()) {
    userInterface.printResult(game.play());
}

결국 tries를 Game에 넘겨주게 되었고 game 객체에 게임이 끝났는지 메세지를 보내는 형식으로 진행하였다.

도메인의 값 검증

도메인 클래스의 값 검증을 사용자 콘솔에서 입력하였다.

기존 나의 코드

public Car(String name) {
    this(name, 0);
}

public Car(String name, int position) {
    this.name = name;
    this.position = position;
}

피드백은 받은 내용은 도메인 값이 유효한지 이 곳에서 검증하는 것이 어떻겠냐는 것이었다. 고민한 결과 View쪽에서 검사하고 넘겨주는 방식의 문제점은 사용자 입력이 아닌 단순 임의값으로 값을 초기화한다면 잘못된 값으로 객체를 초기화할 가능성이 있다고 생각되었다.

피드백 이후

public Car(String name, int position) {
    isProperName(name);
    this.name = name;
    this.position = position;
}

private void isProperName(String name) {
    // 빈문자열, 5글자, 이름에 공백
    validateEmptyName(name);
    validateNameLength(name);
    validateNameContainsBlank(name);
}

private void validateEmptyName(String name) {
    if (name.length() == 0) {
        throw new IllegalArgumentException("비어 있는 이름을 입력할 수 없습니다");
    }
}

private void validateNameLength(String name) {
    if (name.length() > MAX_NAME_LENGTH) {
        throw new IllegalArgumentException("이름은 5글자를 넘을 수 없습니다");
    }
}

private void validateNameContainsBlank(String name) {
    if (name.contains(BLANK_SPACE)) {
        throw new IllegalArgumentException("이름은 공백을 포함할 수 없습니다");
    }
}

도메인 생성자에서 검증하는 부분을 추가하였다. 여기서 시간이 매우 오래걸렸는데 전체적인 게임의 구조를 바꿔야 했기 때문이다. 뷰에서 검증하는 부분을 모조리 삭제하고 도메인 중심으로 돌아가게 했다. 테스트도 싹 갈았다.

Collection의 활용

테스트 메서드 안에서 테스트 객체들을 생성하는 부분에서 피드백이 있었다.

기존 나의 코드

List<Car> cars = new ArrayList<>();
cars.add(new Car("pobi", 3));
cars.add(new Car("jay", 3));
cars.add(new Car("crong", 2));

리스트를 만들고 add하는 부분인데 코드도 너무 길고 지저분했다.

Arrays.asList()를 활용해 구현해 본다.

피드백 이후

List<Car> cars = Arrays.asList(new Car("pobi", 3), new Car("jay", 3), new Car("crong", 2));

코드가 훨씬 짧아졌다.

List Assertion

테스트 메서드에서 코드를 확 줄일수 있는 부분을 짚어주셨다.

기존 나의 코드

@Test
void 정상적인_우승자_자동차_반환() {
    // given
    List<Car> cars = new ArrayList<>();
    cars.add(new Car("pobi", 3));
    cars.add(new Car("jay", 3));
    cars.add(new Car("crong", 2));

    List<Car> expectedWinnerCars = new ArrayList<>();
    expectedWinnerCars.add(new Car("pobi", 3));
    expectedWinnerCars.add(new Car("jay", 3));

    // when
    GameResult gameResult = new GameResult(cars);

    // then
 assertThat(gameResult.getWinnerCars()).isEqualTo(expectedWinnerCars);
}

정상적인 우승자를 반환하는 테스트 코드인데 내가 기대하는 값을 expectedWinnerCars라는 객체를 만들어서 테스트를 하였다. 리스트 비교를 하려고 isEqualTo()를 사용했었다.

containsExactly를 이용해 구현해 본다.

피드백 이후

@Test
void 정상적인_우승자_자동차_반환() {
    // given
    List<Car> cars = Arrays.asList(new Car("pobi", 3),
                                   new Car("jay", 3),
                                   new Car("crong", 2));

    Car winnerPobi = new Car("pobi", 3);
    Car winnerJay = new Car("jay", 3);

    // when
    GameResult gameResult = new GameResult(cars);

    // then
    assertThat(gameResult.getWinnerCars()).containsExactly(winnerPobi, winnerJay);
}

contains를 써도 되지만 assertThat에 conatins계열을 쓴다면 코드가 훨씬 짧아지고 가독성이 높아진 것을 알 수 있었다.

느낀 점

어떤 부분은 매우 단순하지만 오래걸렸던 부분들도 있었다. 그렇게 혼자 엄청 오래 고민했던 부분이 질문이나 피드백을 통해 단번에 해결되는 것을 보고 생각이 폭들이 조금 씩 넓어지는 것 같았다.