feat: step1 요구사항 구현 #4010
feat: step1 요구사항 구현 #4010
Conversation
| @DisplayName("덧셈 테스트") | ||
| @Test | ||
| void addTest() { | ||
| int result = Calculator.run("2 + 3"); | ||
| assertEquals(5, result); | ||
| } | ||
|
|
||
| @DisplayName("뺄셈 테스트") | ||
| @Test | ||
| void subtractTest() { | ||
| int result = Calculator.run("5 - 3"); | ||
| assertEquals(2, result); | ||
| } | ||
|
|
||
| @DisplayName("곱셈 테스트") | ||
| @Test | ||
| void multiplyTest() { | ||
| int result = Calculator.run("2 * 3"); | ||
| assertEquals(6, result); | ||
| } | ||
|
|
||
| @DisplayName("나눗셈 테스트") | ||
| @Test | ||
| void divideTest() { | ||
| int result = Calculator.run("6 / 3"); | ||
| assertEquals(2, result); | ||
| } | ||
|
|
||
| @DisplayName("입력 값이 null이거나 빈 공백 문자일 경우") | ||
| @ParameterizedTest | ||
| @ValueSource(strings = {"", " "}) | ||
| void nullOrEmptyTest() { | ||
| assertThatIllegalArgumentException().isThrownBy(() -> { | ||
| Calculator.run(null); | ||
| }).withMessage("입력 값이 null이거나 빈 공백 문자입니다."); | ||
| } | ||
|
|
||
| @DisplayName("지원하지 않는 연산자일 경우") | ||
| @Test | ||
| void invalidOperatorTest() { | ||
| assertThatIllegalArgumentException().isThrownBy(() -> { | ||
| Calculator.run("2 % 3"); | ||
| }).withMessage("지원하지 않는 연산자입니다."); | ||
| } |
There was a problem hiding this comment.
👍
연산자가 여러개 들어간 수식에 대한 케이스도 있었다면 더 좋았겠네요
| } | ||
|
|
||
| public static int run(String expression) { | ||
| validate_input(expression); |
There was a problem hiding this comment.
자바 컨벤션을 지켜서 스네이크 케이스 대신 카멜 케이스로 바꿔보면 어떨까요?
어떤 부분은 카멜 케이스이고 이 부분은 스네이크 케이스네요!
| String t = tokens[i]; | ||
| if (i % 2 == 0) { | ||
| int number = Integer.parseInt(t); | ||
| result = updateResult(result, operand, number); | ||
| } |
There was a problem hiding this comment.
indent가 두개가 되었는데,
여기서 어떻게 줄여야 가독성을 떨어트리지 않으면서 메소드를 분리할 수 있을지 잘 모르겠습니다.
한 반복에서 모든 것을 처리하려고 하면 depth가 깊어질 질 수 밖에 없습니다.
작은 문제를 하나씩 차근차근 풀다보면 쉽게 구현할 수 있습니다.
- 수식이 올바른지 검증한다. (토큰 개수 검증, 연산자, 피연산자 위치 검증)
- 앞서 수식이 올바른지 체크했으므로 하나씩 꺼내 연산을 처리합니다.
// 수도 코드
var result = tokens.get(0);
조건 loop {
var 피연산자 = tokens.get(i);
var 연산자 = tokens.get(i + 1);
var result += 연산처리
}이런 모양으로 구현을 한다면 가독성 좋게 풀어나갈 수 있지 않을까요?
|
|
||
| String[] tokens = expression.split(" "); | ||
| for (int i = 0; i < tokens.length; i++) { | ||
| String t = tokens[i]; |
There was a problem hiding this comment.
t 라는 변수를 봤을 때 이게 무엇인지 바로 파악하기는 어려워보입니다.
의미있는 이름으로 변경해보면 좋겠습니다!
| int operate(int num1, int num2) { | ||
| if (VALUE.equals("+")) { | ||
| return sum(num1, num2); | ||
| } | ||
| else if (VALUE.equals("-")) { | ||
| return subtract(num1, num2); | ||
| } | ||
| else if (VALUE.equals("*")) { | ||
| return multiply(num1, num2); | ||
| } | ||
| else if (VALUE.equals("/")) { | ||
| return divide(num1, num2); | ||
| } | ||
| else { | ||
| throw new IllegalArgumentException("지원하지 않는 연산자입니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
else를 사용하지 마라.
연산자가 많아진다면 계속해서 코드가 늘어나는 구조겠네요
https://oblivionsb.tistory.com/12
블로그 참고해 보시고 enum 또는 인터페이스 활용으로 풀어나가는 방법도 있구나 정도만 참고해 주시면 좋겠네요 😄
| @@ -0,0 +1,42 @@ | |||
| class Operand { | |||
There was a problem hiding this comment.
Operand는 연산에 필요한 값을 의미하므로
Operation 같은 이름이 더 어울릴 듯 합니다!
| @ParameterizedTest | ||
| @ValueSource(strings = {"", " "}) | ||
| void nullOrEmptyTest() { |
There was a problem hiding this comment.
ParameterizedTest로 여러 값을 반복 처리하지만
테스트 케이스에 입력값으로는 사용되지 않네요!
nullOrEmpty 테스트이지만 null 테스트만 검증하고 있네요 😭
test 코드와 요구사항 구현했습니다.
Calculator.run을 구현하다 보니 indent가 두개가 되었는데,
여기서 어떻게 줄여야 가독성을 떨어트리지 않으면서 메소드를 분리할 수 있을지 잘 모르겠습니다.