우테코 프리코스

2주차 코드에 대한 코드 리뷰

흰곰돌이 2023. 11. 3. 23:21

감사하게도 정말 많은 분들이 다양하게 코드 리뷰를 해주셨다. 이에 대해 일부 정리를 해보려한다.

 

public String showExecutionResults(ArrayList<String> carNames, ArrayList<Integer> carGoingCount) {
        StringBuilder result = new StringBuilder();
        for (int i = 0; i < carNames.size(); i++) {
            result.append(carNames.get(i)).append(" : ");
            for (int j = 0; j < carGoingCount.get(i); j++) {
                result.append("-");
            }
            result.append("\n");
        }
        return result.toString();
}

 

위와 같은 코드에서 이중 for문의 분리에 대한 의견을 여러 분들이 주셨다. 따라서 아래와 같이 안쪽 for문을 메서드로 따로 빼서 가독성을 올렸다.

 

public String showExecutionResults(ArrayList<String> carNames, ArrayList<Integer> carGoingCount) {
        StringBuilder result = new StringBuilder();
        for (int i = 0; i < carNames.size(); i++) {
            result.append(carNames.get(i)).append(" : ");
            appendHyphens(result, carGoingCount.get(i));
        }
        return result.toString();
}

private void appendHyphens(StringBuilder result, int count) {
        for (int i = 0; i < count; i++) {
            result.append("-");
        }
        result.append("\n");
}

전체적으로 주석을 줄이고 메소드, 변수 이름에 좀더 힘을 기울여서 지으면 어떨까 생각하신다는 의견도 매우 감사하다. 주석을 최소화하면서 어떻게 하면 코드 자체만으로 보시는 분들이 이해할 수 있으실지 더 고민해봐야겠다.

 

public String[] inputCarNames() {
    System.out.println(showInputCarNamesPrompt());
    String inputList = UserInput();
    String[] inputArr = inputList.split(",");
    return inputArr;
}

 

1주차 공통 피드백에서 Java Colletion을 사용하라고 하셨는데, 신경 쓴다고 썼었는데 이렇게 떡하니 배열이 있었다. 코드 리뷰를 달아주셔서 처음 깨달았다. 따라서 아래와 같이 코드를 변경했다.

 

public static List<String> inputCarNames() {
    System.out.println(showInputCarNamesPrompt());
    String input = UserInput();
    String[] inputs = input.split(",");

    List<String> inputArr = new ArrayList<>();
    for (String carName : inputs) {
        inputArr.add(carName);
    }
    return inputArr;
}
public void runGame() {
        InputView inputView = new InputView();
        String[] inputArr = inputView.inputCarNames();                       
        CarValidation carValidation = new CarValidation();
        ArrayList<String> carNames = carValidation.carValidateAll(inputArr); 

        String inputNumber = inputView.inputTryNumber();                 
        NumberValidation numberValidation = new NumberValidation();
        int tryNumber = numberValidation.numbervalidateAll(inputNumber);    
        ...
}

 

 

2주차 회고록에서도 언급했던 static 메서드에 대해서도 말씀해주셨다. 굳이 인스턴스를 생성할 필요가 없는 경우, 즉 인스턴스 변수를 사용하지 않는 경우에는 static 메서드로 접근하는게 맞다고 생각한다. 따라서 코드를 아래와 같이 변경했다.

 

public void runGame() {
        String[] inputArr = InputView.inputCarNames();                       
        ArrayList<String> carNames = CarValidation.carValidateAll(inputArr); 

        String inputNumber = InputView.inputTryNumber();                    
        int tryNumber = NumberValidation.numbervalidateAll(inputNumber);
        ...
}
assertThat(output).isEqualTo("Tom : ---\nBob : -----\nLisa : --\n");

 

 

stringBuilder 를 사용하는 건 어떠냐는 의견을 받아들여 아래와 같이 수정했다. 실제 출력값 형태로 보여서 가독성이 더 좋아진 것 같다.

 

StringBuilder expectedOutput = new StringBuilder();
expectedOutput.append("Tom : ---\n");
expectedOutput.append("Bob : -----\n");
expectedOutput.append("Lisa : --\n");

assertThat(output).isEqualTo(expectedOutput.toString());
public class Constants {
    public static final int MAX_CAR_NAME_LENGTH = 5;

    public static final int MIN_TRY_NUMBER = 1;

    public static final int MIN_RANDOM_NUMBER = 0;
    public static final int MAX_RANDOM_NUMBER = 9;

    public static final int FORWARD_CONDITION = 4;

    public static final String NAME_CHARACTER_PATTERN = "^[a-zA-Z0-9]*$";
}

 

 

