[우아한테크코스] 프리코스 2주차 코드 리뷰 후기

[우아한테크코스] 프리코스 2주차 회고

 

 

코드 리뷰 피드백 요약

공백 라인을 의미있게 사용하자

이건 1주차 공통 피드백으로 나왔던 내용인데, 공백 라인을 의미있게 사용하는 것이 좋으며, 문맥을 분리하는 부분에 사용하는 것이 좋다. 과도한 공백은 다른 개발자에게 의문을 줄 수 있다고 했다.

 

이걸 읽었을 때는 그렇게 신경쓰지 않고 넘어갔던 부분이었다. 그런데 딱 이 부분에서 피드백을 받았다.

public void run() {
    List<String> carNames = setCarNames();
    int totalRounds = setTotalRounds();
    Console.close();
    RacingGame racingGame = new RacingGame(carNames);
    playRounds(racingGame, totalRounds);
    printWinners(racingGame);
}

 

`Console.close();`를 기준으로 위는 입력을 받아서 초기화하는 문맥이고, 아래는 게임 진행과 생성에 대한 문맥으로 그러면 문맥 분리를 위해서 공백 라인 한 줄이 추가되면 더 쉽게 읽을 수 있을 것 같다고 했다.

 

대수롭지 않게 넘어갔던 부분인데 이 피드백을 통해 좀 더 확실히 이해할 수 있었다.

 

접근 제어자 설정

접근 제어자가 `default`로 설정되어 있는데, 패키지 외부에서 사용될 가능성이 없다면 `private`로 설정하는 것이 더 좋다.

 

같은 패키지에 해당 메서드명과 같은 메서드가 없기 때문에 `default`로 냅두었던 것인데 그냥 외부에서 쓰지 않는다면 `private`로 하는 것이 맞아 보인다.

 

매직 넘버의 상수화

`Car` 클래스에서 이동 조건을 검사하는 숫자 4는 매직 넘버로 사용하기보다는 상수로 정의하여 `MOVE_THRESHOLD`와 같이 의미를 부여하는 것이 가독성에 유리하다는 피드백을 받았다

 

2주차 회고에 있지만 나는 자주 쓰지 않는다면 상수로 만들 필요가 없다고 생각했다. 하지만... 이번에 나온 2주차 전체 피드백에서 값을 하드코딩하지 않는다고 해서 이제는 상수로 만들 예정이다.

 

 

메서드 통합으로 중복 제거

`getMaxPosition`도 우승자를 가리기 위함이므로 `getMaxPosition`을  `getWinners`에서 호출하는 방식으로 구현되도 좋을 것 같다.

public List<String> getWinners() {
    int maxPosition = getMaxPosition();
    return cars.stream()
            .filter(car -> car.getPosition() == maxPosition)
            .map(Car::getName)
            .toList();
}

 

맞는 말이라고 생각한다. `getMaxPosition`바로 뒤에 `getWinners`을 호출했는데 이렇게 작성하는 게 더 효율적으로 관리할 수 있을 것 같다.

 

 

Stream API 활용

`toList()`가 Java 16부터 `Stream` 인터페이스에 추가되었으므로, `Collectors.toList()` 대신 `toList()`를 사용할 수 있다.

 

자바에 대해서 더 공부해야겠다는 생각이 들었다... 

 

 

객체의 비교 책임 위치

객체끼리의 비교도 `Car`에서 할 수 있지 않을까? 객체의 책임에 대해 고민하고 compareTo 사용도 고려해보면 좋을 것 같다.

 

이 피드백을 보자마자 어떻게 `Car`에서 비교할 수 있다는 건지 의문이 들었다. 그래서 챗지피티한테 물어봤는데 `Car` 클래스에서 `Comparable<Car>` 인터페이스를 구현한다고 되어있었다. 그리고 `compareTo` 메서드를 추가하여 `position`을 기준으로 비교하게 하면 된다고 했다. 

 

