[4기 - 김영훈] 1~2주차 과제: 계산기 구현 미션 제출합니다. #163

B2SIC · 2023-06-11T09:16:41Z

📌 과제 설명

1~2주차 과제인 계산기 구현 미션입니다.

기본적인 사칙연산(+, -, *, /) 이 가능합니다.
사칙연산 시 계산의 우선순위를 반영해 결과를 반환합니다.
연산 이력을 별도로 저장하여 내역을 확인할 수 있습니다.

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

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

아직 객체지향적으로 구현한다는게 어떻게 하는건지 잘 모르겠어서 객체지향 관점에서 피드백을 받고 싶습니다!
전체 패키지 구조와 클래스의 위치가 적절하게 들어갔는지 알고 싶습니다.
인터페이스의 활용이 적절했는지 알고 싶습니다.
- Interface 내 static 메소드를 사용했는데, 근거가 적절했는지 알고 싶습니다.
  -> Converter 라는 이름과 연관된 기능으로 보고 공통된 메소드로 사용 할 수 있다는 점에서 반영했습니다.
Console.run()과 PostfixConverter.convert() 의 코드 라인이 너무 긴 것이 신경쓰입니다, 메소드 구현 코드가 길 때 어떤 기준을 가지고 분리 하는 것이 좋을지 궁금합니다.
Console 클래스 내에서 try-catch를 이용한 예외 처리가 있는데 이런 상황에서 예외 처리를 위한 클래스를 별도로 두는 것이 더 좋을지 궁금합니다.
정규표현식을 사용하는 것과 코드 가독성은 관련이 있을 것 같은데 복잡하지 않은 정규식까지만 활용하는 것이 좋을까요?
테스트 코드를 작성해보았는데 어떤 식으로 작성하는게 좋을지 잘 모르겠습니다..!
- 테스트 클래스 내 메소드 명에 대한 규칙은 어떻게 하는게 좋을지 (@DisplayName과 연관해서)
- 여러 예외 상황에 대한 테스트 케이스를 하나 하나 직접 작성하는게 맞는지 모르겠습니다.
이 외 제가 놓친 부분에 대해 어떤 피드백도 다 좋습니다!

0923kdh

영훈님 고생하셨습니다!
PR 포인트에 남겨주신 것 중 일부는 리뷰에 일부는 아래에 달아놓았어요~

Console.run()과 PostfixConverter.convert() 의 코드 라인이 너무 긴 것이 신경쓰입니다, 메소드 구현 코드가 길 때 어떤 기준을 가지고 분리 하는 것이 좋을지 궁금합니다.
- 보통 저는 이럴 때, 메서드에서 하는일이 어떤일들이 있는지, 한글로 적어봅니다. 1. ~ 함 / 2. ~ 함 / 3. ~ 함 이런식으로요. 메세드에서 하는일을 하나씩 정리하다보면, 어떤 역할을 수행하는지 조금 카테고리화 되는 것 같아요. 이 기준으로 메서드를 나누곤 하는데요. 물론 항상 이렇게 하는것은 아닙니다만, 메서드가 커질때에는 이런식으로 접근하는게 도움이 되기도 하더라구요.
Console 클래스 내에서 try-catch를 이용한 예외 처리가 있는데 이런 상황에서 예외 처리를 위한 클래스를 별도로 두는 것이 더 좋을지 궁금합니다.
- 어떤 말인지 조금 더 자세히 설명해주실 수 있을까요?
정규표현식을 사용하는 것과 코드 가독성은 관련이 있을 것 같은데 복잡하지 않은 정규식까지만 활용하는 것이 좋을까요?
- 너무 복잡한 정규식도 유지보수 비용을 높인다고 생각하기때문에, 너무 복잡한 정규식은 되도록 지양하려고 합니다. (제 기준)
테스트 코드를 작성해보았는데 어떤 식으로 작성하는게 좋을지 잘 모르겠습니다..!
- 테스트 클래스 내 메소드 명에 대한 규칙은 어떻게 하는게 좋을지 (@DisplayName과 연관해서)
  - 저는 보통 메서드명을 작성할 때 명세로써 동작할 수 있도록 하는 편이에요.
  - 처음 코드를 본다고 생각할 때, 테스트 코드만 보고도 어떻게 동작하는지 유추할 수 있도록 말이에요.
- 여러 예외 상황에 대한 테스트 케이스를 하나 하나 직접 작성하는게 맞는지 모르겠습니다.
  - 저는 예외상황에 대한 테스트는 최대한 꼼꼼하게 작성하는게 좋다고 생각합니다.

0923kdh · 2023-06-11T13:23:20Z

src/main/java/repository/MapRepository.java