Constants 파일에 리턴 메세지들도 빼는게 어떻겠냐는 의견이 있었고 생각해보니 맞는 거 같아서 코드를 수정했다. 따라서 메세지를 메서드를 통해 분리하던 부분이 삭제됐고 Constants 파일에 메세지들이 추가됐다.

 

public class Constants {
    public static final int MAX_CAR_NAME_LENGTH = 5;

    public static final int MIN_TRY_NUMBER = 1;

    public static final int MIN_RANDOM_NUMBER = 0;
    public static final int MAX_RANDOM_NUMBER = 9;

    public static final int FORWARD_CONDITION = 4;

    public static final String NAME_CHARACTER_PATTERN = "^[a-zA-Z0-9]*$";

    public static final String CAR_NAMES_PROMPT = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)";
    public static final String TRY_NUMBER_PROMPT = "시도할 회수는 몇회인가요?";

    public static final String EXECUTION_RESULTS_HEADER = "실행 결과";
    public static final String WINNER_MESSAGE = "최종 우승자 : ";
}
public void runGame() {
    List<String> inputArr = InputView.inputCarNames();
    ArrayList<String> carNames = CarValidation.carValidateAll(inputArr);

    String inputNumber = InputView.inputTryNumber();
    int tryNumber = NumberValidation.numbervalidateAll(inputNumber);
    System.out.println();

    Race race = new Race();
    race.initialize(carNames);

    OutputView outputView = new OutputView();
    System.out.println(Constants.EXECUTION_RESULTS_HEADER);
    for (int i = 0; i < tryNumber; i++) {
        race.runRace();
        System.out.println(outputView.showExecutionResults(race.getCarNames(), race.getCarGoingCount()));
    }
    System.out.println(outputView.showWinner(race.winners()));
}

 

 

runGame() 의 핵심 로직들을 함수로 분리하면 좋겠다는 의견도 있었다. 너무 많은 기능을 수행한다는 뜻. 따라서 코드를 아래와 같이 변경했다.

 

public void runStart() {
        List<String> inputArr = InputView.inputCarNames();
        List<String> carNames = CarValidation.carValidateAll(inputArr);

        String inputNumber = InputView.inputTryNumber();
        int tryNumber = NumberValidation.numbervalidateAll(inputNumber);
        System.out.println();

        Race race = new Race();
        race.initialize(carNames);

        runRaceAndPrintResults(race, tryNumber);
}

private void runRaceAndPrintResults(Race race, int tryNumber) {
        System.out.println(Constants.EXECUTION_RESULTS_HEADER.getValue());
        for (int i = 0; i < tryNumber; i++) {
            race.runRace();
            System.out.println(OutputView.showExecutionResults(race.getCarNames(), race.getCarGoingCount()));
        }
        System.out.println(OutputView.showWinner(race.winners()));
}
public class Constants {
    public static final int MAX_CAR_NAME_LENGTH = 5;

    public static final int MIN_TRY_NUMBER = 1;

    public static final int MIN_RANDOM_NUMBER = 0;
    public static final int MAX_RANDOM_NUMBER = 9;

    public static final int FORWARD_CONDITION = 4;

    public static final String NAME_CHARACTER_PATTERN = "^[a-zA-Z0-9]*$";

    public static final String CAR_NAMES_PROMPT = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)";
    public static final String TRY_NUMBER_PROMPT = "시도할 회수는 몇회인가요?";

    public static final String EXECUTION_RESULTS_HEADER = "실행 결과";
    public static final String WINNER_MESSAGE = "최종 우승자 : ";
}

 

 

상수 파일을 enum 클래스로 관리하는건 어떻겠냐는 의견이 있었고 아래와 같이 코드를 변경했다.

 

public enum Constants {
    MAX_CAR_NAME_LENGTH(5),
    MIN_TRY_NUMBER(1),
    MIN_RANDOM_NUMBER(0),
    MAX_RANDOM_NUMBER(9),
    FORWARD_CONDITION(4),
    NAME_CHARACTER_PATTERN("^[a-zA-Z0-9]*$"),
    CAR_NAMES_PROMPT("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"),
    TRY_NUMBER_PROMPT("시도할 회수는 몇회인가요?"),
    EXECUTION_RESULTS_HEADER("실행 결과"),
    WINNER_MESSAGE("최종 우승자 : ");

    private final Object value;

    Constants(Object value) {
        this.value = value;
    }

    public Object getValue() {
        return value;
    }
}

 

반복적인 public static final 을 없앨 수 있고 열거형이다보니 단순 나열보다는 한눈에 더 쉽게 들어온다. 또한 기본 생성자 타입이 private이기 때문에 인스턴스 생성과 상속을 방지하여 상수값의 타입안정성이 보장된다. 그리고 키워드 enum을 사용하기때문에 구현의 의도가 열거임을 분명하게 알 수 있는 장점이 있다.

