AddStringPOJO(5) - [Refactor] 메서드 책임 분할

패키지 구조가 명확하지 않고, 그 마저도 각 클래스가 제대로 자기 위치에 있는거 같지 않습니다. 벨리데이션 클래스가 굳이 다 public 이여야 할까요 코드 컨벤션 맞춰 주시고 커밋 전 오타, 컨벤션 등 기본적인거 확인 해주세요 문제가 어렵지 않은데 너무 어렵게 생각하고, 짜시는거 같습니다 네이밍이 전체적으로 너무 불 명확해서 코드를 하나하나 다 보지 않는다면 하나의 메소드가 무슨 일을 하는지 찾기 힘듭니다 발생하는 예외가 왜 다 일리걸알규먼트 익셉션 일까요 하나의 메소드는 하나의 일만 하게 구현 해주세요 각 메소드 별로 뎊스가 너무 깊습니다. if, for 와 같은 경우 최대 2뎊스 정도로 생각하고 구현 해주세요 기본적인 자바의 개념이 너무 부족합니다. 자바의 정석과 같은 아무 기본서, 혹은 인터넷 강의를 보면서 구현 하셔야 할 것 같습니다 현재 구현 방식으로는 상속과 같은 개념을 이용하는게 좋아 보이는데 아무데도 사용되지 않습니다 접근제어자에 대한 개념이 특히 부족해 보입니다 앞서 리뷰했던 대부분의 문제점들이 잘 고쳐지지 않고 있습니다. 리뷰 해드렸던 내용들 정리해서 확인하고, 공부하고, 구현 하셔야 할 것 같습니다. 현재는 공부가 많이 부족해 보입니다. 그리고 테스트코드를 아예 안 짜고 계신데 따로 이유가 있을까요? 어떤 기능을 구현하기전에 많은 고민을 하고, 구현을 하고 나서도 이게 최선이 맞는지 고민 해보시기 바랍니다 또한 코드에서 같은 일을 2번이상 반복한다면 코드가 잘 못 구현되었을 확률이 높습니다.

해당 문제점을 고치라고 받았다.

public class Calculate {

    public static int add(List<String> numberList) {
        int sum = 0;
        if (numberList.isEmpty()) {
            return sum;
        }
        try {
            for (String number : numberList) {
                sum += Integer.parseInt(number);
            }
        } catch (IllegalArgumentException e) {
            throw new IllegalArgumentException(MUST_CHANGE_NUMBER.getMessage());
        }
        return sum;
    }

}

해당 클래스의 문제점으로, 변수명이 numberList인데 공백이 존재하는 것이 맞는지에 대해 리뷰를 받았고, 스트림으로 간결히 표현해도 되지 않을까에 대한 리뷰, 변환이 되지 않을 때 IllegalArgumentException인지에 대한 리뷰를 받았다.

public class Calculate {

    public static int add(List<Integer> numberList) {
        int sum = 0;
        if (numberList.isEmpty()) {
            return sum;
        }
        try {
            sum = numberList.stream().reduce(0,Integer::sum);
        } catch (NumberFormatException e) {
            throw new NumberFormatException(MUST_CHANGE_NUMBER.getMessage());
        }
        return sum;
    }

}

 

위 코드는 코드리뷰를 반영한 코드이다.

그리고 Input.class에서

private static void exit(String input) {
    if (input.equals("exit")) {
        System.exit(1);
    }
}

위 코드에서는 System.exit(1); 이것을 사용하면 안 되는 이유에 대해 찾아보면 좋을 것 같다는 리뷰를 받았다.

찾아보니 System.exit(1)은 비정상 종료여서 지양하는 것이 좋다는 것을 알았다.

private static void exit(String input) {
    if (input.equals("exit")) {
        System.exit(0);
    }
}

그래서 정상 종료인 System.exit(0)으로 변경하였다.

그리고 Input.class에서