이 부분에 대해서는 좀  찾아봐야겠다.

 

 

변수에 `final` 키워드 사용

`final ` 키워드는 많은 이점을 준다. 전체적으로 변수에 `final`을 붙이는 것을 습관하하는 것이 좋다.

 

`final`이라고 하면 대충 불변성을 확보한다라고만 이해를 하고 있는데 어떠한 이점이 있는지 좀 더 찾아보아야겠다.

 

 

에러 메시지의 Enum화

예외 메시지를 `ExceptionMessage` 클래스로 관리하는 대신 Enum으로 작성하면, 메시지 외에도 에러 코드 등 다양한 상태를 적용할 수 있다.

 

지금은 에러 코드가 필요없어서 클래스에 단순한 문자열 상수로 담아두었는데 나중에 에러 코드가 필요할 경우, 아래처럼 Enum으로 표현하는 것이 좋을 것 같다는 생각이 들었다.

public enum ExceptionMessage {
    INVALID_CAR_NAME("자동차 이름을 올바르게 입력해주세요. 공백이나 빈 문자열은 허용하지 않습니다.", 1001),
    CAR_NAME_TOO_LONG("자동차 이름은 5자 이하만 가능합니다.", 1002),
    DUPLICATE_CAR_NAME_ERROR("자동차 이름은 중복될 수 없습니다.", 1003),

    private final String message;
    private final int errorCode;

    ExceptionMessage(String message, int errorCode) {
        this.message = message;
        this.errorCode = errorCode;
    }

    public String getMessage() {
        return message;
    }

    public int getErrorCode() {
        return errorCode;
    }
}

 

 

`List` 요소가 하나일 때 `String.join` 활용

List<String> `winners`에 요소가 하나만 있을 때 `String.join(", ", winners)`를 호출해도 `,`가 붙지 않은 요소 하나만 반환된다.

 

이 내용을 몰랐어서 나는 우승자가 1명일 경우와 2명 이상일 경우로 나누어 로직을 짰었다

 

 

`moveCount` 위치

`moveCount`가 자동차 경주와 관련있는 상태라고 생각해서 `RacingGame` 클래스 필드에 넣었다.

 

이게 더 추상화 측면에서 더 자연스러운 것 같다. 왜 코드를 짤 때는 이 생각을 못했었지...

 

 

`static import`와 가독성

메서드를 `static import`하면 프로젝트 규모가 커졌을 때, 함수의 출처를 파악하기 어려워질 수 있어 클래스 이름을 명시하는 것이 좋다. (ex. `TotalRoundsInputProcessor.parseTotalRounds(totalRounds)`)

 

이 부분은 나도 고민한 부분이다. 메서드명만 적으면 해당 클래스에 존재하는 메서드와 헷갈릴 수도 있으니 차라리 앞에 클래스명을 붙이는 게 더 낫지 않을까 싶었다. 이번 과제에서는 좀 섞어서 사용했는데 앞으로는 클래스명을 붙여야겠다

---

다른 사람들의 코드에 대한 후기

팩토리 패턴을 활용해 `RacingController`뿐 만 아니라 `RacingControllerFactory`를 만들어서 여기에 생성자 주입 및 객체 생성 역할을 만든 사람도 있었다.

 

나랑 비슷하게 코드를 짠 사람이 있었는데 `InpitView` 메서드를 `static`으로 선언해 같이 넘겨주는 부분이었다. 근데 어떤 분이 코드 리뷰로 `InputView`를 생성자를 통해 주입받음으로써, 클래스 간의 결합도를 낮추고 유연성을 높일 수 있다고 남겼다.

 

정처기를 공부하면서 결합도에 공부했지만, 코드를 짤 때 생각하기에는 좀 어려운 것 같다... 

Winner는 유틸 성격이 많이 띄어서 유틸 클래스 즉, static 메서드를 활용하는건 어떨까요? 

