NEXTSTEP TDD, 클린 코드 with Java 피드백 정리 및 후기

후기

작년에 쎼트렉아이에서 인턴을 하며 자바에 대해 더 깊게 공부하고 좋은 코드를 작성할 수 있는 능력을 갖추고 싶다는 생각을 많이 했었다. 학습 방법을 찾다가 넥스트스텝의 'TDD, 클린 코드 with Java' 과정이 좋다는 이야기를 많이 들어 신청했고 약 4달간 수강했다.

미션을 주고 PR로 제출하면 코드 리뷰 방식으로 피드백을 받고 계속해서 코드를 개선해나가는 과정이기 때문에 짧은 시간이 많은 것들을 배웠다고 느꼈다. PR 당 평균 15 ~ 25 회 정도 코멘트를 주고 받으면서 코드를 사이에 두고 토론하는 경험이 신기했고 재밌었다. 이런 코드 리뷰 문화를 가진 회사에 입사하고 싶다는 생각도 하게되었다.

이전까지는 코드를 작성할 때 이게 정답이 아니면 어떡하지? 하는 막막함때문에 코드를 작성하는데 시간이 오래걸리고 자신감도 없었는데 이 과정을 통해 내가 작성한 코드에 대해 객관적인 피드백을 받아보며 이런 부분을 많이 채울 수 있었다. 가장 큰 교훈을 말해보라고 한다면 개발을 할 때 정답은 없지만 대체로 효율적인 방법이 존재하고, 어떤 결정을 해도 괜찮지만 내가 그 결정을 한 것에 대해 납득 가능하고 타당한 이유를 댈 수 있어야 한다는 걸 배운 것이다. 이런 생각으로 개발을 하니 자신감이 생기고 기존에 수동적으로 남들이 작성한 코드를 찾아보기보다 오히려 원리를 이해하고 왜 그런지 질문하면서 적극적으로 개발을 할 수 있는 마음가짐을 갖게된 것 같다.

개발을 할 때 어떤 고민을 해야 하는지 배웠고 환경과 의지, 의식적인 연습의 중요성 등 이외에도 많은 것들을 배울 수 있었다. 만약 이 과정의 수강을 고민하고 있는 분이 내 주변에 있다면 꼭 들어보라고 말해줄 것 같다. 코드 리뷰를 통해 받았던 피드백 중 나중에 다시 와서 참고하고 싶은 내용들을 간략하게 정리했다.

자동자 경주

스트림을 사용해 가독성 높이기

ex 1

as-is

int sum = 0;
for (int number : numbers) {
    sum += number;
}
return sum;

to-be

return Arrays.stream(numbers).sum();

ex2

as-is

 for (String carName : carNames) {
    cars.add(new Car(carName));
}
return cars;

to-be

return Arrays.stream(carNames) 
    .map(Car::new) 
    .collect(Collectors.toList());

반복문에서 xxx.length 변수로 추출

아래 코드는 for문이 호출될 때 마다 values.length가 호출됨

int[] numbers = new int[values.length];
for (int i = 0; i < values.length; i++) {
    ...
}

아래와 같이 변수로 추출하기

int valuesLength = values.length;
int[] numbers = new int[valuesLength];
for (int i = 0; i < valuesLength; i++) {
    ...
}

validation 메서드명 고민

validation 후 throw를 하기 때문에 assertPositiveNumber 정도의 이름이 더 적합할 수도 있다.

private static void validateNumberIsPositive(int number) {
    if (number < 0) {
        throw new RuntimeException("음수는 입력될 수 없습니다.");
    }
}

생성 비용이 높은 (재사용) 변수는 static으로 선언

ex)

private static final Pattern CUSTOM_SEPARATOR_PATTERN = Pattern.compile("//(.)\n(.*)");

private static final Random RANDOM = new Random();

테스트에서 JUnit 어노테이션 적극 활용

null, "", " " 등은 @Nullsource와 @EmptySource 로 대체 가능

assertThat(splitAndSum(null)).isEqualTo(0);
assertThat(splitAndSum("")).isEqualTo(0);
assertThat(splitAndSum("   ")).isEqualTo(0);

아래와 같이 반복되는 assert는 @ParameterizedTest로 개선 가능

 assertThatThrownBy(() -> {
    splitAndSum("1,-2");
}).isInstanceOf(RuntimeException.class);

