[코드 리뷰 정리] Level 1. 자동차 경주 게임

2021-02-15 글

 

우테코 레벨 1 난생 처음으로 코드리뷰를 받게 되었다.
사실 TDD도 처음이고, 누군가 내 코드를 읽고 피드백 받는 것 또한 처음인데
이런 기회가 정말 감사하면서도 떨렸던 첫 리뷰 요청이었다 👀

코드 리뷰를 받으니 내가 모르는 부분과 나의 실수들을 명확히 볼 수 있었다.
또한 리뷰를 보니 나의 잘못된 코딩 습관도 발견할 수 있었다.
덕분에 고민하고 공부해봐야 할 것들을 알 수 있어
정말로 나에게 너무나도 큰 도움이 되었다 🙇‍♂️

피드백 받은 부분에서 계속 머릿 속에 박기 위해
몇 가지는 정리하고 넘어가는 것이 좋을 것이라 판단이 되었다.

불필요한 객체 생성을 피하라

Pattern 객체는 비싸다

나는 지금껏 올바른 숫자인지, 문자인지 등을 검증할 때
대부분 Pattern 객체를 정말 아무렇지 않게 사용했었다.

그리고 이를 쓰는 방법은 저렇게 메서드를 호출 시 패턴 객체를 매번 생성했다.

하지만 이번에 전반적인 리뷰들을 보니 정규식을 활용해 값을 검증한 로직에는
"Pattern은 매우 비싼 객체이다." 라는 리뷰가 꼭 있었다.
부끄럽지만 프로그램을 구현하면서 성능까지 고려하지는 못했던 것 같다.

이펙티브 자바 item 6 불필요한 객체 생성을 피하라와
이 글은 페어의 리뷰에 달린 글을 나의 참고자료로 삼았다.

Pattern 인스턴스는 1번 사용되고 버려져서
곧바로 GC의 대상이 되어 비싼 비용이 든다.

이런 Pattern 객체는 한 번 생성하고 재활용 할 수 있었고
아래와 같이 메소드 호출 시 매번 생성하는 효율성 떨어지는 방법을 버리고,
static 변수로 선언해주었다.

public class StringCalculator {
    private static final Pattern PATTERN = Pattern.compile("//(.)\n(.*)");

static을 사용하는 이유 명심하기

분명히 static을 사용하는 이유에 대해서 자바 기본 개념에 정리했었는데
구현 중 무분별하게 static 변수와 메소드를 사용하였다.
static 을 사용할 때 "변수가 선언된 클래스의 모든 인스턴스가 공유하는 변수인가?"를 질문하지 않았고,
"어떠한 인스턴스에도 속하지 않는 상태로 메모리 공간에 딱 하나만 존재함"을 염두해두지 않은 문제였다.

"Cars의 List<Car> cars는 프로그램이 실행되고 바로 메모리에 저장되어
모든 인스턴스가 공유하는 값이어야하는가?"를 질문하면
저 코드를 뜯어 고쳐야 한다는 것을 단번에 알 수 있다.

또한 피드백을 받고 보니 일급 컬렉션을 본 의도대로 구현하지 못한 것 같아
주신 jojoldu님의 일급 컬렉션 글을 다시 정독해보았다.

언급한 부분은 일급 컬렉션의 개념을 도입하고, 추가적으로 정적 팩터리 메서드를 공부하면서
아래와 같이 대대적인 리팩토링을 거쳤다.

public class Cars {
    private final List<Car> cars;

    private Cars(final List<Car> cars) {
        this.cars = new ArrayList<>(cars);
    }