public void runRace() {
    List<Integer> randomNumbersList = new ArrayList<>();
    for (int j = 0; j < carNames.size(); j++) {
        int randomNumber = Randoms.pickNumberInRange((int) Constants.MIN_RANDOM_NUMBER.getValue(),
                (int) Constants.MAX_RANDOM_NUMBER.getValue());
        randomNumbersList.add(randomNumber);
    }
    for (int j = 0; j < randomNumbersList.size(); j++) {
        if (randomNumbersList.get(j) >= (int) Constants.FORWARD_CONDITION.getValue()) {
            carGoingCount.set(j, carGoingCount.get(j) + 1);
        }
    }
}

 

for문 보다는 stream을 사용하면 가독성이 더 좋아질 거 같다는 의견에 한 번 stream으로 코드를 다시 작성해보았다.

 

public void runRace() {
    List<Integer> randomNumbersList = carNames.stream()
            .map(name -> Randoms.pickNumberInRange((int) Constants.MIN_RANDOM_NUMBER.getValue(),
                    (int) Constants.MAX_RANDOM_NUMBER.getValue()))
            .collect(Collectors.toList());

    IntStream.range(0, randomNumbersList.size())
            .filter(j -> randomNumbersList.get(j) >= (int) Constants.FORWARD_CONDITION.getValue())
            .forEach(j -> carGoingCount.set(j, carGoingCount.get(j) + 1));
}

 

  1. List<Integer> randomNumbersList = carNames.stream()...
    • carNames 리스트의 각 요소에 대한 스트림을 생성한다.
    • map 연산을 사용하여 각 자동차의 랜덤 숫자를 계산한다.
    • collect(Collectors.toList())를 사용하여 각 자동차의 랜덤 숫자를 수집하고 randomNumbersList 리스트에 저장한다. 이 리스트는 각 자동차에 대한 랜덤 숫자 목록을 나타낸다.
  2. IntStream.range(0, randomNumbersList.size())...
    • 0부터 randomNumbersList의 크기 - 1 까지의 범위에 대한 IntStream을 생성합니다. 이 범위는 0부터 자동차의 개수까지의 정수를 생성한다.
    • filter를 사용하여 랜덤 숫자가 전진 조건 (상수 FORWARD_CONDITION보다 크거나 같은지)을 충족하는 자동차를 필터링한다. j는 현재 자동차의 인덱스를 나타낸다.
    • forEach를 사용하여 각 전진한 자동차에 대해 carGoingCount 리스트의 해당 인덱스를 증가시킨다. 이를 통해 각 자동차의 전진 횟수를 추적할 수 있다.

테스트 케이스들을 어노테이션으로 더 간편하게 만들 수 있다는 의견도 있었다. valueSource을 더 알아보라고 권유하셨다. valueSource는 아래와 같은 상황에서 유용했다.

 

@Test
@DisplayName("User 생성 name 2자 미만 예외처리")
void createUserException01() {
      IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> new User(VALID_EMAIL, "q", password));
      assertThat(e.getMessage()).isEqualTo(NAME_NOT_MATCH_MESSAGE);
}

@Test
@DisplayName("User 생성 name 10자 초과 예외처리")
void createUserException02() {
      IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> new User(VALID_EMAIL, "qwerasdfzxcv", password));
      assertThat(e.getMessage()).isEqualTo(NAME_NOT_MATCH_MESSAGE);
}

@Test
@DisplayName("User 생성 name 숫자 포함 예외처리")
void createUserException03() {
      IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> new User(VALID_EMAIL, "qq23", password));
      assertThat(e.getMessage()).isEqualTo(NAME_NOT_MATCH_MESSAGE);
}

 

위와 같이 같은 경우에 대해 테스트를 하는데 입력값만 다를 경우 아래와 같이 valueSource를 통해서 코드를 간단하게 만들 수 있다.

 

@ParameterizedTest
@ValueSource(strings = {"q", "qwerasdfzxcv", "qq23"})
void createUserException(String name){
      IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> new User(VALID_EMAIL, name, password));
      assertThat(e.getMessage()).isEqualTo(NAME_NOT_MATCH_MESSAGE);
}

 

하지만 내 코드의 경우 유효성 검사는 같은 메서드를 호출하지만, 그 메서드 안에서 각각의 유효성 검증 메서드가 있기 때문에 각 예외 처리에 대한 출력값 역시 다르다. 따라서 valueSource 만으로 입력값만 간편하게 묶어주는 방법은 각각의 다른 출력값을 표현하지 못한다. 좀 더 공부해보니 이에 대한 해결방안은 각 입력값에 대응하는 출력메세지를 함께 묶어주는 methodSource 라는 방법이다.

 