`InpitView`에서는 생성자를 직접 주입받는 게 나아서 `static`을 활용하지 않는 게 더 좋다는 말이 있고, 유틸 성격이 많이 띄우면 `static`을 활용하는 게 어떠냐는 말도 있었다. `static`은 자바 강의를 들을 때, 짧게 지나갔던 부분인데 이에 대해서 확실히 공부해봐야겠다.

 

 

아래 코드처럼 메서드 안에 메서드가 있는 부분이 있었다.

outputView.printRacingResult(racingService.onceRacing());

근데 이렇게 하는 것보다는 변수로 빼주고, 그 변수를 메서드 안에 넣는 게 더 가독성이 좋다고 하였다.

 

 

`Car`의 `name`을 객체로 한 번 포장하고, name과 관련된 책임을 그 객채에서 담당하는 사람도 있었다. 이런 코드를 볼 때마다 고민하게 된다. 나는 지금은 간단한 프로젝트이기 때문에, 이렇게 `name` 클래스로 분리하는 것은 오히려 과도한 추상화라고 생각했고, 이름을 검증하는 것이 그렇게 복잡하지 않기 때문에 `Car` 클래스 하나로만 하는게 적당하다고 생각했다.

 

이렇게 다른 사람들의 코드를 보면 몇몇 분들은 객체를 좀 많이 나누는 것이 보이는데 참 고민된다. 어디까지 객체지향을 위해 클래스로 분리하는게 적절한지... 클래스 간의 의존성에 대해서도 좀 더 고민해봐야할 것 같다.

 

 

사이즈 비교할 때, `set`이 아니라 `add`를 사용할 수 있는 것도 알게되었다.

private void verifyDuplicateCar(List<Car> cars) {
    Set<String> carNames = new HashSet<>();
    for (Car car : cars) {
        if (!carNames.add(car.getName())) { 
            throw new IllegalArgumentException(NOT_PERMIT_SAME_CAR.getMessage());
        }
    }
}

 

 

 

get으로 데이터를 들고 오는 거에 이렇게 코드 리뷰를 단 사람이 있었다.

public String getName() {
    return name;
}

public int getPosition() {
    return position;
}

 getName()과 getMoveDistance() 메서드가 객체의 속성을 직접 반환하는 것은 Tell, Don't Ask 원칙을 위반한다.

 

Tell, Don't Ask 원칙이라는 것을 처음 들어봤기 때문에 의아했었다. 궁금해서 찾아보니 객체에게 데이터를 요구(Ask)하지 말고, 객체에게 일을 시켜라(Tell) 정도로 해석할 수 있다고 한다. 객체 지향적 사고방식 원칙 중 하나라고 했다. 그래서 저렇게 객체의 속성 값을 직접 반환하는 것은 원칙에 어긋나는 것 같다.

 

그래서 이 분은 어떻게 코드를 짰는지 궁금해서 찾아보았더니 코드와 같이 `String`을 return해주고 `TypeOfLocation`이란 enum을 만들어서 이것을 이용해서 winner을 가리는 것을 볼 수 있었다.

public String nameWithLocationToString() {
  return name
      + NAME_AND_LOCATION_DELIMITER
      + IntStream.range(0, location).mapToObj(i -> "-").collect(Collectors.joining());
}

 

다른 사람들의 코드와 코드 리뷰를 보니 옵저버 패턴과 일급 컬렉션 패턴을 새로 학습할 필요가 있다고 느꼈다. 또한, 테스트 함수명을 한글로 작성하는 사례도 눈에 띄었고, 나와 달리 모든 테스트를 따로 파일로 나누기보다는 NsTest에서 제공하는 assertRandomNumberInRangeTest와 같은 공통 테스트 메서드를 활용하여 테스트 파일을 통합적으로 관리하는 방식도 있었다.

 

 

 마지막으로, 문자열 비교 시 소문자로 변환한 후 비교하는 방법을 사용하는 사람도 있었는데, 나는... 이럴 생각을 못했다ㅠㅠㅠ