[4기-구범모] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #144

BeommoKoo-dev · 2023-06-09T13:16:54Z

계산기 과제2

리뷰 반영사항

.idea/ 폴더는 intelliJ를 이용하면서 만들어지는 폴더인데 gitignore 처리를 해주시면 좋을 것 같아요! → .idea, .gradle 폴더를 .gitignore처리하였습니다.
객체지향 생활 체조 원칙을 준수할 수 있도록 전반적인 수정이 필요할 것 같아요! → 클래스마다 각각의 책임을 생각해 보고, 비슷한 책임을 맡는 것들끼리 같은 package에 넣어주었습니다.
명령(요청)을 하는 객체인 것 같은 데, 꽤 많은 일을 하고 있어요!

입력을 받기 위한 준비, 출력, 출력해야할 것들에 대한 정보를 가지고 있는 등! 역할을 나눠보시면 좋을 것 같아요! → Controller라는 클래스로 view에 대한 정보를 갖고 있는 객체, 비즈니스 로직(계산)을 처리하는 service객체들을 갖고 있고, view와 service간에 서로의 정보를 모르도록 분리했습니다.

만약, 입출력 방식이 변경된다면(예를 들어 파일로 읽고 json으로 출력을 해준다던지?) 코드 수정이 굉장히 많을 것 같아요!

이런 부분도 역할로 보고 분리하시면 좋을 것 같아요! → Input, Output 인터페이스를 만들어 두고, 구현체로써 ConsoleInput, ConsoleOutput을 만들어 주었습니다. 이는 나중에 input, output형식이 달라질 경우 인터페이스를 상속받아 다른 구현체로 갈아끼기에 유연한 구조라고 생각했습니다.

유사한 테스트의 형태를 paremetrized test로 실행시켰습니다.
처음인 Queue로 저장해도 문제 없겠다는 생각을 했으나, 조회하는 로직에서 pop + push 두가지 불필요한 연산이 들어가 저장소를 Map으로 대체했습니다. 저장소 역시 나중에 Map이 아닌 다른 DB로 바뀔 수 있는 가능성이 있기에 Repository 인터페이스를 선언하여 상속하도록 했습니다.
Calculator에서 사칙 연산자 Enum을 잘 사용하고 계시네요. 그렇다면 어떤 문자 s가 사칙 연산자인지 확인하는 로직은 Enum이 가지고 있어야 하지 않을까요? 자바의 Enum은 단순한 상수의 열거 타입이 아니에요. 완벽한 싱글톤 인스턴스입니다. 따라서 활용도가 매우 높아요 아래 링크를 확인해보고 수정해주세요 → Input으로 입력될 때 해당 연산자가 맞는 연산자인지 판단했습니다.
Memorizer의 역할은 입력 받은 사칙 연산과 그 결과를 저장 및 조회하는 역할인 것 같아요. 그렇다면 결과를 화면에 출력하는 것은 Memorizer의 역할이 아니에요. 결과를 저장, 조회 하는 행위만 할 수 있도록 해주세요 → Output view에서 출력을 담당하고 싶었지만, 동시에 Output 인터페이스의 showHistory()메소드에서 다형성(showHistory의 인자로 들어가는 Repository의 다형성)을 이용하고 싶었기에 인자로 들어가는 repository에서 출력을 하도록 하고, Output view에서는 repository의 출력 함수를 감싸주는 역할만 했습니다.
싱글턴은 애플리케이션에서 인스턴스가 단 하나죠. 그렇다면 여러 스레드가 하나의 인스턴스를 공유하는 것이에요. 이때 싱글턴 인스턴스의 상태(클래스 멤버)가 가변이라면 문제가 발생할 수 있을 것 같습니다. 싱글턴이 가지는 문제점에 공부해보세요 → Calculator 클래스 내의 calculate 메소드에서, 기존에 static을 쓰던 Operator들을 클래스 필드변수로 교체했습니다.

궁금한 점 : 프로그램 종료시 ConsoleOutput 클래스에서 자원회수를 했습니다.(scanner.close) 자원생성의 주체는 ConsoleInput클래스가 담당했는데, 자원정리를 타 클래스인 ConsoleOutput클래스에서 해주어도 되나요 ? 아니면 exitProgram()하기 전에 ConsoleInput클래스에서 자원회수를 담당하는 것이 좋을까요??

src/main/java/controller/Controller.java

src/main/java/service/Operator.java

src/main/java/view/ConsoleInput.java

seung-hun-h