assertThatThrownBy(() -> {
    splitAndSum("1,a");
}).isInstanceOf(RuntimeException.class);

예외는 진짜 예외 상황에만 사용하기

(이펙티브 자바 69번 아이템)
아래와 같이 try-catch 에 코드를 넣으면 JVM이 적용할 수 있는 최적화가 제한된다.

try {
    number = Integer.parseInt(value);
} catch (NumberFormatException exception) {
    throw new RuntimeException("숫자 이의의 값은 입력될 수 없습니다.");
}

정말 예외 흐름을 타야하는 경우가 아니라면 아래와 같이 정상 흐름으로 개선하기. 이 경우 정규식 활용 가능

private static final Pattern NUMBER_PATTERN = Pattern.compile("\\d+");
...

Matcher numberMatcher = NUMBER_PATTERN.matcher(value);
if (!numberMatcher.matches()) {
    throw new RuntimeException("숫자 이의의 값은 입력될 수 없습니다.");
}

하위 계층에서 테스트한 코드를 상위 계층에서 다시 테스트 해야 하나?

질문

Position 생성자에 입력한 숫자를 position 인스턴스 변수로 갖는 객체가 잘 생성되었음을 이미 Position 단위 테스트에서 확인했는데 Position을 field로 가지면서 생성자에서 생성해 설정해주는 Car 객체에서도 객체 생성 후 field인 Position이 적절한 값을 갖는지 테스트하는게 의미가 있을지 궁금합니다.

답변

하위 계층에서 이미 테스트 된 기능이라면 상위 계층에서 다시 테스트는 하지 않아도 된다고 생각해요. 여기서는 Postion 의 테스트에서는 잘 생성 되었는지를 테스트 했다면 Car 의 테스트 에서는 단순히 Position 값이 있나 없나 보다는 조건에 따라 Position 의 값이 적절한지를 테스트 해주면 좋을거 같습니다.

외부에서 객체를 생성하는 메서드는 해당 클래스의 팩토리 메서드로 분리

GameController

public class GameController {
...
    private static RacingCarGame createRacingCarGame() {
        int carCount = InputView.getCarCount();
        int playCount = InputView.getPlayCount();
        return new RacingCarGame(carCount, playCount);
    }
...
}

위와 같은 코드를 RacingCarGame의 팩토리 메서드로 분리할 수 있다.

RacingCarGame

public class RacingCarGame {
    public static RacingCarGame of(int carCount, int playCount) {
        return new RacingCarGame(carCount, playCount);
    }

    // 생성자
    ...
}

컬렉션은 불변으로 리턴하기

List<Car> maxPositionCars = new ArrayList<>();
for (Car car : cars) {
    matPositionCars.add(car.getPlayResult);
}
return maxPositionCars;

스트림까지 같이 사용하면 다음과 같이 개선할 수 있다.

    return cars.stream()
            .map(Car::getPlayResult)
            .collect(Collectors.toUnmodifiableList());

참고자료 - 방어적 복사와 Unmodifiable Collection

Enum에 함수형 인터페이스 필드 활용하기

기존에는 enum과 팩토리 클래스에서 enum 타입에 따라 각각 다른 인스턴스를 생성해줬었는데 enum에 함수형 인터페이스 필드와 생성 메서드를 넣으면 변경 지점을 최소화 할 수 있다.

public enum MovingStrategyType {
    RANDOM(() -> new RandomMovingStrategy())

    private final Supplier<MovingStrategy> supplier;

    public static getInstance(MovingStrategyType type) {
        ...
    }
}

참고자료 - https://jojoldu.tistory.com/137

테스트는 최대한 리팩토링에 내성이 있도록 작성하기

아래 테스트는 우승한 자동차 를 찾는데, findMaxPositionCars() 메서드를 실행한 결과를 Name 으로 변환하고 다시 새로운 Name을 생성해 비교해서 검증하는 과정이 지나치게 구체적인 작업을 하고 있는 것 같다는 피드백을 받았다.

현재 테스트하려는 Cars 외에 다른 프로덕션 코드의 설계가 변경될 경우에도 영향을 받을 수 있으므로 리팩토링 내성이 약하다고 생각된다. 단순히 우승자 자동차를 변수에 담아서 containsExactly로 검증하는것이 어떻겠냐는 피드백을 주셨다.

