[4기 - 전선희] 1~2주차 과제 : 계산기 구현 미션 제출합니다. (2차) #172

funnysunny08 · 2023-06-12T15:18:23Z

📌 과제 설명

1~2주차 강의에서 배운 내용들을 최대한 활용할 수 있도록 계산기 구현하였습니다!

더불어 코드리뷰 사항 반영해 보았습니다!

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

요구 사항

객체지향적인 코드로 계산기 구현하기
- 더하기
- 빼기
- 곱하기
- 나누기
- 우선순위(사칙연산)
테스트 코드 구현하기
계산 이력을 맵으로 데이터 저장기능 만들기
(선택) 정규식 사용

구현 내용

📁 engine 
|_ 📁 io 
    |_ 🗂 Input.java
    |_ 🗂 Output.java
|_ 📁 model
    |_ 🗂 Menu.java
    |_ 🗂 Operator.java
    |_ 🗂 ResultManager.java
|_ 📁 module
    |_ 📁 convert
        |_ 🗂 AnswerConverter.java
        |_ 🗂 PostfixConverter.java
    |_ 🗂 BasicCalculator.java
|_ 🗂 JavaCalculator.java    
🗂 App.java 
🗂 Console.java

입출력: Input, Output 인터페이스를 Console에서 구현하여 메뉴 입력받기, 수식 입력받기, 메뉴 출력하기, 에러 메시지 출력하기 기능을 구현했습니다.
계산 결과값 저장 및 출력: ResultManager에서 계산 결과 값을 저장할 map을 선언하고 결과값 저장하기, 결과값들 출력하기 기능을 구현했습니다.
계산
- PostfixConverter에서 중위표기식을 후위표기식(list)로 바꿔줍니다
- AnswerConverter에서 후위표기식을 답으로 변환하여 리턴합니다.
- BasicCalculator에서 입력받은 식에 해당하는 답을 반환합니다.
계산기 실행: JavaCalculator에서 계산기를 실행할 수 있도록 하였습니다.

✅ 피드백 반영사항

출력하는 행위 모두 Console로 이동
Result 클래스 적절한 이름으로 변경 -> ResultManager.java
switch문을 통한 메뉴 선택에서 매직 넘버를 상수로 변경
객체지향생활체조원칙 (else문 사용 X, indent 지키기)
BasicCalculator 파일 위치 변경 -> /engine/BasicCalculator.java
BasicCalcualtor 분리 -> PostfixConverter, AnswerConverter
Menu, Operator -> Enum
toPostfix() 메서드명 변경 -> convertInfixToPostfix()
인덱스로 접근할 필요 없는 for문 foreach문으로 변경
추상클래스로 선언하기 ArrayList<String> -> List<String>
계산 결과 저장하는 void() 함수에서 삭제할 때 발생하는 key 중복 대비하기

✅ PR 포인트 & 궁금한 점

객체지향적으로 구현이 잘 된걸까요..?🥲
~~Result.java에서 결과값을 담을 map을 선언하고 관련 함수들을 같이 선언했는데 이게 좋은 방법인지 궁금합니다!~~
~~제가 테스트 코드에 익숙하지 않아 계산기 구현 먼저 PR 올렸습니다! 테스트도 더 추가해서 빠르게 올리겠습니다!!!~~

dojinyou · 2023-06-12T20:03:36Z

src/main/java/com/programmers/engine/JavaCalculator.java

-                case 1 -> result.readAllResults();
-                case 2 -> result.save(bc.doCalculate(input.getExpression()));
-                case 3 -> isExecutable = false;
+            Menu menu = Menu.matchMenu(input.selectMenu());


한줄로 쭉 쓰시면 input.selectMenu() 값을 디버깅하고 확인하기 어려울 것 같아요!

matchMenu라는 메서드의 동사는 match인데 이건 일치하는 지 확인하는 함수처럼 보여서 boolean 값을 반환할 거 같아요. 정적 팩토리 메서드 네이밍을 찾아보시고 참고하시면 좋을 것 같아요.