public static List<Integer> getNumberList() {
    while (true) {
        System.out.println("종료하려면 exit을 입력하세요");
        String input = scanner.nextLine();
        try {
            exit(input);
            validation.validateInput(input);
            return NumberExtract.split(input, Separator.separatorList);
        } catch (IllegalArgumentException e) {
            System.out.println(e.getMessage());
        }
    }

해당 메서드에서 너무 많은 책임이 있다라는 리뷰를 받았다.

public class Input {

    private final Validation validation = new Validation();
    private final Scanner scanner = new Scanner(System.in);

    public Input() {
    }

    public List<Integer> getNumberList() {
        String input = getInputString();
        return extractNumber(input);
    }

    private List<Integer> extractNumber(String input) {
        try {
            return NumberExtract.split(input, Separator.separatorList);
        } catch (IllegalArgumentException e) {
            System.out.println(e.getMessage());
            throw e;
        }
    }

    private void exit(String input) {
        if (input.equals("exit")) {
            System.exit(0);
        }
    }

    private void display() {
        System.out.println("종료하려면 exit을 입력하세요");
    }

    private void validationInput(String input) {
        try {
            validation.validateInput(input);
        } catch (IllegalArgumentException e) {
            throw e;
        }
    }

    private String getInputString() {
        while (true) {
            display();
            String input = scanner.nextLine();
            exit(input);
            try {
                validationInput(input);
                return input;
            } catch (IllegalArgumentException e) {
                System.out.println(e.getMessage());
            }
        }
    }

}

메서드를 쪼개서 책임을 나누어 주었다.

NumberExtract.class에서 무엇을 replace 하는지 명시해주면 좋을 것 같다는 리뷰를 받았다.

private static String replace(String input, List<String> separatorList) {
    for (String separator : separatorList) {
        input = input.replace(separator, "");
    }
    return input;
}

그래서 구분자인 Separator를 replace한다는 의미의 메서드 명으로 바꾸었다.

private static String replaceSeparator(String input, List<String> separatorList) {
    for (String separator : separatorList) {
        input = input.replace(separator, "");
    }
    return input;
}

또한, 

public static List<String> split(String input, List<String> separatorList) {
        String replaceString = replace(input, separatorList);
        String deleteSpace = replaceString.trim();

해당 메서드에서 처음부터 숫자만 담아두면 할 일이 하나 주는 것이 아닐까요 라는 리뷰를 받았다.

public static List<Integer> split(String input, List<String> separatorList) {
    String replaceString = replaceSeparator(input, separatorList);
    int[] numberArr = Arrays.stream(replaceString.split("")).mapToInt(Integer::parseInt).toArray();
    return Arrays.stream(numberArr).boxed().collect(Collectors.toList());
}

그래서 stream 형식으로 바꿔주었다.

public enum ErrorMessage {

    INPUT_NOT_BLANK("입력 문자열에 공백이 존재하면 안 됩니다."),
    CUSTOM_MUST_NEWLINE("Custom Separator 에는 개행문자가 포함되어야 합니다."),
    CUSTOM_NOT_EMPTY("Custom Separator 는 비어져 있으면 안 됩니다."),
    CUSTOM_NOT_NUMBER("Custom Separator 에는 숫자가 될 수 없습니다."),
    MAX_INPUT_LENGTH("입력 문자열의 최대 길이 100을 초과했습니다."),
    CONTINUOUS_SEPARATOR("입력 문자열에 연속된 구분자가 존재하면 안 됩니다."),
    DUPLICATE_CUSTOM_SEPARATOR("중복된 커스텀이 존재합니다."),
    SEPARATOR_NOT_FOUND("존재하지 않는 구분자입니다."),
    MUST_CUSTOM_START_WITH("커스텀 문자열의 //은 문자열 앞부분에 존재해야 합니다."),
    MUST_CHANGE_NUMBER("숫자로 변환 가능해야합니다.");

    private final String message;

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

    public String getMessage() {
        return message;
    }

}

해당 코드는 에러 메시지의 순서가 이상하다는 리뷰를 받았다. custom, input 식으로 순서를 맞추도록 수정하였다.

public enum ErrorMessage {

    CUSTOM_MUST_NEWLINE("Custom Separator 에는 개행문자가 포함되어야 합니다."),
    CUSTOM_NOT_EMPTY("Custom Separator 는 비어져 있으면 안 됩니다."),
    CUSTOM_NOT_NUMBER("Custom Separator 에는 숫자가 될 수 없습니다."),
    DUPLICATE_CUSTOM_SEPARATOR("중복된 커스텀이 존재합니다."),
    MUST_CUSTOM_START_WITH("커스텀 문자열의 //은 문자열 앞부분에 존재해야 합니다."),

    INPUT_NOT_BLANK("입력 문자열에 공백이 존재하면 안 됩니다."),
    MAX_INPUT_LENGTH("입력 문자열의 최대 길이 100을 초과했습니다."),
    CONTINUOUS_SEPARATOR("입력 문자열에 연속된 구분자가 존재하면 안 됩니다."),
    SEPARATOR_NOT_FOUND("존재하지 않는 구분자입니다."),

    MUST_CHANGE_NUMBER("숫자로 변환 가능해야합니다.");

    private final String message;

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

    public String getMessage() {
        return message;
    }
    
}

https://naver.github.io/hackday-conventions-java/

캠퍼스 핵데이 Java 코딩 컨벤션

참고한 컨벤션이다.