피드백에 대한 1차 수정 #1920
피드백에 대한 1차 수정 #1920
Conversation
There was a problem hiding this comment.
이전 PR 보다 객체로 설계하려한게 보여서 칭찬드리고 싶네요.
다음에는 아래의 원칙들을 지키면서 설계하면 좋을거 같습니다.
1. (중요) 객체지향 원칙중 단일책임원칙을 공부해보세요.
현재 코드는 전혀 책임분리가 이루어지지 않았습니다. 입력, 출력, 비지니스 로직이 구분되지않고 있습니다. 로직 안에서도 계산기 객체가 계산기의 책임과 parse 의 책임을 동시에 가지고 있습니다. 단일책임원칙과 객체, 디자인 패턴과의 관계를 공부해보세요.
2. 코드 가독성
-
메서드 이름, 변수 이름은 코드를 들여다보지 않더라도 의미를 추측할 수 있어야 합니다. 예를들어 다른 누군가가 calculate 객체의 plus 메서드를 호출한다면 코드를 들여다보지 않더라도 더하기 기능을 수행하겠구나 하고 생각할 수 있어야 합니다.
-
매직넘버를 제거해주세요. 단순히 숫자, 문자로 로직을 수행하였을때, 해당 의미를 명확히 파악하기도 어려울 뿐만 아니라 확장성이 떨어집니다.
3. 테스트 코드를 작성하는 연습을 해보세요.
이거는 당장은 아니더라도 테스트 코드를 작성하는 습관을 들이면 내 코드가 얼마나 많은 커버리지를 지녔는지, 예외상황을 조금 더 폭넓게 커버할 수 있습니다.
| } | ||
|
|
||
| public int getOutput(String input) { | ||
| int i = parse.parseCustom(input); |
There was a problem hiding this comment.
변수 이름을 조금 더 의미 있게 작성해 주시면 좋겠습니다. 현재 이름만으로는 역할을 이해하기 어렵습니다.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Parse { |
There was a problem hiding this comment.
클래스 이름으로는 동사보다는 명사가 적합합니다. Parser 와 같이 변경하면 어떨까요
| custom.add(','); | ||
| custom.add(':'); |
There was a problem hiding this comment.
단순히 ',' 와 ':' 을 하드코딩하는것 보단 아래와 같이 상수로 분리하여 매직넘버를 제거해주세요.
private static final char DEFAULT_DELIMITER1 = ',';
private static final char DEFAULT_DELIMITER2 = ':';| if (input.length() < 5) { | ||
| return 0; | ||
| } |
|
|
||
| public int parseInteger() { | ||
| int result = 0; | ||
| if (!(value.isEmpty())) { |
There was a problem hiding this comment.
StringBuilder 는 isEmpty() 메서드가 없습니다. string 으로 변환하여 사용하거나 value.length() > 0와 같이 변경해주세요.
| return custom; | ||
| } | ||
|
|
||
| public int parseCustom(String input) { |
There was a problem hiding this comment.
해당 메서드의 반환값은 계산을 시작할 시작 인덱스인데 이름이 parseCustom 이 맞을까요?
| result = calculate(input,i); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
아래와 같이 축소할 수 있을것 같습니다.
return calculate(input,i);2 participants
피드백에 대한 1차 수정입니다.