@Test
void validateAll_too_longCarNames() {
    // given
    List<String> validCarNames = Arrays.asList("Car123454", "Car5", "Car1");

    // when & then
    assertThatThrownBy(() -> carValidation.carValidateAll(validCarNames))
            .isInstanceOf(IllegalArgumentException.class)
            .hasMessage("자동차 이름이 5자 이하가 아닙니다: " + validCarNames.get(0));
}

@Test
void validateAll_invalidCarNames() {
    // given
    List<String> validCarNames = Arrays.asList("Car@4", "Car5$", "Car1");

    // when & then
    assertThatThrownBy(() -> carValidation.carValidateAll(validCarNames))
            .isInstanceOf(IllegalArgumentException.class)
            .hasMessage("특수문자가 포함된 문자열이 입력되었습니다: " + validCarNames.get(0));
}

@Test
void testValidateDuplication() {
    // given
    List<String> validCarNames = Arrays.asList("Car2", "Car2", "Car1");

    // when & then
    assertThatThrownBy(() -> carValidation.carValidateAll(validCarNames))
            .isInstanceOf(IllegalArgumentException.class)
            .hasMessage("중복된 이름이 입력되었습니다: " + validCarNames.get(1));
}

 

위와 같은 내 코드를 methodSource 방법을 통해 간단하게 만들 수 있다.

 

static Stream<Arguments> invalidParameters() {
    return Stream.of(
            Arguments.of("Car1", "Car123454", "Car3", "자동차 이름이 5자 이하가 아닙니다: Car123454"),
            Arguments.of("Car1", "Car@4", "Car3", "특수문자가 포함된 문자열이 입력되었습니다: Car@4"),
            Arguments.of("Car1", "Car1", "Car2", "중복된 이름이 입력되었습니다: Car1")
    );
}

@ParameterizedTest
@MethodSource("invalidParameters")
void validateAll(String car1, String car2, String car3, String errorMessage) {
    // given
    List<String> validCarNames = Arrays.asList(car1, car2, car3);
    
    // when & then
    assertThatThrownBy(() -> CarValidation.carValidateAll(validCarNames))
            .isInstanceOf(IllegalArgumentException.class)
            .hasMessage(errorMessage);
}

에러메세지 역시 앞선 상수들처럼 enum 클래스로 관리해줬다.

 

public enum ErrorMessages {
    CAR_NAME_LENGTH_EXCEED("자동차 이름이 5자 이하가 아닙니다: %s"),
    NAME_CHARACTER_PATTERN_MISMATCH("특수문자가 포함된 문자열이 입력되었습니다: %s"),
    DUPLICATE_NAME("중복된 이름이 입력되었습니다: %s"),
    NOT_A_NUMBER("숫자가 아닌 값을 입력하셨습니다."),
    MINIMUM_TRY_NUMBER("잘못된 시도 횟수입니다.");

    private final String message;

    ErrorMessages(String message) {
        this.message = message;
    }

    public String getMessage() {
        return message;
    }
}
public String winners() {
    List<Integer> carGoingCountCopy = new ArrayList<>(carGoingCount);
    Collections.sort(carGoingCountCopy, Collections.reverseOrder());
    int maxCount = carGoingCountCopy.get(0);

    StringJoiner joiner = new StringJoiner(", ");
    for (int i = 0; i < carGoingCount.size(); i++) {
        if (maxCount == carGoingCount.get(i)) {
            joiner.add(carNames.get(i));
        }
    }
    return joiner.toString();
}

 

메서드를 분리하면 좋겠다는 의견이 나왔고 따라서 maxCount를 추출하는 부분을 분리했다.

 

public String winners() {
    int maxCount = maxCountGenerator();

    StringJoiner joiner = new StringJoiner(", ");
    for (int i = 0; i < carGoingCount.size(); i++) {
        if (maxCount == carGoingCount.get(i)) {
            joiner.add(carNames.get(i));
        }
    }
    return joiner.toString();
}

private int maxCountGenerator() {
        List<Integer> carGoingCountCopy = new ArrayList<>(carGoingCount);
        Collections.sort(carGoingCountCopy, Collections.reverseOrder());
        int maxCount = carGoingCountCopy.get(0);
        return maxCount;
}

 

이외에도 Car 클래스를 따로 만들어서 runRace() 부분을 수행하거나, 변경에 용이하게 인터페이스 타입 지정에 대해서도 말씀해주셨다. 이 부분은 추후에 다시 해보기로 기약하며 여기서 글을 마치겠다.