    public static Cars makeFromCarNames(final List<Name> carNames) {
        List<Car> cars = carNames.stream()
                .map(Car::new)
                .collect(Collectors.toList());
        validateNonDuplicatedNames(cars);
        return new Cars(cars);
    }

내가 "static을 사용하는 이유 명심하기"를 "불필요한 객체 생성을 피하라"의 하위에 넣은 이유는 static 메서드나, static 상수만 갖고 있는 클래스는 굳이 새로 인스턴스로 생성할 필요가 없다.
즉static은 상태 값을 가지지 않아 인스턴스를 생성할 필요가 없다.
때문에 생성자를 private 접근자로 막아 불필요한 객체가 생성되는 것을 막아야 한다.

➕
리뷰어님께서 추가적으로 static과 일급 컬렉션에 참고할만한 너무나도 유익한 글을 주셨다.
(추가로 깨달은 것을 짤막하게 남기자면
static은 다형성을 위반하니 객체지향에서도 멀어지게 된다)
리뷰어님의 모든 코멘트가 내게 피가되고 살이된다 😭🙇‍♂️

Optional의 올바른 사용

주신 글을 읽어보고 반환값이 ‘없음’을 나타내는 것이 주목적인
Optional의 의도에 맞지 않은 get() 메소드를 사용한 문제점을 발견했다.
스스로도 Optional에 대해 정리 했었는데 그냥 단순히 개념을 정리하기에 급급했나 싶기도 했다.

말씀해주신 대로 Car에 Comparable 인터페이스를 구현하여
compareTo()를 오버라이드 하여 map()에서 최댓값을 구할 수 있도록 하였다.

public class Car implements Comparable<Car> {
    private static final String REGEX_ALPHA = "^[a-zA-z]*$";
    private static final String REGEX_KOREAN = "[가-힣]*$";
    private static final int NAME_LENGTH_LIMIT = 5;
    private static final int MOVE_PIVOT = 4;

    private String name;
    private int position;

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

    @Override
    public int compareTo(final Car anotherCar) {
        return Integer.compare(this.getPosition(), anotherCar.getPosition());
    }

수정한 메소드

public Car getMaxPositionCar() {
        return cars.stream()
                .max(Car::compareTo)
                .orElseThrow(IllegalStateException::new);
    }

객체지향에서 get을 지양하자

이 부분은 리뷰어님과 DM으로 나눈 이야기에서 명확히 깨닫게 된 부분인데,
get 메소드를 사용하는 순간 객체가 어떤 상태를 가지고 있는지 외부에 여실히 드러나게 된다.
데이터가 private 이어도 public get을 사용하면 외부에 알려지는 것이 된다.
위 피드백 코드에서도 getPosition()이 position이라는 데이터를 가지고 있음이 드러나니 자동차의 상태가 maxPosition 인지를 car 객체에서 확인하도록 하였다.

public boolean isMaxPosition(final Car maxPositionCar) {
        return this.position == maxPositionCar.getPosition();
}

또 데일리 미팅 동기들과 이야기를 하다 깨닫게 된 것은
나는 지금 tryCount라는 객체에서 값을 꺼내 for 문을 돌렸는데,
이는 while을 사용하여 tryCount에서 그 상태를 판별하도록 변경하였다.
수정 전

private void raceByTryCount() {
    for (int i = 0; i < tryCount.getCount(); i++) {
        race();
        RacingCarView.printProgressResult(cars.cars());
    }
}

수정 후

private void raceByTryCount() {
    while (tryCount.isRemainCount()) {
        cars.race();
        RacingCarView.printProgressResult(cars.cars());
        tryCount.deductCount();
    }
}

HashSet으로 Wrapper class 중복 제거

Car 클래스에 equals와 hashCode를 이미 Override 해놓았는데
웃기게도 Name을 꺼내와서 HashSet으로 중복을 제거했다.
(말그대로 이름의 중복 제거를 한거다 ㅎ)
HashSet은 내부적으로 먼저 해당 객체의 hashCode()와 equals()를 실행해본다.
관련 내용은 내가 이전에 썼던 글이 있길래 그 부분에 추가적으로 더 작성했다.

이번 피드백으로 느낀 점

• 프로그램의 성능을 고려하는 개발자가 되자
• 어떤 개념을 사용할 때 그 의미와 목적을 다시 한번 되새기자
• 레퍼런스를 찾아 글을 읽을 때 내가 얻고자 하는 해결 방법만 뚝딱 가져가는게 아닌,
  그 내용을 완벽히 체화하자
• 이 전에 적은 공부 글들을 다시 보면서 그 부족함을 채워나가는 것도 좋은 방법인 것 같다.