+
+    @Override
+    public void inquiry() {
+        System.out.println(DisplayMessage.SPLIT_LINE.getMessage());


데이터를 저장하고 조회하는 클래스에서 view와 관련된 의존성을 가져가고 있네요.
view와 관련된 의존성을 해당 클래스에서 가지고 있을 필요가 있을까요 ?

음.. view와 관련된 의존성을 Repository에서 가질 필요는 없을 것 같습니다.
IO를 담당하는 곳에서 처리해주는게 더 좋은 방법인 것 같습니다.
의존성에 대해 생각하지 못하고 코드를 구성했던 것 같습니다..!
조금 더 신경 써보도록 하겠습니다.

0923kdh · 2023-06-11T13:23:39Z

src/main/java/repository/MapRepository.java

+    @Override
+    public void inquiry() {
+        System.out.println(DisplayMessage.SPLIT_LINE.getMessage());
+        for (Integer integer : repository.keySet()) {


stream 사용을 권장합니다!

stream을 활용해서 다시 구현해보겠습니다!

0923kdh · 2023-06-11T13:24:03Z

src/main/java/repository/MapRepository.java

+            System.out.println(integer + ". " + repository.get(integer));
+        }
+        System.out.println(DisplayMessage.SPLIT_LINE.getMessage());
+    }


뉴라인을 한칸 넣어주시는것도 코드 가독성에 도움이 됩니다.

뉴라인이 17~18라인 사이를 말씀하시는게 맞을까요?

0923kdh · 2023-06-11T13:27:26Z

src/main/java/repository/MapRepository.java

+    }
+    @Override
+    public void save(String record) {
+        repository.put(++count, record);


MapRepository에 key 값으로 순서를 저장하신 것 같은데요,
저장하는 객체의 입장에서 저장 순서까지 가지고 있는게 좋은지 잘 모르겠네요.
저장 순서는 영속 관점에서도, 데이터의 활용성 관점에서도 중요하지 않는 데이터라 생각이되어요.
게다가 LinkedHashMap는 순서보장도 되기때문에, 불필요한 데이터가 저장되는 것 같네요.

순서가 보장되는 것을 알고 LinkedHashMap을 선택했는데 key 값에 순서를 저장하는 실수를 했네요..
수정해보겠습니다 피드백 감사합니다!

0923kdh · 2023-06-11T13:29:22Z

src/main/java/io/DisplayMessage.java

+    SELECT("1. 조회"),
+    CALCULATION("2. 계산"),
+    EXIT("3. 종료"),
+    CHOOSE("선택 : "),
+    OUTPUT("계산 결과 : "),
+    BAD_REQUEST("잘못된 명령입니다!"),
+    WRONG_EXPR("잘못된 계산식입니다!"),
+    SPLIT_LINE("==================");


출력 메세지와, 메뉴가 섞여있는 것 같아요.
출력 메세지와 메뉴를 분리해서 사용하는 방향은 어떨까요?

아하 메시지가 클래스 이름에 맞게 적절히 나눠질 필요가 있는거군요! 유지보수 측면에서도 좋을 것 같습니다.
감사합니다!

0923kdh · 2023-06-11T13:42:45Z

src/main/java/calculation/Converter.java

+    static String removeWhiteSpace(String targetExpr) {
+        return targetExpr.replace(" ", "");
+    }


Converter 라는 이름과 연관된 기능으로 보고 공통된 메소드로 사용하기위해 static 메서드로 만들었다고 하셨는데요, removeWhiteSpace() 메서드 같은 경우 Converter를 구현하지 않는 구현체에서도 (static 이기때문에) 충분히 사용할 수 있는 메서드라고 생각합니다. 또, 기능 자체만으로 보더라도 Converter에 있는것 보단 util class 로 관리하는게 더 적절해보입니다.

개인적으로 많이 궁금했던 부분이었는데 상세한 리뷰 감사합니다!
Converter 이외의 구현체에서도 활용할 수 있다는 점을 놓쳤던 것 같습니다.
처음엔 별도의 클래스로 관리해볼까 하는 생각도 있었는데 제 코드를 보니 관련해서 묶을 수 있는 메소드가 하나만 있는 것 같아서 주저했던 것 같습니다.
변경사항에 반영해보겠습니다!

0923kdh · 2023-06-11T13:45:01Z

src/main/java/App.java

+public class App {
+    public static void main(String[] args) throws IOException {
+        Console console = new Console(new MapRepository());
+        console.run();
+    }
+}


Suggested change

public class App {

public static void main(String[] args) throws IOException {

Console console = new Console(new MapRepository());

console.run();

}

}

public class CalculatorApplication {

public static void main(String[] args) throws IOException {

Calculator calculator = new Calculator(new CalculatorHistoryRepository());

calculator.run();

}

}

와 같은 네이밍은 어떨까요? App, Console, MapRepository만 보고 어떤 프로그램이 구동되는지 전혀 예측할 수 없는 것 같아요.

깊게 고민해보지 않았던 문제였던 것 같습니다..
앞으로 좀 더 네이밍에 신경쓰면서 이름으로도 구동을 예측할 수 있도록 사용해보겠습니다.

0923kdh · 2023-06-11T13:47:05Z

src/main/java/io/Console.java

+import calculation.Calculator;
+import calculation.Converter;
+import repository.Repository;
+import validation.Validator;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.util.EmptyStackException;


콘솔 생성시 Repository만을 주입받는데요.
import 구문을 보면 Calculator, Converter,Converter, BufferedReader 등 계산프로그램에 필요한 모든것을 관장하고 있네요. 해당 클래스의 책임과 역할을 적절히 쪼갤 필요가 있어보입니다!

추가로 만약 Console이 아닌 웹 환경으로 넘어가야한다면 변경해야 할 부분이 엄청나게 많을 것 같은데요.
좀 더 책임과 역할을 나누어서 유지보수 하기 쉽게 만들어봅시다~!

피드백을 통해 역할과 책임에 대해서 조금씩 알아가는 기분이 듭니다..!
필요한 객체를 외부에서 주입 받도록 하여 책임과 역할을 나눠보겠습니다.

0923kdh · 2023-06-11T13:47:49Z

src/main/java/calculation/Calculator.java

+package calculation;
+
+import validation.Validator;
+import java.util.*;


https://stackoverflow.com/questions/147454/why-is-using-a-wild-card-with-a-java-import-statement-bad 한번 읽어보세요~

좋은 정보 감사합니다!
IDE에 의존하다보니 이런 부분은 생각하지 못했던 것 같습니다.
와일드카드 문자 대신 명시적으로 참조를 표기할 때 얻을 수 있는 이점이 꽤 크네요!

0923kdh · 2023-06-11T13:48:58Z

src/main/java/io/Console.java

+                } catch (EmptyStackException | ArithmeticException e) {
+                    System.out.println(DisplayMessage.WRONG_EXPR.getMessage());
+                }