@Test
void findMaxPositionCars() {
    Cars cars = new Cars(List.of(
        new Car(5, "carA"),
        new Car(5, "carB"),
        new Car(4, "carC"),
        new Car(3, "carD")));

assertThat(cars.findMaxPositionCars().stream()
    .map(Car::getName).toArray())
    .containsExactly(new Name("carA"), new Name("carB"));
}

참고자료 - https://github.com/bingbingpa/dev-book/blob/main/unit-test/README.md

테스트의 목적과 행위가 일치하도록 작성

아래 테스트 케이스는 생성을 검증하는 것인데도 move() 라는 게임 진행 메서드를 사용하고 있다.

@Test
void create() {
    String[] carNames = {"carA", "carB", "carC", "carD", "carE"};
    assertThat(new Cars(carNames).move(() -> true)).hasSize(5);
}

테스트는 문서의 역할도 수행하기 때문에 테스트의 목적과 행위가 달라진다면 테스트를 보는 다른 개발자가 이 메서드가 어떤 작업을 수행하는지 파악하기 어려워질 수 있다. 때문에 위 경우 컬렉션을 반환하는 생성 메서드를 만들고 (혹은 public/protected/default 로 개방) 테스트에 사용하는게 낫다. 다만 메서드를 추가하기 전에 설계의 방향이 맞는지 고민할 필요 있음. (private 메서드를 테스트 하고 싶다면 책임이 과도한 것)

로또

스트림 사용 시 한 줄엔 하나의 점 `.` 만 사용하기

// as-is
collection.stream().map(...).filter(...).collect(...);

// to-be
collection.stream()
        .map(...)
        .filter(...)
        .collect(...);

테스트만을 위해 존재하는 생성자 or 메서드

테스트만을 위해 생성자나 메서드를 추가하는 것은 최대한 지양하자. 동일한 기능을 수행하는 프로덕션 코드의 테스트가 어려워지기 때문에 추후 요구사항이 변경되어 프로덕션 코드만 수정된다면 테스트는 통과하고 프로덕션에서만 버그가 발생할 가능성이 존재한다. 테스트를 만드는 이유 중 하나로 하나의 로직이 변경되었을 때 발생할 수 있는 사이드 이펙트를 확인함이 있는데 위 경우에는 그걸 발견하기가 어려워질 수 있다.

또한 생성자를 추가한다는 것은 객체의 생성 지점을 추가한다는 의미이고 이는 객체가 변경될 때마다 확인해주어야 하는 지점이 늘어나 유지보수가 어려워지는 원인이 될수 있다고 한다.

테스트를 위한 생성자가 필요한 경우 test SourceSet 내부에 테스트 객체를 생성하기 위한 편의 메서드를 구현하는 방법을 고려하기

참고 자료 - 4단계 - 로또(수동) by gmelon · Pull Request #2780 · next-step/java-lotto

rangeClosed()

IntStream에는 range() 뿐만 아니라, rangeClosed()도 있어서 편하게 inclusive 값으로 지정할 수 있다.

enum 값이 key로 사용되는 경우 EnumMap 활용

아래와 같은 enum이 있는데

public enum LotteryRank {

    THREE(3, 5000),
    FOUR(4, 50000),
    FIVE(5, 1500000),
    SIX(6, 2000000000);

...

아래처럼 3, 4, .. 와 같은 값을 key로 갖는 HashMap을 사용하고 있었다..

Map<Integer, Integer> wonCounts = new HashMap<>();

이를 아래와 같이 개선하면 EnumMap이 내부적으로 비트 벡터를 사용하기 때문에 성능이 좋아질뿐만 아니라 enum을 Key로 직접 사용하기 때문에 이해하기 쉬운 코드가 된다.

Map<LotteryRank, Integer> wonCounts = new EnumMap<>(...);

테스트 데이터 생성 및 관리

테스트에서 필요한 데이터를 생성하는 것도 꽤 중요한 작업, 테스트 데이터를 만드는 메서드를 활용하는 것도 좋고 여러 테스트에서 중복되어 사용되는 값은 test SourceSet 하위에 클래스를 별도로 만들어서 사용하는 것도 좋다고 생각함.

상수 위치?

상수를 가장 잘 표현할 수 있는 도메인 객체에 두기

Lottery와 WinningLottery의 공통점을 추상화 & 상속보다는 조합을 사용

기존 일반 로또를 표현한 Lottery와 이를 상속받는 WinningLottery가 있었는데, 이 둘의 공통점을 추상화한 추상 클래스를 정의해서 사용하거나, WinningLottery가 Lottery를 필드로 갖도록 조합해서 사용하도록 개선

as-is - java-lotto/src/main/java/lottery/WinningLottery.java at step2 · gmelon/java-lotto
to-be - https://github.com/gmelon/java-lotto/blob/step4/src/main/java/lottery/WinningLottery.java

참고 자료 - 상속보다는 조합(Composition)을 사용하자.

enum에 기본 값에 해당하는 상수 만들기

기존에는 이런 식으로 enum을 두고

public enum LotteryRank {