추가 리뷰 남겼습니다. 확인해주세요

src/main/java/controller/Controller.java

seung-hun-h · 2023-06-11T14:18:08Z

src/main/java/domain/Command.java

+    public String[] commandArr;
+    public int[] numberArr;
+    public Operator[] optArr;
+    public int optCount;


private으로 변경이 필요해 보입니다!

private로의 변경 이유는, 클라이언트 코드에서 함부로 command의 내용을 갖고오거나, 변경을 막기 위함일까요??

seung-hun-h · 2023-06-11T14:19:32Z

src/main/java/domain/Command.java

+    public String makeHistory(int result) {
+        return String.join(" ", this.commandArr) + " = " + result;
+    }


Command가 history 생성 책임을 가지는 이유가 뭔가요?

seung-hun-h · 2023-06-11T14:20:28Z

src/main/java/domain/Menu.java

+
+    private final int menuNum;
+    private final String menuName;
+    public static int menuCount = 3;


Suggested change

public static int menuCount = 3;

public static final int MENU_COUNT = 3;

seung-hun-h · 2023-06-11T14:22:13Z

src/main/java/service/Calculator.java

+    Operator PM;
+    Operator MD;


Suggested change

Operator PM;

Operator MD;

private Operator PM;

private Operator MD;

seung-hun-h · 2023-06-11T14:23:16Z

src/main/java/service/Calculator.java

+    private void stackPush(int number, Stack<Integer> st) {
+        if(st.isEmpty()) {
+            st.push(number);
+            return;
+        }
+        int top = st.peek();
+        st.pop();
+        st.push(MD.calculateOpt(top, number));
+    }


stack에 push 하는 것 외 다른 일도 하는 것 같네요. 이름을 변경해줄 필요가 있을 것 같습니다

seung-hun-h · 2023-06-11T14:24:15Z

src/main/java/service/Calculator.java

+        int[] numberArr = command.numberArr;
+        Operator[] optArr = command.optArr;
+
+        Stack<Integer> st = new Stack<>();


자바는 일반적으로 변수명을 줄이지 않습니다.

Suggested change

Stack<Integer> st = new Stack<>();

Stack<Integer> stack = new Stack<>();

seung-hun-h · 2023-06-11T14:26:14Z

src/main/java/service/Operator.java

+    public boolean decideToCalculate() {
+        if(this == PLUS || this == MINUS)
+            return true;
+        return false;
+    }


decideToCalculate는 PLUS, MINUS인 경우만 true를 반환하고 있는데, MULTIPLY, DIVIDE인 경우에는 false를 반환하고 있네요. 메서드 이름만 보면 곱하기 나누기는 연산을 지원하지 않는 것 같아요.

seung-hun-h · 2023-06-11T14:29:38Z

src/main/java/util/PrintOutMessage.java

+public class PrintOutMessage {
+
+    public static final String EXIT_MESSAGE = "EXIT calculate ..";
+    public static final String EMPTY_MESSAGE = "Repository is EMPTY!!";
+
+}


상수 클래스를 사용한 이유가 있나요. 두 메시지 모두 각각 한 클래스에서만 사용 하네요

BeommoKoo-dev · 2023-06-14T15:12:59Z

src/main/java/controller/Controller.java

