[ADD] calculator #10
[ADD] calculator #10
Conversation
|
|
||
| public class ResultView { | ||
| static void printOutResult(AddCalculator addCalculator, String[] numbers) { | ||
| System.out.println(addCalculator.calculate(numbers)); |
There was a problem hiding this comment.
view라는 클래스에서 printOutResult는 인자로 받은 것을 print하겠다는 추측으로 읽었는데, 사실상 여기서 addCalculator 연산을 진행하고있네요 이 resultView를 사용하고싶다면, addCalculator의 결과를 받아서 이곳에서 print만 하는게 역할이 분명해질 것 같습니다.
|
|
||
| public class CalculateApplication { | ||
| public static void main(String[] args) { | ||
| ResultView.printOutResult(new AddCalculator(), InputView.inputString(new Scanner(System.in))); |
There was a problem hiding this comment.
이 한줄에서 입력, 계산, 출력 모든게 이루어지고있네요. 변수명을 잘짓는 이유가 가독성을 높이기 위함인 것 처럼, 2줄을 1줄로 줄이는 것, 코드라인 사이를 띄어쓰기 하는것, 1줄을 2줄로 늘리는 것 역시 가독성을 높이기 위함입니다.
| return expression.split(separator); | ||
| } | ||
|
|
||
| private static boolean isBlankSpace(String inputString) { |
There was a problem hiding this comment.
is empty와 inputstring == " " 의 차이가 있나요?
| } | ||
|
|
||
| private static boolean isContains(String expression, String[] separators) { | ||
| if (expression.contains(separators[COMMA])) return true; |
There was a problem hiding this comment.
확장성이 걱정되네요, 만약 요구사항이 기본 separator가 , : ? 로 늘어나면 위에있는 new String[]{",", ":"}) 이 코드도 변경해야하고, splitString(expression, ",|:"); 이것도 변경하고 여기에있는 index도 수정해야겠네요.
또한 만약 인자값 separators가 {","} 만들어오면 index out of range 에러가 뜰수있습니다.
|
|
||
| public class AddCalculator { | ||
| public static int calculate(String[] numbers){ | ||
| return Arrays.stream(numbers).mapToInt(number -> Integer.parseInt(number)).sum(); |
There was a problem hiding this comment.
stream 연산은 stream 대로 줄바꿈 해주는게 디버깅 하기 좋다고 하더라구용!
디버깅은 한줄씩 체크하니까 !
| return Arrays.stream(numbers).mapToInt(number -> Integer.parseInt(number)).sum(); | |
| return Arrays.stream(numbers) | |
| .mapToInt(number -> Integer.parseInt(number)) | |
| .sum(); |
혹시 parseInt 가 실패하면 어떻게되나요?
|
|
||
| public static String[] makeNumbers(String inputString) { | ||
|
|
||
| if (isBlankSpace(inputString)) throw new RuntimeException(); |
There was a problem hiding this comment.
| if (isBlankSpace(inputString)) throw new RuntimeException(); | |
| if (isBlankSpace(inputString)){ | |
| throw new RuntimeException(); | |
| } |
가 더 가독성이 좋을 것 같아요!
There was a problem hiding this comment.
여기에 RuntimeException은 최상위 Exception이니 다른 걸로 바꾸기로 !!
pci2676
left a comment
pci2676
left a comment
There was a problem hiding this comment.
테스트 케이스가 적습니다! 조금 더 테스트를 추가해주세요!
|
|
||
| @Test | ||
| void calculate() { | ||
| assertThat(AddCalculator.calculate(new String[]{"1","2","3"})).isEqualTo(6); | ||
| } |
There was a problem hiding this comment.
@DisplayName을 이용해서 테스트의 의도를 먼저 드러내 주세요!
| if (!isContains(expression, new String[]{",", ":"})) { | ||
| throw new IllegalArgumentException(); | ||
| } |
There was a problem hiding this comment.
isContains 가 여기 말고 는 쓰이지 않는 것같은데 인자를 new String[]{",", ":"} 이렇게 넘길 필요가 있을까요?!
| } | ||
|
|
||
| private static boolean isBlankSpace(String inputString) { | ||
| if (inputString == null || inputString.isEmpty() || inputString == " ") { |
There was a problem hiding this comment.
inputString == " "로 공백을 캐치하려 한것 같은데 " " 공백이 더길어진다면 어떨까요?
trim()에 대해 알아보면 좋을것같아요~
5 participants
풀리퀘 드립니다.
감사합니다.