Conversation
Merge feature/java-calculator
refactor: delete unused class and fix naming
feat: add TDD basic format and test case for calculator class
feat: add comment and refactor
…invalid feat: add invalid test case
chae-heechan
left a comment
chae-heechan
left a comment
There was a problem hiding this comment.
미션 수행하시느라 고생하셨습니다!👍👍
항상 혼자 작성한 코드만 보면서 작성하다 보니 코드를 짜는 방식이 한정적이였는데 작성하신 코드를 보니 시야가 트인 느낌입니다!
리뷰를 하다보니 모르는 부분을 찾으면서도 공부가 되는것 같아 좋은것 같아요. 고생하셨습니다!
| interface Stack{ | ||
| boolean isEmpty(); | ||
| String pop(); | ||
| void clear(); | ||
| } |
There was a problem hiding this comment.
저는 인터페이스는 여러 클래스가 동일한 기능을 하도록 기준을 잡아주는 역할을 하는것으로 알고있는데 이 구조를 사용하는 클래스는 ArithmeticExpressionStack 클래스 하나인데 인터페이스를 사용하신 이유가 무엇인가요?
There was a problem hiding this comment.
이후 코드 확장성 측면을 생각하여 인터페이스를 사용했습니다.
| AddOperation addOperation = new AddOperation(); | ||
| SubOperation subOperation = new SubOperation(); | ||
| MultiplyOperation multiplyOperation = new MultiplyOperation(); | ||
| DivideOperation divideOperation = new DivideOperation(); | ||
| int result = 0; | ||
|
|
||
| // When the operator is equal to "+" | ||
| if (operator.equals(addOperation.operationName())){ | ||
| result = addOperation.calculation(firstNumber, SecondNumber); | ||
| } | ||
|
|
||
| // When the operator is equal to "-" | ||
| else if (operator.equals(subOperation.operationName())){ | ||
| result = subOperation.calculation(firstNumber, SecondNumber); | ||
| } | ||
|
|
||
| // When the operator is equal to "*" | ||
| else if (operator.equals(multiplyOperation.operationName())){ | ||
| result = multiplyOperation.calculation(firstNumber, SecondNumber); | ||
| } | ||
|
|
||
| // When the operator is equal to "/" | ||
| else if (operator.equals(divideOperation.operationName())){ | ||
| result = divideOperation.calculation(firstNumber, SecondNumber); | ||
| } | ||
|
|
||
| // Raise error when a symbol that does not correspond to the arithmetic operations(+, -, *, /) comes | ||
| else{ | ||
| message.exceptionResult("NOT_OPERATOR"); | ||
| } |
There was a problem hiding this comment.
저는 이 부분을 하나의 클래스에서 switch/case문을 사용해서 동작하게 했는데 비슷한 동작을 각자 클래스로 만들어 따로 넣어주신 이유가 궁금합니다!
There was a problem hiding this comment.
위와 같이 이후 확장성을 생각하여 우선순위를 부여하기 위해서 클래스를 따로 만들었습니다.
begaonnuri
left a comment
begaonnuri
left a comment
There was a problem hiding this comment.
잘 구현해주셨네요 👍🏻
몇가지 변경사항 남겼으니 반영해주시고 궁금한것들 질문해주세요
| - Calculator class | ||
| - `+ class` | ||
| - `- class` | ||
| - `* class` | ||
| - `/ class` | ||
| - Input class | ||
| - Output class | ||
| - `Error message` output | ||
| - `Calculation result` output | ||
| - Error exception class | ||
| - Precautions : only use `try ~ catch` | ||
| - Init class | ||
| - When an error occurs, reset after outputting an error message | ||
| - Reset and input when pressing any key value |
|
|
||
| @Test | ||
| void addChooseOperatorAndCalculate(){ | ||
| Calculator calculator = new Calculator(); |
There was a problem hiding this comment.
setUp 메소드에서 매번 객체를 생성해주는데 또 생성해주고있네요.
이 부분을 멤버변수로 빼서 재사용하는건 어떨까요?
| int secondNumber = 3; | ||
| int result = 0; | ||
| result = calculator.chooseOperatorAndCalculate(firstNumber, operator, secondNumber); | ||
| assertEquals(5, result); |
There was a problem hiding this comment.
junit 메소드(assertEquals)보다 assertJ의 메소드(assertThat)를 더 많이 활용해요.
assertJ에 익숙해지는걸 추천드려요
| @Test | ||
| void invalidCalculation(){ | ||
| // todo refactoring | ||
| assertThrows(NumberFormatException.class, new Executable() { |
There was a problem hiding this comment.
마찬가지로 assertThatThrownBy()를 사용해보세요.
| @Override | ||
| public void execute() throws Throwable { | ||
| Calculator calculator = new Calculator(); | ||
|
|
||
| String[] equation_list = {"+", "1", "+"}; | ||
| ArithmeticExpressionStack arithmeticExpressionStack = new ArithmeticExpressionStack(equation_list, equation_list.length); | ||
|
|
||
| calculator.OperatorSetting(arithmeticExpressionStack); | ||
| } |
There was a problem hiding this comment.
이렇게 익명 클래스의 함수는 람다 함수를 이용해서 깔끔하게 처리할 수 있답니다.
람다 함수에 대해서도 공부해보세요.
| package calculator; | ||
|
|
||
|
|
||
| interface Stack{ |
There was a problem hiding this comment.
스택, 큐 같은 자료구조는 대부분의 언어에 기본적으로 내장되어있습니다.
내장된 API를 사용하지 않고 구현하는 것은 오히려 비효율을 유발해요.
협업을 할 때 내장된 API는 자바 개발자들이 학습하지만 이 경우 현수님이 짠 코드를 추가로 분석해야하기 때문이에요.
자바 컬렉션의 Stack을 사용하는 식으로 변경해보세요
| @@ -0,0 +1,14 @@ | |||
| package calculator; | |||
|
|
|||
| interface OperationInterface { | |||
There was a problem hiding this comment.
인터페이스 활용 👍🏻
하지만 클래스와 인터페이스, 자료형 이름에 예약어가 들어가는 것은 권장되지 않아요. 타입으로도 충분히 판단될 수 있기 때문이에요.
aList = new ArrayList<>(); 보다는 a = new ArrayList<>(); 처럼 이름을 Operation으로 변경해주세요
| @Override | ||
| public int operationPriority() { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
사용하지 않은 메소드를 굳이 만든 이유가 있을까요?
추후 연산자의 우선순위가 변경될거라고 생각해서 만드신건가요?
| public int operationPriority(); | ||
|
|
||
| public String operationName(); | ||
|
|
||
| public int calculation(int beforeNumber, int afterNumber); |
There was a problem hiding this comment.
인터페이스의 경우 기본적으로 모두 public이기 때문에 접근제어자를 생략해도 된답니다.
|
|
||
| // Raise error when a symbol that does not correspond to the arithmetic operations(+, -, *, /) comes | ||
| else{ | ||
| message.exceptionResult("NOT_OPERATOR"); |
There was a problem hiding this comment.
상황에 맞게 메시지를 출력해주셨는데, 추가적인 처리가 필요한 경우 해당 에러라고 어떻게 판단할 수 있을까요?
메시지를 통해 판단하는 것은 비효율적이고 메시지를 분석하는 로직이 추가될거에요.
예외를 발생시켜서 처리하는것으로 변경해보는건 어떨까요?
4 participants
No description provided.