범용적으로 사용할 수 있는 예외를 사용하는것도 좋지만,
예외처리 연습하는 겸, custom exception을 만들여서 관리해보는건 어떨까요?
custom exception을 만들면 outputView에서 예외 메세지 내용을 관리하지 않고 custom exception에서 관리할 수 있는 장점이 생길 것 같네요!

예외 메시지 내용을 Exception 내부에서 관리할 수 있다는 장점이 있는 것은 몰랐네요..!
Custom Exception을 활용해서 작성해보겠습니다!

yuminhwan

영훈님! 과제하시느라 수고많으셨습니다! 💯
인터페이스를 통해 확장성을 생각한다던가 검증 객체를 만들어서 한쪽에서 관리한다던가 등 여러 노력이 보여서 좋았습니다 👍
몇가지 코멘트 남겼는데 확인해주세요~!

아직 객체지향적으로 구현한다는게 어떻게 하는건지 잘 모르겠어서 객체지향 관점에서 피드백을 받고 싶습니다!

객체지향을 공부하다보면 책임, 역할, 협력, 캡슐화, 응집도 등등 여러 개념이 나오게 될 것인데요. 하나씩 천천히 공부해보시는 것도 좋을 것 같아요.
예를 들어 하나의 객체에게 여러가지 책임이 있다면 많은 메서드가 생겨날 것이고 재사용이나 유지보수가 쉽지 않겠죠? 이럴 때 책임을 분산시켜 객체를 나누게 된다면 재사용성도 올라갈 것이고 응집도도 높아지게 될거예요! 테스트도 하기 쉬워지구요. 아직 어렵겠지만 많은 고민을 해보시면 좋을 것 같아요.

참고 자료 : 초식 - OOP 잘하려면

전체 패키지 구조와 클래스의 위치가 적절하게 들어갔는지 알고 싶습니다.

현재는 많은 클래스가 없지만 적절하게 나뉘어 진 것 같습니다.
혹시 영훈님이 패키지를 나눈 기준이 있을까요?

인터페이스의 활용이 적절했는지 알고 싶습니다.
Interface 내 static 메소드를 사용했는데, 근거가 적절했는지 알고 싶습니다.
-> Converter 라는 이름과 연관된 기능으로 보고 공통된 메소드로 사용 할 수 있다는 점에서 반영했습니다.

해당 인터페이스에서 static 메서드가 꼭 필요할까?를 생각해보면 좋을 것 같아요. removeWhiteSpace가 굳이 Converter에서 해줘야할까 라는 생각이 들기도 하네요.

Console.run()과 PostfixConverter.convert() 의 코드 라인이 너무 긴 것이 신경쓰입니다, 메소드 구현 코드가 길 때 어떤 기준을 가지고 분리 하는 것이 좋을지 궁금합니다.

