자동차 경주 1단계 미션 리뷰 수정 및 2단계 문자열 계산 소스 생성 #5173
Conversation
There was a problem hiding this comment.
안녕하세요 지경님. 속도도 중요하지만, 더 중요한게 방향이니 꾸준히만 따라와주시면 좋습니다 👍
2단계 미션 구현하신부분들 보니 피드백이 필요한부분들이 있어 코멘트 남겨드렸으니 확인 부탁드려요.
추가적으로 지금 private 함수들중 여러 로직들이 구현되어있는 부분들에 대해서 테스트 케이스가 작성되어있지 않은 것 같아요. 일반적으로 public API를 기준으로 테스트를 작성하지만 그렇다고 private 함수들이 무시되어도 되는게 아닙니다. 보통 화이트박스 테스트의 조건검증, 문장검증등을 통해 해당 분기문이나 조건들을 제대로 타는지도 테스트가 되어야 하는것이죠. 이에 대해 바로 생각하기 힘들다면 플로우차트를 그려보는것도 유의미한 경험이 될 것 같아요.
그리고 현재 함수들에 대한 네이밍 컨벤션이 자바 컨벤션과는 다르기에 맞춰주시면 좋을 것 같습니다. 리뷰 확인후 다시 리뷰요청 부탁드려요!
|
|
||
| int result = ZERO; | ||
|
|
||
| // 1. 빈 문자열 또는 null 값을 입력할 경우 0을 반환해야 한다. |
There was a problem hiding this comment.
주석은 보통 함수가 의미를 충분히 전달할 수 없는 이름이거나, 상황이나 조건에 따라 성능이 달라지거나 외부적인 요인등 함수 시그니처로 충분히 설명이 되지 않고 인수인계가 필요한 경우 작성하는 편이에요.
그 외에는 불필요한 주석은 함수명과 중복되어 의미의 혼동 혹은 중복을 야기하며 가독성을 떨어트리는 편입니다.
| int result = ZERO; | ||
|
|
||
| // 1. 빈 문자열 또는 null 값을 입력할 경우 0을 반환해야 한다. | ||
| if(_checkValidNull(input)) { |
There was a problem hiding this comment.
자바 코드 컨벤션을 맞춰주시면 좋을 것 같습니다! 함수명 혹은 변수명등 모두 언더바로 시작하지 않고 알파벳 소문자로 시작하도록 가이드하고 있습니다.
참고: https://naver.github.io/hackday-conventions-java/#identifier-char-scope
| // 2. 숫자 하나를 문자열로 입력할 경우 해당 숫자를 반환한다. | ||
|
|
||
| if(_isNumeric(input)) { | ||
| result = _parseInt(input); |
There was a problem hiding this comment.
숫자 하나일 경우, 아래 다음 로직들을 탈 필요가 없을 것 같지 않나요?
| result = _parseInt(input); | |
| return parseInt(input); |
| private int _sumNumbericSplitByCommaOrColon(String input) { | ||
| int sumResult = 0; | ||
| String[] numbers = input.split(COMMA); | ||
| for(String number : numbers) { | ||
| if (_isNumeric(number)) { | ||
| sumResult += _parseInt(number); | ||
| } else { | ||
| if(_containsCharacter(number, COLON)) { | ||
| String[] numbersColon = number.split(COLON); | ||
| for(String numberColon : numbersColon) { | ||
| sumResult += _parseInt(numberColon); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return sumResult; | ||
| } |
There was a problem hiding this comment.
- 하나의 함수가 현재 문자열 분리, 변환, 합계등의 다양한 로직들을 책임지고 있습니다. 즉 테스트하기 힘든 함수로 보이네요. private함수인만큼 화이트박스로 테스팅이 필요한데, 해당 반복,분문에 대한 모든 검증 조건을 테스트하기 너무 까다롭지 않을까요 🤔
- 반복문 > 분기문 > 분기문 > 반복문 이처럼 너무 깊은 indent가 이뤄지고 있어요. 객체 지향 생활체조 원칙에서는 indent를 2로 줄여보라는 말을 하고 있는데 이 로직도 할 수 있지 않을까요? 보통 indent가 길어진다는건 하나의 함수에서 너무 많은 책임을 가져간다는 의미와 일맥상통합니다.
| private Matcher _containsCharacter(String input) { | ||
| Matcher m = Pattern.compile("//(.)\n(.*)").matcher(input); | ||
| if (m.find()) { | ||
| return m; | ||
| } else { | ||
| return null; | ||
| } | ||
| } |
| String customDelimiter = m.group(1); | ||
| String[] tokens= m.group(2).split(customDelimiter); |
There was a problem hiding this comment.
1과 2가 무엇을 의미하는지 알기 쉬울까요? 매직넘버는 상수로 분리해주면 좋을 것 같습니다
| public static StringAddCalculator getInstance() { | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
싱글턴 패턴으로 만드셨군요. 그렇다면 생성자 제한을 통해 해당 함수가 아니면 객체 생성을 못하도록 막는게 어떨까요?
그리고 보통 생성자의 위치가 최상단인만큼 싱글턴 관련 함수들도 최상단에 있는게 가독성을 높힐 수 있을 것 같아요
| if(input == null ) { | ||
| return true; | ||
| } | ||
| if(input.isEmpty()) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
| if(input == null ) { | |
| return true; | |
| } | |
| if(input.isEmpty()) { | |
| return true; | |
| } | |
| return false; | |
| return `null인가?` and `공백인가?`; |
여러 조건들을 묶을 수 없을까요?
자동차 경주 1단계 미션 리뷰 수정 및 2단계 문자열 계산 소스 생성
StringTest.java
불필요한 주석제거
TestCase에서 CsvSource나 ValueSource등을 이용
SetTest.java
Set 자료구조의 특징인 중복 제거에 대한 검사도 테스트 케이스 추가
진도가 늦은 편이지만 제 속도에 맞춰서 따라가보려고 하고 있습니다~
리뷰 기다리겠습니다! 감사합니다 :)