    THREE(3, 5000),
    FOUR(4, 50000),
    FIVE(5, 1500000),
    SIX(6, 2000000000);

...

스트림에서 계산이 불가능한 등수면 다음과 같이 예외를 던졌는데

.orElseThrow(() -> new IllegalArgumentException("지원하지 않는 등수입니다."));

enum에 NONE(0, 0) 과 같은 값을 추가하면 아래처럼 개선할 수 있다.

.orElse(NONE);

모든 비지니스 로직을 검사하기

기존에는 아래처럼 각 케이스(내부적으로 isBonusMathced가 영향을 주는가? matchingCount가 0인가?)별로 하나씩만 검사해주면 된다고 생각했는데

@ParameterizedTest
@MethodSource("valueOfProvider")
void valueOf_일반(int matchingCount, boolean isBonusMatched, LotteryRank expectedRank) {
    assertThat(LotteryRank.valueOf(matchingCount, isBonusMatched)).isEqualTo(expectedRank);
}

static Stream<Arguments> valueOfProvider() {
    return Stream.of(
            Arguments.of(0, true, LotteryRank.NONE),
            Arguments.of(0, false, LotteryRank.NONE),
            Arguments.of(1, true, LotteryRank.NONE),
            Arguments.of(1, false, LotteryRank.NONE),
            Arguments.of(6, true, LotteryRank.FIRST),
            Arguments.of(6, false, LotteryRank.FIRST),

            Arguments.of(5, true, LotteryRank.SECOND),
            Arguments.of(5, false, LotteryRank.THIRD)
    );
}

그렇게 검사한 등수 없음, 1, 2, 3 등 말고 나머지 4, 5등에 대해서도 핵심 비지니스 로직이기 때문에 프로덕션 코드 내부적으로 로직이 동일하더라도 결과가 제대로 나오는지 반드시 검사해주어야 한다.

메서드 이름은 요구사항을 드러내도록 작성하기

기존에 계속 프로덕션 메서드 이름 변경 -> 테스트 메서드 이름 변경 과 같은 일이 반복되고 있었다. 테스트명은 테스트하려는 상황과 대상, 예상 결과를 표현하면 테스트를 이해하기도 쉽고 위와 같이 계속 메서드 명을 변경해야 하는 일도 발생하지 않는다.

이렇게 테스트 메서드 명을 작성하면 테스트가 요구사항에 대한 문서 역할도 할 수 있게 된다.

볼링

일급컬렉션 사용 시 불변 객체 고려하기

변경 커밋 - 🚀 1단계 - 질문 삭제하기 기능 리팩토링 by gmelon · Pull Request #988 · next-step/java-bowling
참고자료 - https://jojoldu.tistory.com/412

테스트 코드 간의 의존성 끊기

기본적으로 상태가 변할 가능성이 있는 값을 정적 변수로 사용하면 버그 발생 가능성이 매우 높다
- 테스트에서 상태를 변경하는 변수는 인스턴스 변수로 사용하기
어쩔 수 없는 경우 @BeforeEach 로 초기화하기
- 테스트가 끝난 후보다는 시작 전에 초기화하는게 더 좋다
- 다른 어떤 테스트에서 영향을 받았을지 모르므로 시작 전 깔끔하게 초기화하는 게 더 안전

상속 시 부모의 필드도 private으로 구현하기

부모의 필드를 protected로 설정하고 직접 접근하기보다 private로 설정하고 메서드를 통해 접근하는 것을 추천.

2023 하반기 CJ올리브네트웍스 지원 후기 (0)	2023.12.29
인프콘 2023 후기 (0)	2023.08.31
2022 회고 (0)	2023.01.21
GDG Daegu - devfest 2022 후기 (0)	2023.01.21
쎄트렉아이 인턴 후기 (1)	2022.12.07

gmelon's devlog