dojinyou · 2023-06-12T20:06:42Z

src/main/java/com/programmers/engine/JavaCalculator.java

+            switch (menu) {
+                case LOOK_UP -> output.readAllResults(resultManager.readAllResults());
+                case CALCULATE -> calculate();
+                case EXIT -> {
+                    return;
+                }
                default -> output.inputError();
            }
        }
    }
+
+    private void calculate() {
+        String expression = input.getExpression();
+        int answer = bc.doCalculate(expression);
+        output.printAnswer(answer);
+        resultManager.save(expression, answer);


실행되는 application과 계산하는 calculator가 한 클래스에 같이 있는 것 같아요!

dojinyou · 2023-06-12T20:07:58Z

src/main/java/com/programmers/engine/io/Output.java

 public interface Output {
    void showMenu();
    void inputError();
+    void readAllResults(Map<Integer, String> map);


Map은 다양한 형태의 값이 들어올 수 있을 것 같아요! 이런 부분에 대한 검증을 할 수 있는 클래스로 변경하거나 검증 로직이 있어야할 것 같아요!

dojinyou · 2023-06-12T20:09:20Z

src/main/java/com/programmers/engine/model/Menu.java

+    CALCULATE("2"),
+    EXIT("3");
+
+    private final String menu;


Mene라는 클래스의 menu라는 필드는 네이밍이 적절해 보이지 않아요! 아래에서 number와 변수명과 동일성을 비교하기 떄문에 관련해서 유사한 네이밍을 가져가면 좋을 것 같아요!

dojinyou · 2023-06-12T20:11:04Z

src/main/java/com/programmers/engine/model/ResultManager.java

+    private int key = 0;
+
+    public void save(String expression, int answer) {
+        results.put(++key, expression + " = " + String.valueOf(answer));


[nit]
++key로 id값을 넣어줄 때는 멀티 쓰레드 환경이라면 동시에 같은 값이 들어갈 수 있을 것 같아요!
자바 내부적으로 할 수 있는 동시성 제어에 대해서 알아보셔도 좋을 것 같아요!

dojinyou · 2023-06-12T20:13:16Z

src/main/java/com/programmers/engine/module/BasicCalculator.java

+    private final AnswerConverter answerConverter = new AnswerConverter();
+
+    public int doCalculate(String expression) {
+        List<String> postfixList =  postfixConverter.convertInfixToPostfix(expression);
+        return answerConverter.convertPostfixToAnswer(postfixList);


식에서 답으로 구하는 과정을 일반적으로 계산이라고 표현하는 것 같은 데 이걸 변환이라고 해서 그런 지 AnswerConverter 라는 개념이 잘 와닿지 않는 것 같아요.

dojinyou · 2023-06-12T20:13:48Z

src/main/java/com/programmers/engine/module/convert/AnswerConverter.java

+public class AnswerConverter {
+    public int convertPostfixToAnswer(List<String> list) {
+        Stack<Integer> st = new Stack<>();
+        for (String now : list) {
+            if (!now.equals("+") && !now.equals("-") && !now.equals("*") && !now.equals("/")) {
+                st.push(Integer.parseInt(now));
+                continue;
+            }
+            Operator operator = Operator.matchOperator(now);
+            int x = st.pop();
+            int y = st.pop();
+            st.push(operator.calculate(x, y));
+        }
+        return st.pop();
+    }
+}


static하게 만들지 않은 이유가 있을까요?

dojinyou · 2023-06-12T20:17:00Z

src/test/java/CalculatorTest.java

+    @DisplayName("계산기 테스트")
+    @Test
+    void basicCalculatorTest() {
+        String str = "3 + 2 + 4 * 5 + 3 / 1";
+        int result = bc.doCalculate(str);
+        assertThat(result).isEqualTo(28);
+    }


잘못된 문장이 들어오거나 공백 없이 들어오는 경우 등에 다른 케이스에 대해서는 테스트를 안하는 이유가 있을까요?
3 / 2 같은 표현에 대해서 계산 결과가 int형이라면 정확하지 않을 것 같은 데 어떻게 생각하시나요?

dojinyou · 2023-06-12T20:17:59Z

src/test/java/MenuTest.java

+        assertThat(Menu.matchMenu(1)).isEqualTo(Menu.LOOK_UP);
+        assertThat(Menu.matchMenu(2)).isEqualTo(Menu.CALCULATE);
+        assertThat(Menu.matchMenu(3)).isEqualTo(Menu.EXIT);


동일한 형태의 코드에서 몇몇 파라미터만 변경되고 있어서 많은 중복이 발생하고 있어요
ParameterizedTest와 EnumSource를 찾아보시면 좋을 것 같아요!

seung-hun-h

안녕하세요 선희님 리뷰남겼습니다.
자바에서는 변수명이나 클래스명을 줄여 사용하지 않습니다. 그리고 의미있는 변수명을 지을 수 있도록 노력하면 좋을 것 같아요.

seung-hun-h · 2023-06-13T14:33:27Z

src/main/java/com/programmers/engine/JavaCalculator.java

+            Menu menu = Menu.matchMenu(input.selectMenu());
+
+            switch (menu) {
+                case LOOK_UP -> output.readAllResults(resultManager.readAllResults());


메서드가 write로 시작하는 것이 맞지 않을까요?

seung-hun-h · 2023-06-13T14:39:24Z

src/main/java/com/programmers/engine/model/ResultManager.java

+    public Map<Integer, String> readAllResults() {
+        return results;
+    }


복사본을 반환해주는 것이 좋아보이네요. 외부에서 Map 객체를 조작하면 내부가 변경될 수 있을 것 같아요.
새로운 Map 인스턴스를 생성하고 데이터를 복사해서 전달하거나 Collections.unmodifiableMap을 사용해보는 것도 좋겟네요

https://lng1982.tistory.com/155

seung-hun-h · 2023-06-13T14:41:44Z

src/main/java/com/programmers/engine/module/convert/AnswerConverter.java

+    public int convertPostfixToAnswer(List<String> list) {
+        Stack<Integer> st = new Stack<>();
+        for (String now : list) {
+            if (!now.equals("+") && !now.equals("-") && !now.equals("*") && !now.equals("/")) {


문자열이 연산자인지 판단하는 로직은 Operator에 있어야 할 거 같네요.
mod 연산이 추가 됐다고 상상을 해보면, Operator와 이곳 두 군데를 수정해야하므로 실수가 발생할 수 있을 것 같아요

seung-hun-h · 2023-06-13T14:44:13Z

src/main/java/com/programmers/engine/JavaCalculator.java

    private final BasicCalculator bc;

    @Override
    public void run() {
-        boolean isExecutable = true;
-        while (isExecutable) {
+        while (true) {


이 부분은 반영전이 더 나은거 같은데 true를 사용하신 이유가 있을까요?

seung-hun-h · 2023-06-13T14:49:41Z

src/main/java/com/programmers/engine/model/ResultManager.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class ResultManager {


ResultManager가 model 패키지에 있는게 어색해보이네요. 영속화를 담당하고 있는 것 같아서요

seung-hun-h · 2023-06-13T14:51:29Z

src/main/java/com/programmers/engine/module/BasicCalculator.java

@@ -0,0 +1,16 @@
+package com.programmers.engine.module;


패키지 이름을 module이라 지으신 이유가 있을까요?
모듈이라고 하면 여러군데에서 공통적으로 사용하는 것들이 모여있을 것 같아서요

seung-hun-h · 2023-06-13T14:53:04Z

src/main/java/com/programmers/engine/module/convert/PostfixConverter.java

+
+        for (String now: strArr) {
+            if (now.equals("+") || now.equals("-") || now.equals("*") || now.equals("/")) {
+                while (!st.isEmpty() && Operator.matchOperator(st.peek()).getPriority() >= Operator.matchOperator(now).getPriority()) {


우선순위를 판단하는 로직도 Operator가 가져야 할 책임인거 같네요

seung-hun-h · 2023-06-13T14:55:03Z

src/test/java/CalculatorTest.java

+    BasicCalculator bc = new BasicCalculator();
+    AnswerConverter ac = new AnswerConverter();
+    PostfixConverter pc = new PostfixConverter();


접근 제어자도 추가해주세요

seung-hun-h · 2023-06-13T14:57:00Z

src/test/java/CalculatorTest.java

+    @DisplayName("정답 변환기 테스트")
+    @Test
+    void answerConverterTest() {
+        List<String> list = new ArrayList<>(Arrays.asList("3", "2", "+", "4", "5", "*", "+", "3", "1", "/", "+"));
+        int answer = ac.convertPostfixToAnswer(list);
+        assertThat(answer).isEqualTo(28);
+    }
+
+    @DisplayName("후위표기 변환기 테스트")
+    @Test
+    void postfixConverterTest() {
+        String str = "3 + 2 + 4 * 5 + 3 / 1";
+        List<String> list = pc.convertInfixToPostfix(str);
+        List<String> ans = new ArrayList<>(Arrays.asList("3", "2", "+", "4", "5", "*", "+", "3", "1", "/", "+"));
+        assertThat(list).isEqualTo(ans);
+    }


이 테스트는 분리된 클래스에 작성해주면 좋겠네요.
계산 결과를 제대로 반환 하는지 테스트 하는 것이 아니라 후위 표기로 변환이 되는지, 후위 표기식에서 정답이 도출 되는 지 판단하는 테스트라서요

seung-hun-h · 2023-06-13T14:58:10Z

src/main/java/com/programmers/engine/module/convert/AnswerConverter.java

+            int x = st.pop();
+            int y = st.pop();


의미있는 변수명을 사용하면 좋겠네요. 순서가 중요하다면 first, second 정도로 지어줄 수 있을 것 같아요

funnysunny08 added 9 commits June 12, 2023 21:37

REFACTOR : 출력 부분 모두 Console로 분리

d793990

REFACTOR : Result 클래스 적절한 이름으로 바꾸기

80ccd94

REFACTOR : 메뉴 선택에서 매직 넘버를 상수로 바꾸기

0fac830

REFACTOR : boolean isExcutable 사용 X

559c4f9

REFACTOR : JavaCalculator에서 calculate 메서드로 분리

e6aa4d4

REFACTOR : BasicCalculator 분리하기, 관련 리팩토링

a150c73

REFACTOR : 불필요한 import 제거

166e131

REFACTOR : save 함수 삭제 문제 관련 수정

04fdc47

FEAT : 테스트 코드 추가

d492c36

funnysunny08 added 🔥 매운맛 Team-Seunghoon labels Jun 12, 2023

funnysunny08 requested review from seung-hun-h and dojinyou June 12, 2023 15:18

funnysunny08 self-assigned this Jun 12, 2023

dojinyou approved these changes Jun 12, 2023

View reviewed changes

seung-hun-h suggested changes Jun 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4기 - 전선희] 1~2주차 과제 : 계산기 구현 미션 제출합니다. (2차) #172

[4기 - 전선희] 1~2주차 과제 : 계산기 구현 미션 제출합니다. (2차) #172

funnysunny08 commented Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

dojinyou Jun 12, 2023

seung-hun-h left a comment

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

seung-hun-h Jun 13, 2023

[4기 - 전선희] 1~2주차 과제 : 계산기 구현 미션 제출합니다. (2차) #172

Are you sure you want to change the base?

[4기 - 전선희] 1~2주차 과제 : 계산기 구현 미션 제출합니다. (2차) #172

Conversation

funnysunny08 commented Jun 12, 2023

📌 과제 설명

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

요구 사항

구현 내용

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

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

seung-hun-h 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