계산기 구현 #1

Jummi10 · 2021-02-25T16:09:33Z

No description provided.

- 문자열이 숫자로 시작하지 않을 경우 - 연산자나 숫자가 두 번 이상 연속으로 나올 경우

- Operation 예외처리 구현 - Operation 테스트 구현

Merge feature/java-calculator

pkalsh

미션 진행하느라 수고 많으셨고 제가 남긴 리뷰에 의문이 드는 점이 있으시다면 언제든 질문해주시면 감사하겠습니다 !

pkalsh · 2021-02-27T11:29:49Z

src/main/java/calculator/Calculator.java

+        return result;
+    }
+
+    public double fourFundamentalArithmeticOperations(double tempResult, char operator, double operand) {


메서드 이름:
핵데이 자바 컨벤션 2.10에 메서드 이름은 동사나 전치사로 시작할 것을 권장하고 있네요!
동사로 시작하는 다른 이름을 고려해보는게 어떤가요?

Suggested change

public double fourFundamentalArithmeticOperations(double tempResult, char operator, double operand) {

public double calculateExpression(double tempResult, char operator, double operand) {

pkalsh · 2021-02-27T11:37:10Z

src/main/java/calculator/Calculator.java

+        double result = operands.get(0);
+        int numOfOperations = operators.size();
+        int times = 0;
+
+        while (times < numOfOperations) {
+            result = fourFundamentalArithmeticOperations(result, operators.get(times), operands.get(++times));
+        }


times라는 변수명:
애플리케이션의 중요한 로직을 처리하는 부분인데 개인적으로 times라는 변수명은 그 목적이 불분명한 것 같아요.
operatorPosition와 같은 변수명이면 좋을 것 같아요 😃
마찬가지로 operands 리스트에서 요소를 가져오는 ++times도 rightOperandPosition와 같은 변수명은 어떨까요?

Suggested change

double result = operands.get(0);

int numOfOperations = operators.size();

int times = 0;

while (times < numOfOperations) {

result = fourFundamentalArithmeticOperations(result, operators.get(times), operands.get(++times));

}

double result = operands.get(0);

int numOfOperations = operators.size();

int operatorPosition = 0;

while (operatorPosition < numOfOperations) {

int rightOperandPosition = operatorPosition + 1;

result = calculateExpression(result, operators.get(operatorPosition), operands.get(rightOperandPosition));

operatorPosition++;

}

pkalsh · 2021-02-27T11:50:58Z

src/main/java/calculator/Calculator.java

+    public double fourFundamentalArithmeticOperations(double tempResult, char operator, double operand) {
+        double result;
+
+        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;
+        }
+
+        return result;
+    }


switch 문을 활용해 결과를 깔끔하게 처리하셨군요! 👍
하지만 return 문을 활용하면 break 삭제할 수 있어서 코드 길이를 줄일 수 있을 것 같아요.

Suggested change

public double fourFundamentalArithmeticOperations(double tempResult, char operator, double operand) {

double result;

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;

}

return result;

}

public calculateExpression(double tempResult, char operator, double operand) {

switch (operator) {

case '+':

return tempResult + operand;

case '-':

return tempResult - operand;

case '*':

return tempResult * operand;

case '/':

return tempResult / operand;

}

return 0;

}

한 가지 덧붙이자면 보통 switch 문은 함수의 길이를 지나치게 길게 만들기 때문에 기피하고는 한답니다. (클린코드, 47p)
책의 내용을 참고해서 해석하자면 함수의 길이가 길다는 의미는
'더하기', '빼기', '곱하기', '나누기' 등의 여러 작업을 동일 함수 내에서 진행하고 있고 이는 곧 코드를 변경할 이유가 여러 가지라고 합니다.

하지만 switch 문 자체를 완전히 배제할 수는 없기 때문에 책에서 제안하는 방법은 다형성을 활용해서 switch 문을 풀어내는 것입니다. 자세한 내용은 책을 참조하시면 좋을 것 같습니다!

pkalsh · 2021-02-27T12:06:03Z

src/main/java/calculator/Operation.java

+
+    public void validateOtherSymbols() {
+        for (String tempStr : operation) {
+            if (!(tempStr.matches("^[0-9]*$") || tempStr.matches("^\\-[1-9]\\d*$") || tempStr.matches("[+*/-]")))


"[]"로 묶어서 사칙 연산 검증을 할 수도 있군요. 더 깔끔해보입니다 👍

하지만 정규표현식이 Operation 클래스의 여러 메서드에 반복적으로 등장하는 만큼 이름에 의미가 부여된 상수나 메서드를 활용하시면 더 읽기 좋은 코드가 될 것 같습니다!

한 가지 궁금한게 있는데 숫자를 검증하는 정규 표현식이 *라면 비어있는 문자열도 통과시켜 버그가 나지 않나 조심스레 질문 남깁니다.

오호

+: 앞 글자가 1개 이상 존재함 ex) a+ : a라는 글자가 1개 이상이다.
*: 앞 글자가 0개 이상 존재함 ex) a * : a라는 글자가 없거나 반복된다.

이므로 * -> +로 바꾸어야겠네요!

pkalsh · 2021-02-27T12:11:12Z

src/main/java/calculator/Operation.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class Operation {


개인적으로 Operation 클래스의 책임이 너무 많다고 느껴집니다.

입력 문자열을 분리한다. (split)

분리한 문자열을 검증한다.(validate)

분리한 문자열을 따로 저장하고 외부에 의해 조회된다. (get~)

클래스를 추가하여 책임을 나누는 것이 어떨지 제안해봅니다 😄

kouz95

고생하셨습니다. 👍
전체적으로 구조가 깔끔하다는 느낌을 받았습니다. (특히 main문)

그런 깔끔함은 유지하면서 객체 분리를 좀 더 해보도록해요 🙂

kouz95 · 2021-02-28T05:36:52Z

src/main/java/Application.java

+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();
+
+        Operation operation = new Operation(input);
+        List<Integer> operands = operation.getOperands();
+        List<Character> operators = operation.getOperators();
+
+        Calculator calculator = new Calculator();
+        double result = calculator.calculate(operands, operators);
+        System.out.println("result = " + result);


main에서 프로그램의 흐름이 느껴지네요 👍
깔끔해서 보기 좋습니다.

kouz95 · 2021-02-28T05:48:40Z

src/main/java/calculator/Operation.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class Operation {


객체는 독자적인 데이터(상태)를 갖고, 갖고 있는 데이터를 이용하여 행동을 합니다.

Operation이 하는 행동은 무엇인가요?
input의 가공과 검증에서 그치고 객체의 상태를 그대로 다른 클래스에게 넘겨주는게 뭔가 아쉽다는 느낌이 드네요.

객체지향의 사실과 오해(2장) 정리
위 링크의 행동이 상태를 결정한다를 참고해보면 좋을 것 같아요 🙂

getter를 최소한으로 사용해보세요.

kouz95 · 2021-02-28T05:50:48Z

src/main/java/calculator/Operation.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class Operation {


모든 메소드가 public으로 정의되어 있네요. 괜찮을까요?

kouz95 · 2021-02-28T05:53:14Z

src/main/java/calculator/Calculator.java

+            default:
+                result = 0;
+                break;


+, -, *, / 가 아닌 문자의 경우 result를 0으로 만드는것에 문제는 없을까요?

kouz95 · 2021-02-28T05:59:20Z

src/main/java/calculator/Calculator.java

+    public double fourFundamentalArithmeticOperations(double tempResult, char operator, double operand) {
+        double result;
+
+        switch (operator) {


switch문은 동민님이 말했듯, 선호되지 않는 방법입니다.

enum을 활용하여 리팩토링 해보는것도 좋을것 같아요 🙂
Enum 활용사례 3가지

kouz95 · 2021-02-28T06:03:02Z

src/main/java/Application.java

+
+        Operation operation = new Operation(input);
+        List<Integer> operands = operation.getOperands();
+        List<Character> operators = operation.getOperators();


연산자의 클래스가 Character로 사용되는건 마음이 아프네요 😥

연산자 객체를 따로 만들수 있진 않을까요?

Character객체를 사용하는 것이 아닌, 직접 정의한 객체로 분리한다면 어떤 장점이 있을까요?

Character로 사용되는게 왜 마음이 아픈지 알 수 있을까요.. 직접 정의한 객체로 분리하라는 말은 first-class 컬렉션을 사용해보라는 말씀이신가요??

Character 클래스는 너무 광범위하지 않나요?
Charactor 클래스가 연산을 처리하기에 적절한 클래스일까요?

직접 정의한 클래스를 사용한다면 어떤 장점이 있을까요?

일급컬렉션을 사용해보라는 뜻은 아니었습니다. (물론 적용이 가능한 지점이긴 합니다 🙂)

Calculator가 Character 리스트를 확인하고 그에 맞는 연산을 하고 있습니다.
Calculator 객체의 책임이 버겁진 않나요? (단일 책임 원칙)
객체간의 협력을 통해 책임을 나누어 해결할 수 있지 않을까요?

kouz95 · 2021-02-28T06:03:57Z

src/main/java/calculator/Operation.java

+    private List<Character> operators;
+
+    public Operation(String input) {
+        this.operation = input.split(" ");


" " 와 같은 문자열은 상수로 포장하여 의미를 드러낼수 있을것 같아요

kouz95 · 2021-02-28T06:06:43Z

src/main/java/calculator/Operation.java

+        for (int i = 0; i < operation.length - 1; i++) {
+            if ((!operation[i].matches("[+*/-]") && !operation[i + 1].matches("[+*/-]"))
+                || (operation[i].matches("[+*/-]") && operation[i + 1].matches("[+*/-]"))) {
+                throw new IllegalArgumentException("기호나 숫자가 두 번 연속 입력되었습니다.");
+            }


regex의 활용은 좋습니다 👍
하지만 배열의 사용으로 인해 index기반으로 접근하게 되는것과 regex가 합쳐지니 가독성이 떨어지는것 같아요.
개선 시킬수 있을까요? 🤔

kouz95 · 2021-02-28T06:09:13Z

src/test/java/calculator/OperationTest.java

+
+    @BeforeEach
+    void setUp() {
+        operation = null;


null로 초기화 하는것은 setUp 메소드를 제대로 활용한 것 같진 않네요.

모든 테스트에서 매번 인스턴스를 초기화를 하고 있는 불편함이 해결되지 않은 느낌이에요.

Jummi10 added 7 commits February 24, 2021 20:56

docs: 기능 목록 작성

cde694a

feat: Calculator 구현

b8146b8

test: Calculator 테스트 케이스 구현

858f32d

feat: 문자열 입력받아 피연산자와 연산자로 나눔

8949088

feat: Operation 예외 처리

30cb7c4

- 문자열이 숫자로 시작하지 않을 경우 - 연산자나 숫자가 두 번 이상 연속으로 나올 경우

chore: 코드 병합

b6e8aab

- Operation 예외처리 구현 - Operation 테스트 구현

refactor: 마지막 줄 공백 추가

2049377

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

Merge pull request biforest#1 from oereo/feature/java-calculator

f53fdf9

Merge feature/java-calculator

pkalsh reviewed Feb 27, 2021

View reviewed changes

kouz95 requested changes Feb 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

계산기 구현 #1

계산기 구현 #1

Jummi10 commented Feb 25, 2021

pkalsh left a comment

pkalsh Feb 27, 2021

pkalsh Feb 27, 2021

pkalsh Feb 27, 2021

pkalsh Feb 27, 2021

Jummi10 Mar 7, 2021

pkalsh Feb 27, 2021

kouz95 left a comment

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

Jummi10 Mar 7, 2021

kouz95 Mar 7, 2021 •

edited

Loading

kouz95 Mar 7, 2021 •

edited

Loading

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

kouz95 Feb 28, 2021

	public double fourFundamentalArithmeticOperations(double tempResult, char operator, double operand) {
	public double calculateExpression(double tempResult, char operator, double operand) {

계산기 구현 #1

Are you sure you want to change the base?

계산기 구현 #1

Conversation

Jummi10 commented Feb 25, 2021

pkalsh 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

kouz95 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

kouz95 Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

kouz95 Mar 7, 2021 • edited Loading

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

kouz95 Mar 7, 2021 •

edited

Loading

kouz95 Mar 7, 2021 •

edited

Loading