+
+        while(true) {
+            try {
+                output.printMenu(Menu.values());


말씀해주신 피드백을 반영하여, 기존과 동일하게 printMenu의 책임은 output에 있지만, 인자로 Menu.values를 넣어주어 menu ENUM이 관여하도록 했습니다.

BeommoKoo-dev · 2023-06-14T15:16:18Z

src/main/java/domain/Command.java

+    private String[] commandArr;
+    private int[] numberArr;
+    private Operator[] optArr;
+    private int optCount;
+
+    public Command(String[] commandArr) {
+        this.commandArr = commandArr;
+        this.optCount = commandArr.length / 2;
+        this.numberArr = new int[optCount + 1];
+        this.optArr = new Operator[optCount];
+
+        parseComamand();
+    }
+
+    public String[] getCommandArr() {
+        return commandArr;
+    }
+
+    public int[] getNumberArr() {
+        return numberArr;
+    }
+
+    public Operator[] getOptArr() {
+        return optArr;
+    }
+
+    public int getOptCount() {
+        return optCount;
+    }


말씀하신 Command의 필드들을 private으로 지정하고, 그에 맞게 getter / setter 메소드를 만들어 주었습니다.

BeommoKoo-dev · 2023-06-14T15:18:09Z

src/main/java/domain/History.java

+    public String getHistory() {
+        return history;
+    }
+}


기존에는 Command에서 history를 만드는 책임이 있었지만, 그럴 필요가 없다는 피드백을 수긍하여 History 클래스를 만들어 주고, Controller 클래스에서 한줄로 입력받은 수식(commandArr)와 계산결과로 나온 result를 합쳐 하나의 history를 만들 수 있도록 변경했습니다.

BeommoKoo-dev · 2023-06-14T15:20:18Z

src/main/java/repository/HistoryRepository.java

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


승훈님이 말씀해주신 대로, History를 저장하는 저장소이므로, HistoryRepository로 인터페이스의 이름을 수정했고,
그에따라 구현체 클래스명도 MapHistoryRepository로 변경하였습니다.

BeommoKoo-dev · 2023-06-14T15:20:52Z

src/main/java/service/Calculator.java

+        return result;
+    }
+
+    private void calculateAndPush(int number, Stack<Integer> stack) {


기존에는 stackPush라는 네이밍을 했었는데, 메소드 내에서 push뿐만이 아닌 연산도 하므로 메소드명을 수정했습니다.

메서드가 두 가지 일을 하고 있는 것 같아요. calculate와 push가 분리되면 좋겠네요

BeommoKoo-dev · 2023-06-14T15:22:22Z

src/main/java/service/Operator.java

+        return optName;
+    }
+
+    public boolean isPlusOrMinus() {


승훈님의 피드백을 수긍하여 메소드명 수정했습니다.

BeommoKoo-dev · 2023-06-14T15:24:54Z

src/main/java/view/ConsoleOutput.java

+    private static final String EXIT_MESSAGE = "EXIT calculate ..";
+    public static final String EMPTY_MESSAGE = "Repository is EMPTY!!";


기존의 util 패키지에서 모아뒀던 상수들을 ConsoleOutput클래스로 옮겨주었습니다.

BeommoKoo-dev · 2023-06-14T15:25:52Z

src/main/java/view/ConsoleOutput.java

+    }
+
+    @Override
+    public void showHistory(List<History> historyList) {


MapHistoryRepository에서 반환된 historyList 내의 history를 출력하도록 메소드를 수정했습니다.

showHistory의 파라미터를 HistoryRepository로 전달하여, 해당 Repository 내의 History를 모두 출력하려 했으나, 결국 Output과 Repository는 서로 정보를 몰라야 한다고 생각하여(MVC패턴 기준) historyList를 dto느낌으로 인자로 넣어주었습니다.

하지만 List형식을 받아서 history를 출력한다면 기존의 Map으로 저장했던 history를 출력하는 것보다 성능상 안좋을까요?? (모든 history를 출력하는건 List이든 Map이든 O(n)이지만, map이 key값으로 접근하는 것이므로 List보단 메모리접근? 쪽에서 빠를것 같다는 생각이 듭니다.)

만약 성능상 List가 더 안좋다 하더라도, Output과 Repository는 서로의 정보를 몰라야 되기 때문에 1번의 오버헤드정도는 감수해야 하는 것일까요??

seung-hun-h

범모님 리뷰 남겼습니다.
Scanner 자원 회수는 ConsoleInput에서 이루어지고 있는 것 같은데요?

seung-hun-h · 2023-06-15T12:28:50Z

java-calculator.iml

요것도 .gitignore에 추가해주세요

src/main/java/controller/Controller.java

seung-hun-h · 2023-06-15T12:33:46Z

src/main/java/controller/Controller.java

+                        break;
+
+                    case CALCULATEMENU:
+                        Command command = new Command(input.getLineAndParse());


And를 사용하고 있는 것으로 보아, 메서드가 두 가지 일을 하고 있는 것 같아요

seung-hun-h · 2023-06-15T12:34:49Z

src/main/java/domain/History.java

+public class History {
+    private String history;
+
+    public History(String[] commandArr, int result) {


단순 String 배열보다는 Command 타입을 받는 것이 좋아보이네요

seung-hun-h · 2023-06-15T12:35:36Z

src/main/java/controller/Controller.java

+
+    public void run() {
+
+        while(true) {


의미있는 이름을 가진 변수를 사용하면 좋겠네요.

seung-hun-h · 2023-06-15T12:44:46Z

src/main/java/service/Calculator.java

+    private Operator PM;
+    private Operator MD;


인스턴스 변수로 선언되지 않고, 지역 변수로 충분할 것 같아요. 그리고 자바 네이밍 규칙을 준수해주세요. 상수를 제외한 변수의 이름은 카멜 케이스로 작성해주세요

seung-hun-h · 2023-06-15T12:47:14Z

src/main/java/service/Operator.java

+    private static List<String> toStringArray(Operator[] operators) {
+        return Arrays.stream(operators)
+                .map(Operator::getOptName)
+                .collect(Collectors.toList());
+    }


이 메서드가 Operator에 위치할 필요가 있을까요?

seung-hun-h · 2023-06-15T12:52:58Z

src/main/java/view/ConsoleInput.java

+                || Integer.parseInt(menuCandidate) < 0) {
+            throw new IllegalArgumentException(printError("MENU"));
+        }
+        return getMenu(Integer.parseInt(menuCandidate));


사용자의 입력을 받고 반환만 해주고, 메뉴 객체는 클라이언트 코드에서 해줘도 좋을 것 같아요

seung-hun-h · 2023-06-15T12:56:27Z

src/test/CalculatorTest.java

+
+class CalculatorTest {
+
+    static Calculator calculator;


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

seung-hun-h · 2023-06-15T12:57:22Z

src/test/CommandTest.java

+    static Command command;
+    static int[] compNumArr;
+    static Operator[] compOptArr;
+    static Calculator calculator;
+
+    @BeforeAll
+    public static void setting() {
+        command = new Command(stringToArray("1 + 2 + 3"));
+        compNumArr = new int[] {1, 2, 3};
+        compOptArr = new Operator[] {Operator.PLUS, Operator.PLUS};
+        calculator = new Calculator();
+    }


멤버가 테스트전 딱 한번만 초기화 되므로 다른 테스트에 영향을 줄 것 같아요 @BeforeEach를 사용하도록 변경하는 것이 좋겠네요

BeommoKoo added 13 commits June 5, 2023 03:07

need wiki

d96ea14

project end. need wiki

0390412

테스트케이스 추가

90a46be

calculator test 코드 간결화

247002d

backu

f937877

gitignore test

ef32ad9

Applied gitignore

73e958b

refactoring..

ec0af0a

added .idea to gitignore

4d4ed8a

applied gitignore

bbdccb6

will PR with Wiki

7d3e07f

added .gradle to .gitignore

da1c49f

done

7511b58

BeommoKoo-dev added 💧 순한맛 Team-Seunghoon labels Jun 9, 2023

BeommoKoo-dev requested review from seung-hun-h and dojinyou June 9, 2023 13:16

BeommoKoo-dev self-assigned this Jun 9, 2023

dojinyou reviewed Jun 9, 2023

View reviewed changes

seung-hun-h requested changes Jun 11, 2023

View reviewed changes

BeommoKoo added 2 commits June 14, 2023 17:41

refactoring end

7b75557

method / class renaming

2eb3acb

BeommoKoo-dev commented Jun 14, 2023

View reviewed changes

seung-hun-h requested changes Jun 15, 2023

View reviewed changes

chore : git clean 전 백업

4bd94e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4기-구범모] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #144

[4기-구범모] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #144

BeommoKoo-dev commented Jun 9, 2023

seung-hun-h left a comment

seung-hun-h Jun 11, 2023

BeommoKoo-dev Jun 13, 2023

seung-hun-h Jun 11, 2023

seung-hun-h Jun 11, 2023

seung-hun-h Jun 11, 2023

seung-hun-h Jun 11, 2023

seung-hun-h Jun 11, 2023

seung-hun-h Jun 11, 2023

seung-hun-h Jun 11, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023 •

edited

Loading

seung-hun-h Jun 15, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023

BeommoKoo-dev Jun 14, 2023

seung-hun-h left a comment

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

seung-hun-h Jun 15, 2023

	public static int menuCount = 3;
	public static final int MENU_COUNT = 3;

	Stack<Integer> st = new Stack<>();
	Stack<Integer> stack = new Stack<>();

		private static final String EXIT_MESSAGE = "EXIT calculate ..";
		public static final String EMPTY_MESSAGE = "Repository is EMPTY!!";

[4기-구범모] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #144

Are you sure you want to change the base?

[4기-구범모] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #144

Conversation

BeommoKoo-dev commented Jun 9, 2023

계산기 과제2

리뷰 반영사항

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeommoKoo-dev Jun 14, 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

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

BeommoKoo-dev Jun 14, 2023 •

edited

Loading