[4기 - 신예진] 1~2주차 과제: 계산기 구현 미션 제출합니다. #194

born-A · 2023-06-15T19:02:41Z

📌 과제 설명

계산기를 TDD, OOP로 구현

👩‍💻 요구 사항과 구현 내용

추상화시킨 입력,출력을 구현한 콘솔을 통해 사용자에게 입출력 기능을 제공합니다
사용자로부터 입력받은 숫자(1,2,3)에 따라 조회, 계산, 종료 기능을 제공합니다.

적합한 입력 값(1,2,3) 이 아닌 경우에 사용자에게 오류 메시지를 출력합니다.

계산 기능을 수행 시, 입력식의 포맷에 맞지 않는 경우와 0으로 나눌 경우를 예외처리 해주어 사용자에게 오류 메시지를 제공합니다.
조회할 시, 저장,조회 기능을 가진 추상화한 �MemoryRepository 저장소를 구현한 CalMemoryRepository를 이용합니다.
enum을 통해 메뉴 옵션, 사칙 연산을 정의하였습니다.
확장성을 위해, AbstractCalculator, Converter를 인터페이스로 만들어 구현체를 사용했습니다.
Calculator는 실질적으로 계산을 하여 결과만을 제공하는 로직만을 가져, SRP를 충족시키도록 하였습니다.
CalculatorManager는 사용자에게 메뉴 옵션 입력 부터, 검증, 후위표기식 변환, 조회와 같은 전반적인 기능을 제공합니다.

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

객체 지향적으로 코드를 짤 때, 기능을 담당하는 단위에 대해서 감이 잘 안잡혀 추상화의 수준과 클래스를 세분화 하는데에 고민이 많이 되었습니다.
1차 리뷰시에 멘토님께서 logic, behavior, data를 잘 구분해보라 하셨는데 각각을 구분하는데에 어려움이 있었습니다.
예외처리

저는 예외처리를 CalculatorManager가 구동을 시작하는 while문 안에서 모든 예외를 RuntimeException으로 받아 해당 예외들의 메시지를 띄우는 방식으로 사용자에게 에러를 표시하고자 하였는데 효율적인 예외 처리에 대해 궁금합니다.
예외 처리 테스트 코드의 경우 저는 단순히 해당 예외 메시지가 같은지를 비교했는데, 어떻게 코드 작성을 하는게 좋은 지 궁금합니다.

Calculator를 계산만 하는 객체로 두고 나머지 기능들은 CalculatorManager가 담당하는데, 이 경우 CalculatorManager 또한 세분화 시키는게 맞을지 궁급합니다.

항상 세심하게 코드 리뷰 해주셔서 감사합니다!!

[ADD] CalMemoryRepository save() test

devYSK · 2023-06-16T06:06:59Z

src/main/java/com/bona/javacalculator/core/calculator/AbstractCalculator.java