위에 영상에서도 나오다 싶이 어떻게 해야하는 지 보단 뭘 해야하는지를 먼저 생각해보면 좋을 것 같아요.
계산이라는 기능을 하기 위해서 파싱도 해야할 것이고 검증도 해야할 것이고 계산까지도 해야할텐데요. 요런 필요한 기능별로 먼저 나누어 보고 거기 안에서도 세부적으로 나누어보는 것은 어떨까요? 최종적으로는 한 메서드가 하나의 기능만 하도록이요!

Console 클래스 내에서 try-catch를 이용한 예외 처리가 있는데 이런 상황에서 예외 처리를 위한 클래스를 별도로 두는 것이 더 좋을지 궁금합니다.

별도로 둔다는게 예외가 발생하면 어떤 동작을 하겠다는 객체를 만든다는 말씀이실까요?? 구체적으로 설명해주세요!

정규표현식을 사용하는 것과 코드 가독성은 관련이 있을 것 같은데 복잡하지 않은 정규식까지만 활용하는 것이 좋을까요?

가독성을 높이기 위해 사용하는 것은 좋다고 생각하지만 너무 과하면 또 안좋다고 생각합니다! 정규식보단 메서드 나누기, 책임 나누기 등으로 가독성을 높이기는 더 중요할 것 같아요.

테스트 코드를 작성해보았는데 어떤 식으로 작성하는게 좋을지 잘 모르겠습니다..!
테스트 클래스 내 메소드 명에 대한 규칙은 어떻게 하는게 좋을지 (@DisplayName과 연관해서)
여러 예외 상황에 대한 테스트 케이스를 하나 하나 직접 작성하는게 맞는지 모르겠습니다.

일단 예외 상황에 대해서는 최대한 꼼꼼하게 해주는 것이 좋아요! 실수를 할 수 도 있고 어떤 값이 들어오는 지도 불분명하기 때문에 최대한 많은 케이스를 테스트해서 미리 방지하는 게 좋겠죠?
DisplayName의 경우 테스트하는 메서드(기능)이 어떤 것인지 나타내주면 좋을 것 같은데요.
예를 들어 연산자를 통해 계산을 한다는 메서드를 테스트 한다면 주어진 연산자에게 맞는 계산을 진행한다
로 작성할 수 있겠죠?
후에 테스트가 명세가 될 수 있다고 생각하고 잘 적어주는 게 좋습니다.

yuminhwan · 2023-06-12T13:01:20Z

src/main/java/calculation/PostfixConverter.java

