문자열 계산기 구현했습니다! #2

chae-heechan · 2021-02-25T16:15:44Z

No description provided.

refactor: delete unused class and fix naming

jjanggyu · 2021-02-28T01:17:34Z

src/main/java/Calculator.java

+            case '/':
+                result = tempResult / operand;
+                break;


0으로 나눌 때의 에러도 함께 있으면 좋을거 같아요!

jjanggyu · 2021-02-28T01:22:13Z

src/test/java/CalculatorTest.java

+    @Test
+    void 사칙연산() {
+        double addition, subtraction, multiplication, division, wrongOperatorResult;
+
+        addition = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(0), operands.get(1));
+        subtraction = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(1),
+                operands.get(1));
+        multiplication = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(2),
+                operands.get(1));
+        division = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(3), operands.get(1));
+        wrongOperatorResult = calculator.fourFundamentalArithmeticOperations(operands.get(0), '?', operands.get(1));
+
+        assertThat(addition).isEqualTo(6.0);
+        assertThat(subtraction).isEqualTo(-2.0);
+        assertThat(multiplication).isEqualTo(8.0);
+        assertThat(division).isEqualTo(0.5);
+        assertThat(wrongOperatorResult).isEqualTo(0);
+    }


더하기, 빼기, 곱하기, 나누기 등 기능 별로 따로 테스트를 만드는 건 어떨까요!!
예를 들어 이 테스트에서 뺄셈에서만 작동하지 않을 경우에도 이 테스트 전체에서 작동하지 않는다고 나올거 같아서요!

동의합니다!
단위 테스트이기 때문에 최대한 작은 단위를 테스트 해주시는것이 좋아요.
또한 테스트도 유지보수의 대상이기 때문에 작게 유지할 수록 좋답니다.

jjanggyu

문자열 계산기 리뷰 입니다! 정말 잘 짜셨네요 수고하셨습니다

begaonnuri

잘 구현해 주셨네요! 👍🏻
몇가지 변경사항 남겨놨으니 반영해주세요!

begaonnuri · 2021-02-28T11:05:44Z

README.md

+### 기능
+- 예외
+    - [x] 숫자로 시작하지 않을 경우
+    - [x] 띄어쓰기가 잘 안 돼서 숫자와 문자가 섞였을 경우(공백문자 미사용)
+    - [x] 숫자와 연산자 이외의 문자
+    - [x] 연산자나 숫자가 두 번 이상 연속으로 나올 경우
+- 순서
+    - [x] 문자열 입력 받기
+    - [x] 띄어쓰기로 분리해서 배열에 넣기
+        - [x] 예외 처리
+    - [x] 피연산자(operand 숫자)와 연산자(operator +-/*)로 나누기
+    - [x] 계산하기


예외 케이스까지 잘 적어주셨네요 👍🏻

begaonnuri · 2021-02-28T11:09:28Z

src/main/java/Application.java

+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();


비즈니스 로직과 뷰 로직의 분리 👍🏻
근데 애플리케이션이 너무 불친절한것같아요!
안내메시지 없이 밑도끝도 없이 숫자를 입력하라그러네요 😂

begaonnuri · 2021-02-28T11:10:22Z

src/main/java/Application.java

+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();


Application의 역할은 무엇일까요?
그리고 입력해야 하는 메소드와 출력해야 하는 메소드가 많아질 경우 어떻게 해야할까요?

begaonnuri · 2021-02-28T11:17:10Z

src/main/java/Operation.java

+    public Operation(String input) {
+        this.operation = input.split(" ");
+        validateOperation();
+        splitOperation();
+    }


Operation�의 역할이 너무 많은것같아요.
Operation이 지금 어떤 행위들을 하고있나요?
SRP에 대해 학습해보세요.

begaonnuri · 2021-02-28T11:19:14Z

src/main/java/Operation.java

+import java.util.stream.Stream;
+
+public class Operation {
+    private final String[] operation;


쪼갠 스트링 배열을 굳이 상태(필드)로 갖고 있을 필요가 있을까요?
클래스의 필드가 많아지면 무엇이 단점일까요?

begaonnuri · 2021-02-28T11:22:25Z

src/main/java/Operation.java

+            if ((!operation[i].matches("[+*/-]") && !operation[i + 1].matches("[+*/-]"))
+                    || (operation[i].matches("[+*/-]") && operation[i + 1].matches("[+*/-]"))) {


처음 투입한 프로젝트에서 이 부분만 보고 무엇을 의미하는지 알 수 있을까요?
boolean을 리턴하는 메소드로 분리하는건 어떨까요?

begaonnuri · 2021-02-28T11:25:21Z

src/main/java/Operation.java

+        operands = Arrays.stream(this.operation)
+                .filter(operand -> operand.matches("^[0-9]*$") || operand.matches("^\\-[1-9]\\d*$"))
+                .map(Integer::parseInt)
+                .collect(Collectors.toList());
+
+        operators = Arrays.stream(this.operation)
+                .filter(operator -> operator.matches("[+*/-]"))
+                .map(operator -> operator.charAt(0))
+                .collect(Collectors.toList());


Operation에서 operands와 operators를 모두 처리하려다 보니 Operation의 책임도 커지고 로직도 길어지는 경향이 생기는 것 같아요.
일급 컬렉션을 사용해 보는건 어떨까요?
이 부분은 너무 어렵게 느껴지시면 반영 안하셔도 됩니다.

begaonnuri · 2021-02-28T11:31:02Z

src/main/java/Calculator.java

+        switch (operator) {
+            case '+':
+                result = tempResult + operand;
+                break;
+            case '-':
+                result = tempResult - operand;
+                break;
+            case '*':
+                result = tempResult * operand;
+                break;
+            case '/':
+                result = tempResult / operand;
+                break;
+            default:
+                result = 0;
+                break;


계산기에서 지원하는 연산자가 증가할때마다 이 switch문이 비례해서 증가할 것 같아요.
어떻게 하면 이것을 해결할 수 있을까요?
인터페이스와 다형성을 학습해보세요.

begaonnuri · 2021-02-28T11:32:59Z

src/test/java/CalculatorTest.java

+    @Test
+    void 사칙연산() {
+        double addition, subtraction, multiplication, division, wrongOperatorResult;
+
+        addition = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(0), operands.get(1));
+        subtraction = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(1),
+                operands.get(1));
+        multiplication = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(2),
+                operands.get(1));
+        division = calculator.fourFundamentalArithmeticOperations(operands.get(0), operators.get(3), operands.get(1));
+        wrongOperatorResult = calculator.fourFundamentalArithmeticOperations(operands.get(0), '?', operands.get(1));
+
+        assertThat(addition).isEqualTo(6.0);
+        assertThat(subtraction).isEqualTo(-2.0);
+        assertThat(multiplication).isEqualTo(8.0);
+        assertThat(division).isEqualTo(0.5);
+        assertThat(wrongOperatorResult).isEqualTo(0);
+    }