+
+import java.util.List;
+
+public interface AbstractCalculator {


인터페이스를 사용하신 예진님의 생각이 궁금합니다!

일반적으로 abstract class에 prefix로 Abstract라고 붙이는 경우가 종종 있어요.

예진님의 코드를 누군가 본다면 어? Abstract인데 interface네? 라고 어색함을 느낄수도 있을것 같아요! 다른 좋은 방법은 없을까요?

아 그렇네요!! 이 경우에는 클래스명이 Abstract이기에 abstract로 바꾸는게 맞는거 같습니다!

devYSK · 2023-06-16T06:07:58Z

src/main/java/com/bona/javacalculator/core/calculator/AbstractCalculator.java

+
+public interface AbstractCalculator {
+
+    double calculate(List<String> formula);


메소드의 파라미터랑 반환타입이, double이나 List<String>이 아닌 어떤 객체 이면 어떤 장점이 있을까요?

한정된 파라미터와 반환값이 아니라 여러 타입의 파라미터와 반환 타입을 처리할 수 있어서 매순간마다 불필요하게 doulbe값으로 바인딩하는 경우가 없어지는 장점이 있습니다! 객체로 바꿔야겠습니다!!

devYSK · 2023-06-16T06:15:52Z

src/main/java/com/bona/javacalculator/core/calculator/Calculator.java

+
+        Stack<Double> numbers = new Stack<>();
+
+        String split;


split을 꼭 for문 밖에 이렇게 초기화 없이 선언해서 사용해야 할까요?

foreach문을 사용해서 리팩토링 할 수도 있을것 같아요.

for (String element : formula) { if (Validator.isNumber(element)) { double value = Double.valueOf(element); numbers.push(value); } else { Operand operand2 = operands.pop(); Operand operand1 = operands.pop(); double result = Operator.calculate(element, operand1, operand2); numbers.push(double); } }

primitive 타입이나 Wrapper 클래스를 다음과 같이 래핑해서 사용하면 어떤 장점이 있을까요?

class Operand { private double value; public Operand(double value) { validate(value) this.value = value; } public double getValue() { return value; } } Stack<Operand> operands = new Stack<>();

for each문을 사용해서 리팩토링 해야겠습니다!

Wrapper클래스로 래핑을 하게되면 객체로 취급하게 되어 보다 객체지향적인 프로그래밍이 가능해서 좋은거 같습니다..!

devYSK · 2023-06-16T06:17:25Z

src/main/java/com/bona/javacalculator/core/converter/PostfixConverter.java

+import java.util.Stack;
+
+public class PostfixConverter implements Converter {
+    private final String WHITE_SPACE = "";


일반적으로 static final 타입이 대문자와 언더스코어(_)를 사용하여 이름을 지정하는 것이 관례 입니다.

네넵 수정하겠습니다!

devYSK · 2023-06-16T06:18:10Z

src/main/java/com/bona/javacalculator/core/converter/PostfixConverter.java

+        }
+        while (!stack.isEmpty()) {
+            postfix.add(stack.pop());
+        }
+        return postfix;


괄호가 끝나고 한칸씩 개행해주시는 것이 가독성에 더 좋은 경우가 잦습니다!
서로 보기 편한 코드가 좋지 않을까요!

devYSK · 2023-06-16T06:18:59Z

src/main/java/com/bona/javacalculator/core/converter/PostfixConverter.java

+    public void pushOperator(String operator, List<String> postfix, Stack<String> operatorStack) {
+        if (operatorStack.isEmpty() || Priority.compareOperation(operatorStack.peek(), operator) > 0) {
+            operatorStack.push(operator);
+            return;
+        }
+        while (!operatorStack.isEmpty() && Priority.compareOperation(operatorStack.peek(), operator) <= 0) {
+            postfix.add(operatorStack.pop());
+        }
+        operatorStack.push(operator);
+    }
+
+
+    public void pushNumber(String number, List<String> postfix) {
+        if (!Validator.isNumber(number)) {


이 두 메소드는 외부에서도 사용하나요 ?

아니요.. private으로 막아두어야겠습니다.. 접근제한자를 설정할때 고민을 많이 하지 않고 public으로 지정하는 경우가 많은거 같습니다. 접근제한자에 대해 더 고민을 해보겠습니다!

devYSK · 2023-06-16T06:20:13Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+        boolean isRun = true;
+
+        while (isRun) {


조금더 명확한 이름을 사용하는것이 가독성을 향상시킬 수 있어요

예를들면, 현재 실행중인지를 의미하는 isRunning 이 더 명확하지요

아하 네넵!!

devYSK · 2023-06-16T06:20:36Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+
+        while (isRun) {
+            try {
+                String input = console.input("1. 조회 2. 계산 3.종료");


메시지에 공백이 안맞는것 같아요!

넵 공백 처리 깔끔하게 해두겠습니다!!

devYSK · 2023-06-16T06:20:49Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+                    case INQUIRY -> inquiry();
+                    case CALCULATE -> calculate();
+                    case EXIT -> isRun = false;
+                    default -> {


default엔 무엇이 들어가야 할까요?

devYSK · 2023-06-16T06:23:13Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+    public void calculate() {
+        String formula = console.input("식을 입력해주세요 : ");
+
+        validator.validateFormula(formula);
+
+        List<String> postfix = converter.convert(formula);
+        double result = calculator.calculate(postfix);
+
+        repository.save(new ExpressionResult(formula, result));
+
+        console.outAnswer(result);
+    }
+
+
+    public void inquiry() {
+        List<ExpressionResult> all = repository.findAll();
+
+        if (all.isEmpty()) {
+            console.printMessage("조회 결과 기록이 존재하지 않습니다");
+            return;
+        }
+
+        console.printAll(all);


외부에서 사용하지 않는 메소드는 어떻게 관리하면 좋을까요!

public으로 선언되어있지만 사실은 Manager 내부에서만 사용한다면 가시성을 private 으로 감춰두면 어떨까요?

메소드 캡슐화!

조회하는 로직
조회하는 행위자가 마땅하지 않으니 조회로직만 있을듯 합니다.

영수님은 public으로 열어놔서 말씀주신것 같아요.

아하 네네!! 캡슐화 기억하겠습니다!!

devYSK · 2023-06-16T06:24:07Z

src/main/java/com/bona/javacalculator/core/Operator.java

+    private static Operator getOperator(String operator) {
+        return Arrays.stream(values())
+                .filter(o -> o.operator.equals(operator))
+                .findFirst().orElseThrow(() -> new IllegalArgumentException("올바른 연산자가 아닙니다."));


메서드 체이닝할때는
. 마다 개행해주는것이 가독성에 좋습니다!

아하 네네!!

devYSK · 2023-06-16T06:24:44Z

src/main/java/com/bona/javacalculator/core/Option.java

+    public static Option of(String input) {
+
+        for (Option value : values()) {
+            if (value.matchNum == Integer.parseInt(input)) {
+                return value;
+            }
+        }
+
+        throw new RuntimeException("입력이 잘못되었습니다.");
+    }


Operator의 getOperator()와 비슷하지않나요?

앗 그렇네요.. 똑같은 기능을 하는 메소드인데 통일해야겠습니다!

devYSK · 2023-06-16T06:26:12Z

src/main/java/com/bona/javacalculator/core/Priority.java

+public class Priority {
+


Util성 클래스인거같아요.
final 키워드를 사용하고 생성자를 방어할 수 있을것같아요!

아무데서나 생성해서 쓸 필요가 없는거같은데요

new Priority(); new Priority(); new Priority(); new Priority();

다른분들 코드와 리뷰를 한번 보시면 좋을것같아요.

아하 네넵 코드 참고해서 리팩토링하겠습니다!

devYSK · 2023-06-16T06:27:02Z

src/main/java/com/bona/javacalculator/core/Priority.java

+    public static int getOperationPriority(String operator) {
+
+        switch (operator) {
+            case "*":
+            case "/":
+                return 2;
+            case "+":
+            case "-":
+                return 1;
+            default:
+                throw new IllegalStateException("유효하지 않은 연산자 입니다" + operator);
+        }
+    }


자기주도 리뷰때에도 나온 이야기지만, 중간에 다른 우선순위가 끼어든다면 2와 1사이는 매우 애매할 수 있어요
10, 1 처럼 조금 더 크게 벌려놓는것은 어떻게 생각하세요?

연산자 Enum 만들었는데 사용했으면 좋겠습니다!

넵 10과 1정도로 크게 벌려놓겠습니다!
연산자 enum을 사용해서 리팩토링 하겠습니다!

devYSK · 2023-06-16T06:28:43Z

src/main/java/com/bona/javacalculator/core/Priority.java

+        }
+    }
+
+    //연산자 우선순위 비교


주석을 달지 않고도 메서드 명과 코드만으로 무슨 일을 하는지 이해가 될 수 있으면 좋겠어요!
앞으로 점점 성장하여 더욱 잘하실거라 믿습니다!

넵 앞으로는 코드만으로도 이해가 가능하도록 작성할 수 있게 노력하겠습니다🥹!!

devYSK · 2023-06-16T06:28:53Z

src/main/java/com/bona/javacalculator/core/Priority.java

+        int operationPriorityNow = getOperationPriority(NowOperator);
+
+        return Integer.compare(operationPriorityNow, operationPriority);
+


오 이 개행은 지워도 될거같은데요?

devYSK · 2023-06-16T06:29:13Z

src/main/java/com/bona/javacalculator/io/Console.java

+
+

두칸식 띄운 이유가 있으실까요!

윗줄 코드를 지우면서 두칸씩 띄워졌습니다.. 라인 정리에 신경쓰도록 하겠습니다!

devYSK · 2023-06-16T06:29:30Z

src/main/java/com/bona/javacalculator/io/Console.java

+
+    @Override
+    public String input(String input) {
+        System.out.println(input);


혹시 입력 받고 sys.out(input) 하는 이유가 궁금합니다!

입력받는 값을 콘솔창에 띄우면 좋을거 같다는 생각에 작성을 하였는데 굳이 필요없는거 같네요... 지우겠습니다!

devYSK · 2023-06-16T06:30:25Z

src/main/java/com/bona/javacalculator/io/Console.java

+        boolean isLong = Validator.isLong(answer);
+        if (isLong) {
+            Long changeAnswer = answer.longValue();
+            System.out.println(changeAnswer);
+        } else {
+            System.out.println(answer);
+        }


if (Validator.isLong(answer))

하면 조금 더 깔끔하게 수정할 수 있을것 같아요

넵! 효율적으로 코드를 줄일수 있도록 더 고민해봐야겠습니다...

devYSK · 2023-06-16T06:33:21Z

src/main/java/com/bona/javacalculator/io/Console.java

+    public void printAll(List<ExpressionResult> expressionResultList) {
+        for (ExpressionResult history : expressionResultList) {
+            String input = history.getInput();
+            Double answer = history.getAnswer();
+
+            System.out.print(input + "=");
+            boolean isLong = Validator.isLong(answer);
+            if (isLong) {
+                Long changeAnswer = answer.longValue();
+                System.out.println(changeAnswer);
+            } else {
+                System.out.println(answer);
+            }
+        }
+    }


ExpressionRㄷsult와 history.. 뭔가 통일성이 없어서 아쉬워요!

result로 하면 더 통일성 있을것 같습니다

ExpressionResult 객체에 toString() 메서드를 오버라이드 하거나
내부적으로 String을 반환하는 메소드를 새로 만들어 주면 어떨까요?
그러면 여기 로직이 조금 더 깔끔해질 것 같습니다.

아하 네네! 전반적으로 통일성을 많이 생각하고 코딩을 해야겠습니다

객체 내부적으로 일 처리를 할 수 있도록 해야 코드가 더 깔끔해진다는것을 리뷰를 보고 많이 느끼네요.. 리팩토링 하겠습니다!!

devYSK · 2023-06-16T06:33:57Z

src/main/java/com/bona/javacalculator/io/Input.java

+package com.bona.javacalculator.io;
+
+public interface Input {
+    String input(String S);


S가 무엇인지 모르겠어요~!

str 이나 input과 같이 명확한 명칭을 사용하겠습니다ㅠㅠ!!

devYSK · 2023-06-16T06:34:32Z

src/main/java/com/bona/javacalculator/io/Console.java

+
+
+    @Override
+    public void outAnswer(Double answer) {


혹시 이 메소드는 무슨 일을 하고 있나요?

계산한 결과값을 출력하는 역할을 합니다!

devYSK · 2023-06-16T06:38:49Z

src/main/java/com/bona/javacalculator/model/ExpressionResult.java

+    public void setInput(String input) {
+        this.input = input;
+    }


setInput이 꼭 필요할까요!
사용되지 않는거 같습니다만,

꼭 값을 바꿀일이 있어 사용해야 한다면 조금 더 명확한

public void updateInput(String input) {...로직 }

과 비슷하게 써주는것은 어떨까요?
set이라는 이름 자체가 모호한 면도 있고, 더 명확하게 해준다면 예진님의 코드를 사용하는 다른 개발자 입장에서 명확해서 사용하기 좋을것 같습니다!

아하.. set사용을 지양하고 update와 같은 명확한 용어를 사용해야겠습니다..!

devYSK · 2023-06-16T06:43:40Z

src/main/java/com/bona/javacalculator/repository/CalMemoryRepository.java

+
+    @Override
+    public ExpressionResult findById(Long id) {
+        return store.get(id);


없으면 null을 반환할 것 같아요.

너가 찾는애는 있을수도 없을수도 있어.
예외를 던질수도 있겠지만 중단시키고 싶지 않다면
조금 더 방어적인 Optional을 사용해보는것은 어떨까요?

null이 뜰 수 있는 경우는 optional로 처리하는게 더 좋겠군요..리팩토링하겠습니다!!

devYSK · 2023-06-16T06:44:08Z

src/main/java/com/bona/javacalculator/repository/CalMemoryRepository.java

+import java.util.List;
+import java.util.Map;
+
+@Repository


혹시 이 어노테이션은 왜 사용하셨을까요!

devYSK · 2023-06-16T06:44:41Z

src/main/java/com/bona/javacalculator/repository/MemoryRepository.java

+
+

공백 한칸만 있어도 충분할 것 같아요!

공백 신경쓰겠습니다!

devYSK · 2023-06-16T06:45:27Z

src/main/java/com/bona/javacalculator/repository/MemoryRepository.java

+
+import java.util.List;
+
+@Repository


요기도있네용

Spring에서 이 어노테이션이 Repository를 의미하고 Bean으로 만들어 주기도 하지만 다른 역할도 해요! 한번 찾아보시겠어요?

devYSK · 2023-06-16T06:46:57Z

src/main/java/com/bona/javacalculator/util/Validator.java

+    public static boolean isNumber(String input) {
+        for (int i = 0; i < input.length(); i++) {
+            if (!Character.isDigit(input.charAt(i))) {
+                return false;
+            }
+        }
+        return true;
+    }


이렇게도 바꿀 수 있겠네요

public static boolean isNumber(String input) { for (char c : input.toCharArray()) { if (!Character.isDigit(c)) { return false; } } return true; }

for문과 foreach는 어떤 차이가 있을까요?

정규표현식을 사용하는것과는 어떠어떠한 차이가 있을지 공부해보세용

String의 요소만을 꺼내서 사용하고 싶기에 굳이 for문을 통해 인덱스를 꺼내서 해당 인덱스에 해당하는 char를 구하는것이 아니라 for-each문을 사용해 배열의 요소를 그대로 반환하는 것이 적합합니다!

정규표현식도 추가적으로 공부해서 차이점을 생각해보겠습니다!

devYSK · 2023-06-16T06:48:08Z

src/main/java/com/bona/javacalculator/util/Validator.java

+
+public class Validator {
+
+    private final Console console = new Console();


Valdator에 Console이 필요한지 조금 의문이 들어요!

사용하지 않는 코드를 지우는걸 종종 잊어버리네요.. 지우겠습니다!!

devYSK · 2023-06-16T06:48:40Z

src/main/java/com/bona/javacalculator/util/Validator.java

+            throw new RuntimeException("입력식이 포맷에 맞지 않습니다.");
+        }
+
+        if (!isNumber(inputString[inputString.length - 1])) {
+            throw new RuntimeException("입력식이 포맷에 맞지 않습니다.");
+        }
+        for (int i = 0; i < inputString.length; i++) {
+            if (i % 2 == 1) {
+                if (!Operator.isOperator(inputString[i])) {
+                    throw new RuntimeException("입력식이 포맷에 맞지 않습니다.");


Message가 중복으로 사용되네요

그렇다면?

메시지를 처리하는 클래스를 만든 후, 각각의 메시지를 private static final로 정의하고, public static으로 지정하여 get메소드를 통해 반환할 수 있게 해야할것 같습니다..!

devYSK · 2023-06-16T06:53:25Z

src/test/java/com/bona/javacalculator/core/CalculateTest.java

+import java.util.List;
+
+class CalculateTest {
+


Calculate라는 객체는 찾아볼수 없어서 뭐에 대한 테스트인지 모르겠어요.

코드를 보면 Calculator를 테스트 하는것 같은데 CalculatorTest가 더 맞지 않을까 싶습니다.

String input = "akdkf239 +dsam - adkfkdf / 0;

이걸 넣으면 예외가 발생할수도 있겟는데요.
방어할 수 있는 방법이 있을것같아요.
calculator에 List<String> 보다는, Expression 이나 Formula라는 객체를 넘기는데, 이 객체는 생성시에 검증을 하여 get()등과 같은 메소드로 꺼내면 안전한 식만 반환하게 하고
calculator에서 사용하는거죠
이런 방법이 있을것 같습니다.

오 네네! 해당 방법으로 코드를 다시 짜보겠습니다!

클래스명 변수명을 정하는데에 부족함을 많이 느낍니다😂

devYSK · 2023-06-16T06:53:43Z

src/test/java/com/bona/javacalculator/core/CalculateTest.java

+
+    Calculator calculator = new Calculator();
+    PostfixConverter makePostfix = new PostfixConverter();
+


예외가 발생할 수 있는 '실패'에 대한 테스트도 작성해보는것이 어떨까요?

통과하는 경우만 테스트코드를 작성했었네요..
예외 발생되는 테스트코드도 작성해보겠습니다!

devYSK · 2023-06-16T06:54:28Z

src/test/java/com/bona/javacalculator/core/PostfixTest.java

+
+    @Test
+    @DisplayName("후위표기식 출력 테스트")
+    public void makePostfixTest() {


테스트 메소드가 public 일 필요가 있는지 생각해보시겠어요 !
의견 부탁드려요

아 테스트 메소드는 해당 클래스에서만 쓰이기에 private이 맞습니다!

devYSK · 2023-06-16T06:55:08Z

src/test/java/com/bona/javacalculator/core/ValidatorTest.java

+        });
+        //then
+        assertEquals("0으로 나눌 수 없습니다.", exception.getMessage());
+


devYSK · 2023-06-16T06:55:20Z

src/test/java/com/bona/javacalculator/core/ValidatorTest.java

+
+
+class ValidatorTest {
+


테스트 케이스가 다양해서 좋습니다!

devYSK · 2023-06-16T06:56:15Z

src/test/java/com/bona/javacalculator/repository/CalMemoryRepositoryTest.java

+
+class CalMemoryRepositoryTest {
+    MemoryRepository memoryRepository = new CalMemoryRepository();
+


예진님께서 구현하신 코드이므로
findAll과 findById에 대한 테스트도 하는 것이 좋지 않을까요?

구현한 코드에 대해서 모두 테스트를 해보는게 맞는거 같습니다.. 추가적으로 테스트코드 돌려보겠습니다!

devYSK · 2023-06-16T06:56:28Z

src/test/java/com/bona/javacalculator/JavaCalculatorApplicationTests.java

+
+@SpringBootTest
+class JavaCalculatorApplicationTests {
+


필요없는 코드 같습니다!

필요 없는 코드는 지워도 되지 않을까요!

넵ㅜㅜ 불필요한 코드는 그때그때 지워두겠습니다!!

devYSK · 2023-06-16T07:09:05Z

src/test/java/com/bona/javacalculator/core/ValidatorTest.java

+        //given
+        String input = "1/0";
+        //when
+        Throwable exception = assertThrows(RuntimeException.class, () -> {


assertThatThrownBy 라는 메소드를 알아보시겠어요!

assertThatThrownBy는 가독성이 더 좋군요!! assertThatThrownBy를 사용해서 리팩토링 해보겠습니다!

devYSK

안녕하세요 예진님. 라이브 코드리뷰 후 피드백 반영하시고 개선하여 피알 올리시느냐 고생 많으셨습니다.
깔끔하게 올려주셔서 읽기 편했네요!
전반적으로 많이 개선되었고 고민하신 점이 많이 보여서 좋습니다.
리팩토링 하느냐 고민하신게 보여요!

여러 사정으로 많이 타이트하실탠데 과제하느냐 고생 많으셨어요.

코드를 보아하니 전반적으로 통일성이 많이 부족한거 같아요! 맞춰보려고 노력하시면 좋을것 같습니다.
Operator 등과 같은 객체도 꼼꼼하게 테스트 해주는 것이 좋다고 생각합니다. 단위테스트를 꼼꼼하게 해보려고 해보세요!
또한 null 방어나, 코드가 바뀌면 사이드 이펙트가 퍼질 곳이 많아 보입니다! 변경에 대한 영향을 최소화 하실 수 있는 방법을 고민해보시면 좋을것같아요

—
객체 지향적으로 코드를 짤 때, 기능을 담당하는 단위에 대해서 감이 잘 안잡혀 추상화의 수준과 클래스를 세분화 하는데에 고민이 많이 되었습니다.

역할을 잘 구분하고, 역할에 맞는 책임을 가지도록 나누는 것이 좋습니다!.

같은 기능을 하는 코드끼리 모여있어야 응집도가 높아지고, 흩어지지 않으면 찾기가 더 쉽기도 하구요
변경에 있어 영향이 최소화 되기도 합니다.

SOLID에 대해 많이 학습해보세요!

또한 SOLID 중 S (SRP, 단일 책임 원칙)을 생각하시면 조금 더 쉽게 설계하실 수 있을것 같습니다

클래스나 객체가 단 하나의 책임을 가지도록 해보세요. 클래스가 여러 가지 역할이나 기능을 수행하면 코드를 이해하고 유지보수하기 어려워집니다.
클래스나 객체를 작은 기능 단위로 분리하고, 각 클래스는 한 가지 명확한 역할을 수행하도록 유지하는것이 좋습니다

많이 짜보시고 다른 사람 코드 많이 보시면 늘 수 있다고 생각합니다!

—

예외처리

저는 예외처리를 CalculatorManager가 구동을 시작하는 while문 안에서 모든 예외를 RuntimeException으로 받아 해당 예외들의 메시지를 띄우는 방식으로 사용자에게 에러를 표시하고자 하였는데 효율적인 예외 처리에 대해 궁금합니다.

현재 구조에서는 예외처리를 마땅히 처리하기가 어려운 구조이긴 합니다!

효율적인 예외 처리라는게 어떤 말씀인지 이해가 잘 안되지만,
구조적으로 디자인 패턴 중 Proxy 패턴을 이용한다면 메인 로직에서는 try-catch를 감출 수 있을 것 같긴 합니다.

보통 메시지가 정해져 있다면 상수를 정의하고, 해당 상수를 custom 예외에서 받고, exception에서 해당 상수로 메시지를 꺼내 출력하곤 합니다.

예외 처리 테스트 코드의 경우 저는 단순히 해당 예외 메시지가 같은지를 비교했는데, 어떻게 코드 작성을 하는게 좋은 지 궁금합니다.

저는 의도한 상황대로 예외가 발생하는지 실패하는 테스트를 자주 합니다!
특정 동작을 실행했을 때, 의도한 예외가 발생하고, 해당 예외 클래스랑 메시지 등을 비교합니다

코드라면, assertj를 사용하시는 것으로 보아 assertThatThrownBy 에 대해 알아보세요!
assertj를 학습하시면 더 많은 기능을 제공하는 것들을 알 수 있습니다.

—
Calculator를 계산만 하는 객체로 두고 나머지 기능들은 CalculatorManager가 담당하는데, 이 경우 CalculatorManager 또한 세분화 시키는게 맞을지 궁급합니다.

혹시 세분화라는게 어떤 의도로 말씀하신건지 궁금합니다!

제가 예진님 코드를 보았을때에는
매니저란 객체가

사용자로부터 입력을 받음
입력을 검증하고 Calculator한테 계산을 위임
결과를 Repository에게 저장하라고 시킴
결과들을 Repository들로부터 돌려받아 사용자에게 출력

을 하는데, 이 객체들을 관리하는 역할이라면 적합한 역할이라고 생각은 합니다만, 너무 많은 일을 하는거 같기도 합니다.

입력과 검증을 Manager로부터 제외시키고, Manager는 계산을 위임하고 저장하고, 계산값들을 돌려받는 정도의 역할로 줄일수도 있다고 생각합니다.

궁금하신점 있으시면 말씀주세요!

WooSungHwan

예진님 고생하셨습니다 ㅎㅎ
병행하시느라 고생많으실텐데 인생에서 이때만큼 열심히 산적 없었다..! 싶을 정도로 집중력 발휘해주셔서 보기 좋네요 ㅎㅎ
리뷰 몇자 남겼으니 확인 부탁드리겠습니다~

WooSungHwan · 2023-06-16T14:31:21Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+    public void calculate() {
+        String formula = console.input("식을 입력해주세요 : ");
+
+        validator.validateFormula(formula);
+
+        List<String> postfix = converter.convert(formula);
+        double result = calculator.calculate(postfix);
+
+        repository.save(new ExpressionResult(formula, result));
+
+        console.outAnswer(result);
+    }


2번을 누르고 계산으로 왔기 때문에 이제 계산을 진행하는 흐름으로 생각하면 됩니다.
여기는 logic에 해당하는 부분이에요.
근데 이 logic 또한 추상화를 한다면 계산하는 로직을 담당하는 무언가(클래스)가 되는 거죠.

근데 그 무언가가 계산하는게 사칙연산이고 postfix를 통해서 변환해서 계산하는 방식인거죠. 여기서 로직과 행위에 대해서 구분의 어려움이 있을 수 있겠는데.. 로직은 보통 행위자를 통해 조종하는 녀석이고 행위자는 특정 데이터만을 기준으로 처리하는 놈이라고 생각하면 될것 같아요.

여기서 보면 calculate은 계산로직을 수행하므로 로직이될것이고, console, validator, convertor, calculator를 활용해서 계산 결과를 보여주는 행위를 구현하기 때문에 logic이라고 볼 수 있는 것이죠. ㅎㅎ
많이 어려웠을 거에요. 여기까지만 이해해봅시다! 궁금한건 질문해주세요.

class 로직 { private 행위자 행위자; private 기타행위자 기타행위자; 행위로직() { 행위자.행위(); 기타행위자.기타행위(); } }

�class 계산 { private 계산행위자 계산행위자; 계산로직() { 계산행위자.계산(); // 계산결과 출력 // 계산결과 저장. } }

우와 상세한 설명 감사합니다!! 이제야 로직과 행위 구분을 할 수 있게 된거 같습니다!!

WooSungHwan · 2023-06-16T14:34:07Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+    public void calculate() {
+        String formula = console.input("식을 입력해주세요 : ");
+
+        validator.validateFormula(formula);
+
+        List<String> postfix = converter.convert(formula);
+        double result = calculator.calculate(postfix);
+
+        repository.save(new ExpressionResult(formula, result));
+
+        console.outAnswer(result);
+    }
+
+
+    public void inquiry() {
+        List<ExpressionResult> all = repository.findAll();
+
+        if (all.isEmpty()) {
+            console.printMessage("조회 결과 기록이 존재하지 않습니다");
+            return;
+        }
+
+        console.printAll(all);


조회하는 로직
조회하는 행위자가 마땅하지 않으니 조회로직만 있을듯 합니다.

WooSungHwan · 2023-06-16T14:34:38Z

src/main/java/com/bona/javacalculator/core/CalculatorManager.java

+    public void calculate() {
+        String formula = console.input("식을 입력해주세요 : ");
+
+        validator.validateFormula(formula);
+
+        List<String> postfix = converter.convert(formula);
+        double result = calculator.calculate(postfix);
+
+        repository.save(new ExpressionResult(formula, result));
+
+        console.outAnswer(result);
+    }
+
+
+    public void inquiry() {
+        List<ExpressionResult> all = repository.findAll();
+
+        if (all.isEmpty()) {
+            console.printMessage("조회 결과 기록이 존재하지 않습니다");
+            return;
+        }
+
+        console.printAll(all);


영수님은 public으로 열어놔서 말씀주신것 같아요.

WooSungHwan · 2023-06-16T14:37:36Z

src/main/java/com/bona/javacalculator/core/Priority.java

+    public static int getOperationPriority(String operator) {
+
+        switch (operator) {
+            case "*":
+            case "/":
+                return 2;
+            case "+":
+            case "-":
+                return 1;
+            default:
+                throw new IllegalStateException("유효하지 않은 연산자 입니다" + operator);
+        }
+    }


연산자 Enum 만들었는데 사용했으면 좋겠습니다!

WooSungHwan · 2023-06-16T14:42:55Z

src/main/java/com/bona/javacalculator/core/converter/PostfixConverter.java

+        for (int index = 0; index < splitInput.length; index++) {
+            String pieceInput = splitInput[index];
+            if (Operator.isOperator(pieceInput)) {
+                pushOperator(pieceInput, postfix, stack);
+            } else {
+                pushNumber(pieceInput, postfix);
+            }
+        }


이 행위는 좀더 구체적으로 구분하자면
숫자와 연산자를 "구분" 하는 행위로 추상화할 수 있겠군요~~~~~

오 네네! 구분하는 행위를 하도록 추상화해보겠습니다!!

born-A added 13 commits June 8, 2023 16:15

project initial setting

5aa40b2

[ADD] CalMemoryRepository save() test

[Update]우선순위, 사칙연선 서비스

02bdc17

[Update]우선순위 리팩토링 완료

30aee88

[ADD]ValidateService

2003310

[UPDATE]ValidateService

7e074e5

[UPDATE]ValidateService

067ffa5

[ADD]ValidateServiceTest

1a67072

[ADD]Calculator - InquiryService, CalculateService 추출

b8c9bd0

[ADD] AnalyzeServiceTest, MakePostfixServiceTest

c12ed55

[UPDATE] 종료 선택, 예외 처리

95e32c1

[UPDATE]패러다임 수정, Calculator->srp, Validator->validateFormula 통합

1c13f9a

[UPDATE] exception 처리

55ee76d

[UPDATE] 주석 수정

a54696a

born-A added Team-Blackdog 🔥 매운맛 labels Jun 15, 2023

born-A requested review from WooSungHwan and devYSK June 15, 2023 19:02

born-A self-assigned this Jun 16, 2023

devYSK reviewed Jun 16, 2023

View reviewed changes

devYSK requested changes Jun 16, 2023

View reviewed changes

WooSungHwan requested changes Jun 16, 2023

View reviewed changes


		public interface AbstractCalculator {

		double calculate(List<String> formula);

		int operationPriorityNow = getOperationPriority(NowOperator);

		return Integer.compare(operationPriorityNow, operationPriority);


		public class Validator {

		private final Console console = new Console();


		Calculator calculator = new Calculator();
		PostfixConverter makePostfix = new PostfixConverter();


		class CalMemoryRepositoryTest {
		MemoryRepository memoryRepository = new CalMemoryRepository();


		import java.util.List;

		@Repository

[4기 - 신예진] 1~2주차 과제: 계산기 구현 미션 제출합니다. #194

Are you sure you want to change the base?

[4기 - 신예진] 1~2주차 과제: 계산기 구현 미션 제출합니다. #194

Uh oh!

Conversation

born-A commented Jun 15, 2023

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devYSK Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

devYSK Jun 16, 2023 •

edited

Loading