1주차 - Step1 문자열 계산기 구현 #284
Conversation
| .isThrownBy(() -> { | ||
| calculator.divide(1, 0); | ||
| }).withMessageContaining("by zero"); | ||
| } |
There was a problem hiding this comment.
단위테스트에는 하나의 assertion만 작성하는 것이 좋다고 생각합니다.
테스트가 실패할 경우 이전 테스트 결과로 인한 전파가 발생했는지 단독적인 이슈인지를 직접 확인해봐야 하기 때문입니다.
그리고 테스트당 개념 하나만을 테스트해야 한다는 원칙도 위배되는 경우가 발생할 우려도 있습니다.
이 테스트 케이스의 경우 예외처리에 대한 테스트와 기능에 대한 테스트를 같이 진행하고 있습니다.
|
|
||
| int calculationResult = Integer.parseInt(separatedElements[0]); | ||
| for(int i=1; i<separatedElements.length; i+=2) { | ||
| String operatorType = separatedElements[i]; //TODO: separatedElements[i]가 연산자를 뜻한다는 것을 파악하기 쉽도록 변수에 담아서 사용했는데요, 불필요할까요? |
There was a problem hiding this comment.
의미를 가지는 경우 지역변수로 드러내는 것이 좋다고 생각합니다. 따라서 필요한 부분이라고 생각합니다.
| public int calculateElements(String[] separatedElements) { | ||
|
|
||
| int calculationResult = Integer.parseInt(separatedElements[0]); | ||
| for(int i=1; i<separatedElements.length; i+=2) { |
There was a problem hiding this comment.
공백 컨벤션위반이 있는데요. intellij 의 경우 option + command + l 단축키를 활용해보세요.
자바 컨벤션과 관련해서는 https://myeonguni.tistory.com/1596 링크를 참조해보세요.
There was a problem hiding this comment.
공유해주신 자바 컨벤션 글을 읽어보니, 제가 지금까지 잘못 사용하고 있던 것들이 엄청 많았네요!
직접 링크 공유해주셔서 너무 감사합니다! 👍 👍
|
|
||
| public String operate(String inputString) { | ||
|
|
||
| validator.stringValidation(inputString); //TODO: 처음에는 이 부분을 runCalculatorApplication 메서드에 두었었는데요, 공백과 null을 체크하는 이 메서드를 어디서 실행하는게 좋을까요? |
There was a problem hiding this comment.
input 값에 대한 검증이라면 presentation layer 에서 확인하는 것이 좋다고 생각합니다.
값 검증이 도메인 모델에서 진행하는 것이 유의미할 경우 (계산기 연산자에 포함되는지, 아니면 이 계산기의 경우 양의 정수만을 받는 것인지 등)에는 도메인 모델에서 진행되어야 한다고 생각합니다.
https://stackoverflow.com/questions/21073878/validation-in-3-tier-architecture
| Operator operator = new Operator(); | ||
| return operator.operate(inputString); | ||
| } catch (IllegalArgumentException e) { | ||
| return "공백을 입력하셨거나, 연산자 기호가 아닌 기호를 입력하셨습니다."; //TODO: 공백 입력의 경우와 잘못된 연산자 입력한 경우를 각각 잡으려면 어떤 방식이 좋을까요? |
There was a problem hiding this comment.
custom exception을 작성하시면 될거 같아요. 추후 미션에서 진행되니 연습할 기회가 있을 거에요.
그리고 지금의 try catch 구조는 anti pattern 입니다.
https://www.slipp.net/questions/350 를 참고해보세요
| System.out.println("연산 결과 : " + result); | ||
| } | ||
|
|
||
| public static String runCalculatorApplication(String inputString) { |
There was a problem hiding this comment.
method의 이름은 행위를 드러내는 동사를 사용하는 것이 좋다고 생각합니다.
http://www.mimul.com/pebble/default/2013/05/04/1367639300999.html
| if("+".equals(operatorType)) return add(leftValue, rightValue); | ||
| if("-".equals(operatorType)) return subtract(leftValue, rightValue); | ||
| if("*".equals(operatorType)) return multiple(leftValue, rightValue); | ||
| if("/".equals(operatorType)) return divide(leftValue, rightValue); |
There was a problem hiding this comment.
저는 한줄이더라도 curly brace를 활용하는 편입니다. 추후에 statements 구분이 혼동되어 실수할 요소를 줄이기 위함입니다.
https://myeonguni.tistory.com/1596 자바 컨벤션에도 7.4의 항목으로 다루고 있습니다.
| if("*".equals(operatorType)) return multiple(leftValue, rightValue); | ||
| if("/".equals(operatorType)) return divide(leftValue, rightValue); | ||
|
|
||
| return -1; //TODO: 질문입니다! 여기서 무엇을 반환해야 할 지 모르겠습니다. |
There was a problem hiding this comment.
위의 기능을 enum을 활용하면 이 부분은 해결될 거라고 생각합니다.
하지만 그렇지 않을 경우엔 예외를 의미하기에 Exception을 발생시키거나, 상황에 따라서는 빈객체를 리턴하는 것도 좋다고 생각합니다.
flag 사용은 지양하고 있습니다.
| validator.stringValidation(inputString); //TODO: 처음에는 이 부분을 runCalculatorApplication 메서드에 두었었는데요, 공백과 null을 체크하는 이 메서드를 어디서 실행하는게 좋을까요? | ||
| String[] separatedElements = inputString.split(" "); | ||
| int result = calculateElements(separatedElements); | ||
| return String.valueOf(result); //TODO: 이렇게 반환하는 것이 올바른 방법인지 의문입니다. |
There was a problem hiding this comment.
String 리턴값이 필요한 경우라면 이렇게 반환하는 것이 문제되진 않다고 생각합니다.
다만 이 경우에 리턴 받은 이후 출력 외의 별다른 기능을 하지 않기에 그냥 int로 리턴해도 된다고 생각합니다.
println의 경우 실제 구현체가 하단과 같기 때문에 int로 넣어도 String.valueOf 과정을 거치게 됩니다 .
public void println(int x) {
synchronized (this) {
print(x);
...
public void print(int s) {
write(String.valueOf(s));
}그리고 "연산 결과 : " 문자열과 합치는 경우에 autoboxing을 거쳐 String으로 변환됩니다.
아낌없는 지적과 조언 부탁드립니닷! :)