동의합니다!
단위 테스트이기 때문에 최대한 작은 단위를 테스트 해주시는것이 좋아요.
또한 테스트도 유지보수의 대상이기 때문에 작게 유지할 수록 좋답니다.

begaonnuri · 2021-02-28T11:35:09Z

src/test/java/OperationTest.java

+        assertThatThrownBy(() -> operation = new Operation(wrongInput))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("한 String에 숫자와 연산자가 함께 있거나, ");


assertJ의 다양한 메소드를 잘 활용해주셨네요!
현재 모든 예외를 IllegalArgumentException으로 처리하고있어서, 메시지를 통해 확인한 점도 좋습니다 👍🏻

chae-heechan added 6 commits February 26, 2021 00:57

docs: 구현할 기능 목록 추가

dc23b10

feat: 수식 계산 기능 추가

80a7b87

feat: 입력 예외처리 및 수식 분리 기능 추가

f52f6f2

test: 수식 계산 기능 테스트

1cbf903

test: 입력 예외처리 및 수식 분리 기능 테스트

9cab3c5

feat: 계산기 작동 기능 추가

e50cdb8

songpang pushed a commit to songpang/java-calculator that referenced this pull request Feb 25, 2021

Merge pull request biforest#2 from oereo/fix/java-calculator

b6d6340

refactor: delete unused class and fix naming

begaonnuri self-requested a review February 26, 2021 01:53

jjanggyu reviewed Feb 28, 2021

View reviewed changes

begaonnuri requested changes Feb 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

문자열 계산기 구현했습니다! #2

문자열 계산기 구현했습니다! #2

chae-heechan commented Feb 25, 2021

jjanggyu Feb 28, 2021

jjanggyu Feb 28, 2021

begaonnuri Feb 28, 2021

jjanggyu left a comment

begaonnuri left a comment

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

begaonnuri Feb 28, 2021

		Scanner scanner = new Scanner(System.in);
		String input = scanner.nextLine();

		if ((!operation[i].matches("[+/-]") && !operation[i + 1].matches("[+/-]"))
		\|\| (operation[i].matches("[+/-]") && operation[i + 1].matches("[+/-]"))) {

문자열 계산기 구현했습니다! #2

Are you sure you want to change the base?

문자열 계산기 구현했습니다! #2

Conversation

chae-heechan commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjanggyu left a comment

Choose a reason for hiding this comment

begaonnuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment