우테코 프리코스 1주차 나의 코드리뷰

프리코스 1주차가 끝나고 사람들과 코드리뷰를 나누었습니다. 

제가 놓친 부분이나 몰랐던 부분을 타인에 시선에서 객관적으로 받아 들일 수 있는 좋은 기회 였습니다. 

static으로 정의한 이유

질문 코드

public class Calculator {

    private OutputView outputView;
    private InputView inputView;
    private Separator separator;

코멘트

추가로 취향(?)이지만 메서드 파라미터로 받으신 필드 값이 의도적으로 변경될 여지가 없다면 
final키워드로 묶어보는게 어떨까요? 다른 개발자가 이 코드를 봤을때 final이 붙어 있으면 필드 변경에 
좀 더 신중할 수 있을거 같습니다! (항상 소통의 문제로 의도치 않은 문제들이 발생하더라구요...!)

나의 리플

'다른 개발자가 이 코드를 봤을때 final이 붙어 있으면 필드 변경에 좀 더 신중할 수 있을거 같습니다!'
라는 말에 동의 합니다. 변경의 여지가 없음을 나타내주는 것이 좋을 것 같습니다. 

BigDecimal

질문 코드

    public void printResult(Numbers numbers) {
        Double sum = numbers.getSum();
        if(sum%1 == 0) {
            String result = String.format("%.0f", sum);

코멘트 - bowook

코드에 대한 분석을 보니, 양의 실수를 처리하기 위해 Double 타입을 선택한 점이 인상적입니다.
특히 문제의 요구사항을 충실히 반영하려는 노력이 보이는 코드인 것 같아요!
Double 타입을 사용할 경우 근삿값이 나올 수 있다는 점에서 %.0f 형식을 활용하여 이를 처리하려고 한 시도가 눈에 띄네요!

다만, Double 타입은 오차가 발생할 수 있는 특성이 있기 때문에 정확한 계산이 필요한 경우에는 BigDecimal 타입을 사용하는 방법을 고려해 볼 수 있습니다. 이를 통해 오차를 최소화할 수 있는 장점이 있으니, 해당 기능도 알아보면 좋겠습니다!

나의 리플

BigDecimal 타입은 생각도 안해보았습니다. 존재 자체는 알고 있는데 사용해본적이 없어서 익숙하지 않습니다.
부동 소수점에 대한 내용을 조금만 알고 있어서 조금 더 공부해보고 나중에 코드에 적용해보겠습니다. 감사합니다.

overflow

질문 코드

 		public Double getSum() {
        Double sum = 0.0;
        for (Double number : numbers) {
            sum += number;
        }
        return sum;
    }

코멘트 - gyuoo

overflow에 대한 고려가 되어있지 않은 것으로 확인되는데, 맞을까요?

나의 리플

overflow에 대해 간과했던것 같습니다. 알려주셔서 감사합니다. 

예외처리

질문 코드

    private String getNumberString(String readString) {
        int subStringInt = readString.indexOf(CUSTOM_SEPARATOR_END) + CUSTOM_SEPARATOR_END.length();
        String substring = readString.substring(subStringInt);
        return substring;
    }

코멘트 - gyuoo

커스텀 구분자가 중간에 있는 경우도 고려된 것 같네요!
readString.indexOf(CUSTOM_SEPARATOR_END)가 -1인 경우나
\\\\n1:2:3과 같은 입력도 예외 처리를 했다면 더 좋았을 것 같습니다.

나의 리플

getNumberString메서드가 호출되기 이전에 hasCustomSeparator 이라는 메서드를 통해서 readString.indexOf(CUSTOM_SEPARATOR_END) 가 -1인 경우의 예외처리를 하고 있습니다.
마찬가지로 \\n1:2:3 입력의 경우에도 hasCustomSeparator 에서 예외처리를 하고 있습니다.

리펙토링1

질문 코드

        if(readString.contains(CUSTOM_SEPARATOR_START) && readString.contains(CUSTOM_SEPARATOR_END)){
            return true;
        }
        return false;

코멘트 - gyuoo

return readString.contains(CUSTOM_SEPARATOR_START) && readString.contains(CUSTOM_SEPARATOR_END) 
은 어떤가요?

나의 리플

제가 놓친것 같습니다. 알려주셔서 감사합니다. ㅎㅎ

빈 값 0으로 출력하기

질문 코드

        if(s.isEmpty()) {
            throw new IllegalArgumentException("[ERROR]덧셈할 문자열을 입력해 주세요");
        }

코멘트 - gyuoo

요구사항에 빈 값이 입력되는 경우 0을 출력해야 하는 것으로 알고 있습니다!

나의 리플

알려주셔서 감사합니다. 이걸 놓쳤었네요 ㅠㅠ

symbolArray

질문 코드

    private List<Double> split(String numberString) {
        List<Double> numberList;
        String[] symbolArray = symbols.split(CUSTOM_SEPARATOR_CENTER_REGEX);

코멘트 - sunkong25

symbols로 문자열을 합쳤다가 symbolArray로 나눠서 다시 저장하신 것 같은데 왜 이런 방법을 
사용했는지 궁금합니다!

나의 리플

구분자를 추가할 때 특수 문자나 숫자 등 문제가 발생할수 있는 문자들을 전부 ','로 치환해서 
split을 해주면 문제가 발생하지 않을 것 같아서 다시 배열에 저장하였습니다.