+
+public class PostfixConverter implements Converter {
+    private final String infixExpr;
+    private final Map<String, Integer> priority = new HashMap<>() {{


Map.of을 사용해봐도 좋을 것 같아요.

아하 Map.of를 쓰면 좀 더 간결하게 작성할 수 있는 것 같습니다!
적용해보겠습니다.

yuminhwan · 2023-06-12T13:02:17Z

src/main/java/calculation/PostfixConverter.java

+    public PostfixConverter(String infixExpr) {
+        this.infixExpr = infixExpr;
+    }
+


인스턴스 변수로 문자열을 받고 있는데요.
인스턴스 변수로 가지고 있어야할 이유가 있을까요?

인스턴스 변수로 가지고 있어야 할 명확한 이유는 없는 것 같습니다. 계산을 위한 계산식을 객체 생성 시점에 가져와서 사용하려고 했는데 단순히 메소드의 파라미터로 전달해도 될 것 같습니다!

yuminhwan · 2023-06-12T13:05:53Z

src/main/java/calculation/Calculator.java

+                switch (item) {
+                    case "+":
+                        calculationStack.push(operand1 + operand2);
+                        break;
+                    case "-":
+                        calculationStack.push(operand1 - operand2);
+                        break;
+                    case "*":
+                        calculationStack.push(operand1 * operand2);
+                        break;
+                    case "/":
+                        calculationStack.push(operand1 / operand2);
+                        break;
+                }
+            }


stack에 push하는 행위와 계산하는 행위를 한번 분리해볼까요~?

네! Operator에 따른 계산부와 Stack을 관리하는 부분을 분리해보겠습니다.

yuminhwan · 2023-06-12T13:08:29Z

src/main/java/repository/MapRepository.java

+    public int getCount() {
+        return count;
+    }
+}


요곤 테스트를 위한 메서드일까요?

네 맞습니다..!
repository를 private으로 설정해두면서 테스트 메소드를 통해 접근이 안되어 작성했던 메소드인데 좋은 방법은 아니었던 것 같습니다..
private 객체 내부에 뭔가를 테스트 해야 할 때는 어떻게 할지 궁금합니다!

private 메서드에 대한 테스트는 아직까지 많이 논의가 되고 있는 주제인데요.
저는 지양하는 편이지만 영훈님만의 기준을 잡으면 좋을 것 같아요 ~

참고자료 : https://mangkyu.tistory.com/235

추가로 count같은 경우 자료구조의 size를 통해서 대처가 될듯합니다!

아하 굉장히 궁금했던 주제였는데 나름 이슈가 있군요!
피드백 감사합니다!

yuminhwan · 2023-06-12T13:11:47Z

src/main/java/calculation/Calculator.java

+public class Calculator {
+    private final String expression;
+
+    public Calculator(String infixExpr) {
+        Converter converter = new PostfixConverter(infixExpr);
+        expression = converter.convert();
+    }
+


Converter를 인터페이스로 만들어서 유연하게 사용하려고 하신 것 같은데 Calculator 객체에서 PostfixConverter를 의존하고 있기 때문에
내부 구현에 의존적이라고 생각합니다.
밖에서 주입하거나 후위표기식 자체를 생성자로 받고 검증을 한다던지 해도 좋을 것 같아요.

멘토님 리뷰를 통해 의존성이 있다는게 무슨 의미인지 조금 알 것 같습니다.
PostfixConverter를 의존하지 않고 외부에서 주입을 받는 식으로 변경해보겠습니다!

yuminhwan · 2023-06-12T13:13:37Z

src/test/java/calculation/CalculatorTest.java

+        String infixExpr = "1 + 3 * 6";
+        Calculator calculator = new Calculator(infixExpr);
+        int result = calculator.calculate();
+        assertThat(result).isEqualTo(19);


추가로 given / when / then 으로 테스트를 정리해봐도 좋을 것 같아요.

- Given : 시나리오 진행에 필요한 모든 준비 과정 (객체, 값, 상태 등) - 어떤 환경에서 - When : 시나리오 행동 진행 - 어떤 행동을 진행했을 때 - Then : 시나리오 진행에 대한 결과 명시 ,검증 - 어떤 상태 변화가 일어난다.

yuminhwan · 2023-06-12T13:14:47Z

src/main/java/calculation/PostfixConverter.java

+    private <T> boolean stackIsNotEmpty(Stack<T> getStack) {
+        return !getStack.isEmpty();
+    }
+


제너릭을 사용해주셨는데요! 사용하신 이유가 있을까요~? (정말 궁금해서 여쭤보는 겁니다!)
추가로 해당 메서드를 굳이 추출해줄 필요가 있을까? 라는 생각이 드네요. 그냥 감싸고 있는 느낌만 듭니다!

제너릭을 사용해주셨는데요! 사용하신 이유가 있을까요~? (정말 궁금해서 여쭤보는 겁니다!)

제너릭을 사용했던 의도는 만약 다른 곳에서도 Stack를 활용한다면 공통 메소드로 뽑아서 하나의 메소드로 활용하도록 하는게 좋지 않을까 했기 때문이었습니다.
그런데 현재 구현에서는 PostfixConverter 에서만 Stack이 비어있는지 여부를 체크하고 있어서 큰 의미는 없는 것 같습니다..!

이와 관련해서 평소 궁금했던 점이 있습니다.
처음 구현을 할 때 향후 범용성 까지 고려해서 메소드를 구현하는 것이 좋은지, 아니면 우선 필요한 구현에만 충실한 후 확장 가능성이 생길 때 또는 필요할 때 범용성 있게 메소드를 리팩토링 하는게 좋을지 멘토님의 의견을 듣고 싶습니다!

추가로 해당 메서드를 굳이 추출해줄 필요가 있을까? 라는 생각이 드네요. 그냥 감싸고 있는 느낌만 듭니다!

제 의도는 Not 표기법인 ! 때문이었습니다.
Stack에서 isNotEmpty() 라는 메소드는 없기 때문에 !getStack.isEmpty(); 보다는 stackIsNotEmpty가 코드 가독성 측면에서 더 좋지 않을까 해서 시도해봤는데 큰 효용성은 없었던 것 같습니다..! 혹시 더 좋은 방법이 있을까요? 아니면 보통 그냥 ! 표기법을 그대로 사용하는게 좋을까요?

처음 구현을 할 때 향후 범용성 까지 고려해서 메소드를 구현하는 것이 좋은지, 아니면 우선 필요한 구현에만 충실한 후 확장 가능성이 생길 때 또는 필요할 때 범용성 있게 메소드를 리팩토링 하는게 좋을지 멘토님의 의견을 듣고 싶습니다!

요고는 구현하는 사람마다 다를 듯 한데요. 구현하시다보면 요곤 다른곳에서도 유용하게 사용될 것 같다면 주로 제너릭을 사용해서 확장성을 넓여주는 것 같습니다.
저 같은 경우 일단 구현에 충실하면서도 후에 변경되더라도 최대한 그 변경범위가 적도록 하는 것 같아요!

제 의도는 Not 표기법인 ! 때문이었습니다.
Stack에서 isNotEmpty() 라는 메소드는 없기 때문에 !getStack.isEmpty(); 보다는 stackIsNotEmpty가 코드 가독성 측면에서 더 좋지 않을까 해서 시도해봤는데 큰 효용성은 없었던 것 같습니다..! 혹시 더 좋은 방법이 있을까요? 아니면 보통 그냥 ! 표기법을 그대로 사용하는게 좋을까요?

!을 제가 놓쳤네요 ㅎㅎ !을 피하기 위해서 사용하는 것은 매우 좋은 습관입니다. 읽기가 쉬워지거든요. 대신 메서드 자체에서 자료구조를 표현하는 것은 지양하는 게 좋을 듯 싶네요.
좀더 확장성 있게 사용한다면 Collection으로 업캐스팅해서 사용해볼 수 있을 것 같아요. 요런 메서드는 라이브러리가 잘 되어 있어서 라이브러리 코드를 한번 보시는 것도 추천드립니다.

오 좋은 정보 좋은 피드백 감사합니다!!
다양하게 시도해보면서 익혀보겠습니다~

yuminhwan · 2023-06-12T13:16:14Z

src/main/java/io/Console.java

+import calculation.Calculator;
+import calculation.Converter;
+import repository.Repository;
+import validation.Validator;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.util.EmptyStackException;


추가로 만약 Console이 아닌 웹 환경으로 넘어가야한다면 변경해야 할 부분이 엄청나게 많을 것 같은데요.
좀 더 책임과 역할을 나누어서 유지보수 하기 쉽게 만들어봅시다~!

yuminhwan

영훈님 리팩토링 하시느라 수고하셨습니다!
처음때 보다 코드가 많이 좋아졌네요! 💯
대신 조금만 더 객체간의 역할을 분리해보는 건 어떨까 싶습니다!
간단하게 코멘트 남겼는데 확인해주세요~

yuminhwan · 2023-06-19T13:00:14Z

src/main/java/computation/Operator.java

+    public static Operator getOperator(String getString) {
+        return Arrays.stream(Operator.values())
+                .filter(s -> s.operator.equals(getString))
+                .findFirst()
+                .get();
+    }


getString이라는 네이밍보다는 operator라는 네이밍이 좀 더 자연스러울 것 같아요.
자바에서 변수는 명사를 주로 사용합니다!

참고자료 : https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/

추가로 Optional.get()을 이용해서 객체를 꺼내주고 있는데요. 만약 �이상한 값이 들어오면 null이 반환될 것 같아요.
Optional에 대해 공부해보시면 좋을 것 같아요!

참고자료 : https://www.daleseo.com/java8-optional-before/

아하 그렇군요! 네이밍에 조금 더 신경을 써야 할 것 같습니다.
Optional에 대한 공부 자료도 감사합니다!

yuminhwan · 2023-06-19T13:05:24Z

src/main/java/io/Calculator.java

+            int choice = Integer.parseInt(getChoice);
+            if (choice == Menu.QUERY.getMenuNumber()) {
+                // 조회
+                runInquiry();
+            } else if (choice == Menu.CALCULATE.getMenuNumber()) {
+                // 계산
+                runCalculator();
+            } else if (choice == Menu.EXIT.getMenuNumber()) {
+                // 종료
+                System.exit(0);
+            } else {
+                System.out.println(DisplayMessage.BAD_REQUEST.getMessage());
+            }


Menu Enum에 저장되어 있는 값을 꺼내서 비교하지 말고 입력받은 값을 Menu로 파싱한 다음
비교하면 어떨까요??
이렇게 되면 굳이 Menu를 Enum으로 관리할 필요는 없어보여요!

값을 직접 꺼내서 비교하는 것보다 입력 값을 파싱을 통해 비교하도록 하는게 Enum을 더 효과적으로 쓰는 방법이 되겠네요!
반영해보겠습니다!

yuminhwan · 2023-06-19T13:06:25Z

src/main/java/io/Calculator.java

+import java.io.InputStreamReader;
+
+public class Calculator {
+    private final BufferedReader br = new BufferedReader(new InputStreamReader(System.in));


View객체를 따로 구현하지 않은 이유가 있을까요?? (정말 몰라서 여쭤보는 겁니다!)

특별한 이유는 없고 View 객체가 무엇을 의미하는지 조금 헷갈려서 제대로 분리하지 못한 것 같습니다 😢
여기서 View 객체의 역할을 계산기 프로그램에서 사용자와 인터렉션 하는 부분으로 지정하고 분리해보겠습니다!

yuminhwan · 2023-06-19T13:08:12Z

src/main/java/computation/PostfixConverter.java

+import java.util.Stack;
+
+public class PostfixConverter implements Converter {
+    private final StringBuilder sb = new StringBuilder();


Stack자료구조와 StringBuilder를 함께 사용하고 있는데요.
굳이 StringBuilder를 사용해야 할까? 라는 생각이 드네요.
문자열을 이용하신 이유가 있을까요?

convert 메소드는 입력 받은 문자열을 한글자씩 가져와서 처리를 하는데, 이 때 두 자리 이상의 숫자가 들어오는 경우
postfix 리스트에 하나의 숫자로 묶어서 저장하기 위해서 사용했습니다.
음.. StringBuilder 없이 Stack 만으로도 처리가 가능한 방법에 대해 고민해보겠습니다 🤔

yuminhwan · 2023-06-19T13:09:12Z

src/main/java/repository/CalculatorHistoryRepository.java

+public class CalculatorHistoryRepository implements Repository {
+    private static final List<String> repository = new ArrayList<>();
+    private final StringBuilder sb = new StringBuilder();
+
+    @Override
+    public void inquiry() {
+        repository.forEach(System.out::println);
+    }
+
+    @Override
+    public void save(String record, int result) {
+        sb.setLength(0);
+        sb.append(record);
+        sb.append("=");
+        sb.append(result);
+
+        repository.add(sb.toString());
+    }
+}


현재 저장소인 repostiroy에서 View의 역할도 함께 하는 것 같은데요.
영훈님은 repository에게 어떤 역할을 생각하고 구현하셨을까요?

View 객체의 역할을 제대로 분리하지 못해서 생긴 문제였던 것 같습니다..!
말씀해주신 것처럼 repository는 오직 저장과 저장된 값을 반환해주는 기능 등 저장소의 역할만 수행하도록 구성하는 것이 좋은 방향이라고 생각합니다 😭
피드백 감사합니다!

yuminhwan · 2023-06-19T13:10:35Z

src/test/java/computation/PostfixComputerTest.java

+    @Test
+    @DisplayName("기본 사칙 연산 테스트")
+    void basicCalculationTest() {
+        String infixExpr = "3 + 4";
+        int result = postfixComputer.calculate(converter.convert(infixExpr));
+        assertThat(result).isEqualTo(7);
+
+        infixExpr = "3 - 4";
+        result = postfixComputer.calculate(converter.convert(infixExpr));
+        assertThat(result).isEqualTo(-1);
+
+        infixExpr = "3 * 4";
+        result = postfixComputer.calculate(converter.convert(infixExpr));
+        assertThat(result).isEqualTo(12);
+
+        infixExpr = "6 / 3";
+        result = postfixComputer.calculate(converter.convert(infixExpr));
+        assertThat(result).isEqualTo(2);
+    }


여러가지 케이스를 테스트하시려는 노력 좋습니다! 💯
대신 코드가 반복되게 되는데요.
junit의 parameterized test를 활용해보셔도 좋을 것 같아요!

@ParameterizedTest 가 서로 다른 결과에 대해서도 처리해줄까라는 의문이 들어서 반복 테스트를 작성했던 것 같습니다!
조금 더 깊게 공부해서 반복을 줄여보겠습니다!

yuminhwan · 2023-06-19T13:11:06Z

src/test/java/computation/PostfixComputerTest.java

+class PostfixComputerTest {
+
+    private final Converter converter = new PostfixConverter();
+    private final PostfixComputer postfixComputer = new PostfixComputer();


크게 상관은 없지만 인터페이스를 활용했으니 인터페이스로 선언해주는 건 어떨까요~?

Converter 부분을 말씀 하신걸까요?!
인터페이스 타입으로 지정하고 구현체를 지정해주었는데 인터페이스로 선언한다고 하면
구현체를 어느 위치에서 지정 해주는게 더 좋은 방향일지 잘 모르겠습니다.. 🤔

yuminhwan

앞으로의 미션이 있으니 요번 미션은 요기까지 진행해도 좋을 것 같아요!
코멘트 남긴 부분은 리팩토링해보시고 머지하셔도 좋구요. 궁금한 점은 의견주셔도 좋습니다!

0923kdh

영훈님~ 스펜서한테 영훈님 소식을 아주 짧게 전해들었어요!
이번 코드리뷰는 여유가 되실 때, 마무리 해주시고, 여유가 안되신다면 바로 머지해주셔도 괜찮습니다!
과제 하느라 고생하셨어요~ lgtm!

B2SIC · 2023-06-20T16:30:59Z

영훈님~ 스펜서한테 영훈님 소식을 아주 짧게 전해들었어요! 이번 코드리뷰는 여유가 되실 때, 마무리 해주시고, 여유가 안되신다면 바로 머지해주셔도 괜찮습니다! 과제 하느라 고생하셨어요~ lgtm!

제가 먼저 소식을 전해드렸어야 했는데 어떻게 전달 해드리는게 좋을지 고민하다가 타이밍을 놓쳤네요 😢
리뷰 감사합니다!

B2SIC added 19 commits June 8, 2023 01:28

init: 프로젝트 초기 세팅

aa3cbc6

chore: .gitignore에 gradle 관련 내용 추가 및 불필요한 파일 제거

7e4578f

chore: .gitignore에 .idea 추가 및 불필요한 파일 제거

6368d93

chore: 빌드 관련 파일 추가

a37c6cd

feat: 메뉴 선택 기능 및 관련 메세지 정의 추가

18c4dbc

feat: 연산자, 피연산자, 괄호 판별을 위한 Validator 추가

339bac9

feat: 후위표기식 변환 및 계산 기능 추가

2dcd810

refactor: 실행 메인부 클래스명 변경

b0839b6

refactor: 실행 메인부 클래스명 재변경

39ff8c7

feat: 메뉴 선택 관련 입출력 기능 및 메시지 정의 추가

9cb8ce8

refactor: 메시지 관련 클래스명 변경에 따른 기존 파일 삭제

6cbe184

feat: 계산 이력 저장 기능 추가

616e910

refactor: Converter 인터페이스 및 계산 기능 분리

a119b9e

chore: 테스트를 위한 AssertJ 의존성 추가

03389be

feat: Validator 기능 추가

45abcdc

test: Validator 메소드 테스트

dd6f3bc

test: 계산 관련 테스트

5305130

test: 저장소 테스트

2a7acd7

docs: README.md 체크 리스트 수정

88049e0

B2SIC added 🔥 매운맛 Team-HiHi labels Jun 11, 2023

B2SIC requested review from yuminhwan and 0923kdh June 11, 2023 09:16

B2SIC self-assigned this Jun 11, 2023

0923kdh reviewed Jun 11, 2023

View reviewed changes

yuminhwan reviewed Jun 12, 2023

View reviewed changes

B2SIC added 4 commits June 15, 2023 20:00

chore: 테스트 관련 의존성(JUnit5) 추가

54792dd

refactor: Application명 변경

f3c524e

refactor: 패키지명 변경 및 static 메소드 분리, 메소드 타입 변경

2cc308c

refactor: 패키지명, 클래스명 변경 및 연산식 분리, 의존성 외부 주입

6fbb043

B2SIC added 12 commits June 15, 2023 20:06

refactor: 패키지명 변경 및 표현식 저장 타입 변경

aae2a9e

refactor: 연산 동작 분리를 위한 Enum 클래스 추가

5edf79d

refactor: 메뉴 메시지 역할 분리

2464e92

refactor: Custom Exception으로 ZeroDivisionException 클래스 추가

4e58371

refactor: 클래스명 변경 및 저장소 타입 변경, 결과값 저장 기능 추가

4167cfc

refactor: 클래스명 변경 및 메뉴 기능별 분리

9e06b24

refactor: 기존 interface 메소드 -> StringUtil 클래스로 이동

3037564

fix: 두자리수 이상의 연산식을 검증 못하는 버그 수정

4f30930

refactor: 클래스명 변경에 따른 테스트 클래스명 변경

5b23467

refactor: @ParameterizedTest 반영 및 올바른 식 검증

5dcd2aa

refactor: 결과 값 저장 추가에 따른 테스트 메소드 변경

99f6ce7

refactor: 메뉴 번호를 Enum 클래스로 관리

ac8794f

B2SIC requested review from 0923kdh and yuminhwan June 17, 2023 12:51

yuminhwan reviewed Jun 19, 2023

View reviewed changes

yuminhwan approved these changes Jun 19, 2023

View reviewed changes

0923kdh approved these changes Jun 19, 2023

View reviewed changes

[4기 - 김영훈] 1~2주차 과제: 계산기 구현 미션 제출합니다. #163

Are you sure you want to change the base?

[4기 - 김영훈] 1~2주차 과제: 계산기 구현 미션 제출합니다. #163

Conversation

B2SIC commented Jun 11, 2023 • edited Loading

📌 과제 설명

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

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

0923kdh 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

B2SIC Jun 15, 2023 • 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

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

B2SIC Jun 15, 2023 • 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

Choose a reason for hiding this comment

yuminhwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B2SIC Jun 15, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B2SIC Jun 15, 2023 • 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

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

B2SIC Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuminhwan left a comment • 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

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

B2SIC Jun 20, 2023 • 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

Choose a reason for hiding this comment

yuminhwan left a comment

Choose a reason for hiding this comment

0923kdh left a comment

B2SIC commented Jun 11, 2023 •

edited

Loading

B2SIC Jun 15, 2023 •

edited

Loading

B2SIC Jun 15, 2023 •

edited

Loading

B2SIC Jun 15, 2023 •

edited

Loading

B2SIC Jun 15, 2023 •

edited

Loading

B2SIC Jun 20, 2023 •

edited

Loading

yuminhwan left a comment •

edited

Loading

B2SIC Jun 20, 2023 •

edited

Loading