문자열 계산기 피드백 반영 #1

DevRunner21 · 2022-01-12T17:18:38Z

No description provided.

- serOperators 제거 - addOperator 추가

DongGeon0908

고생하셨습니다!

DongGeon0908 · 2022-01-13T02:28:21Z

src/main/java/StringCalculatorApplication.java

+        Operators operators = new OperatorsQueue();
+        InputNumbers inputNumbers = new InputNumbersList();


클래스 네이밍에 자료형이 들어가면 좋지 않습니다!
추후에 변경이나 확장이 있을 때 자료형 때문에 어려워질 수 있습니다!

OCP 원칙을 지키기 위해 Interface를 사용했는데,
Operators의 구현체 중에 Queue를 사용해서 만든 구현체라는 의미를 주고 싶었습니다.

변경이나 확장이 필요할 경우 다른 자료구조를 사용한 객체로 갈아끼워 실제 비즈니스 로직에서의 소스변경을 없애는걸 의도했죠.
예를 들어 만약 Queue가 아닌 Stack을 사용하도록 변경할 경우 OperatorsStack이라는 클래스를 만들어서 외부에서 주입만 해주는 식으로 생각했습니다.

그래서 OperatorsQueue Class는 내부 로직의 변경이 있더라도 Queue를 사용하는 것은 변함없을 것이라 생각해서
네이밍에 자료구조를 넣게 되었습니다.

그런데 생각해보니 구현에 사용할 자료구조를 바꾸려 할 경우 새로운 Class를 만드는 것 보다
그냥 원래 구현체에서 유연하게 바꾸는게 더 좋겠군요 ㅋㅋ

깔아끼우는 것만 생각하다보니 저런식의 코딩이 된 것 같아요.

피드백 감사합니다~!
@DongGeon0908

DongGeon0908 · 2022-01-13T02:29:14Z

src/main/java/Calculator.java

+    }
+
+    private void validateInputFormula(List<String> splitStrings) {
+        if (splitStrings.size() % 2 == 0) {


매직넘버가 존재하네요~

앗! 수정하겠습니다!

DongGeon0908 · 2022-01-13T02:29:23Z

src/main/java/Calculator.java

+    }
+
+    private void separateQueueData(Queue<String> queue, int position) {
+        if (position % 2 == 0) {


여기도 매직넘버가 존재합니다~

DongGeon0908 · 2022-01-13T02:30:40Z

src/main/java/Calculator.java

+        String formula = input.inputFormula();
+
+        List<String> splittedStrings = Arrays.stream(formula.split(DELIMETER))
+            .collect(Collectors.toList());
+
+        validateInputFormula(splittedStrings);
+
+        Queue<String> queue = new LinkedList<>(splittedStrings);
+        for (int i = 0; i < splittedStrings.size(); i++) {
+            separateQueueData(queue, i);
+        }
+
+        int result = operators.operateAll(inputNumbers.convertToList());
+
+        output.showResult(result);


이부분도 메서드로 나뉠 수 있을 것 같아요!
최대한, 가능한! 메서드로 나누면서 메서드의 길이를 줄여주는게 좋을 것 같습니다!!

물론, 여기서까지 줄이는건 취향차이??랄가

사실 이 부분을 어떻게 나눠야 할지 고민이 많았었는데.. 크흠
한번 해보겠습니다! ㅎㅎ

DongGeon0908 · 2022-01-13T02:31:04Z

src/main/java/Calculator.java

+        } else {
+            operators.addOperator(queue.poll());
+        }


if-else를 사용하지 않고 구현 가능할 것 같습니다!

아! 컨밴션! ㅎㅎ
짚어주셔서 감사합니다!!

DongGeon0908 · 2022-01-13T02:32:05Z

src/main/java/ConsoleInput.java

+
+public class ConsoleInput implements Input{
+
+    private final String REQUEST_INPUT_MESSAGE = "계산식을 입력해주세요";


OUTPUT에서는 메서드 안에서 출력값을 표기하셨는데, INPUT에서는 빼신 이유가 있을까요?

Input에서 메시지를 밖으로 뺸 이유는 관리하기 편하려고 뻈었습니다. ㅎㅎ
Output도 통일성 있게 빼야 할 것 같아요 ㅎㅎ

DongGeon0908 · 2022-01-13T02:33:04Z

src/main/java/InputNumber.java

+    }
+
+    private void validateNumberStr(String numberStr) {
+        if (numberStr.isBlank() || !Character.isDigit(numberStr.charAt(0))) {


validate할 때 조건문은 긍정문으로 시작하는게 좋습니다! !이 들어가는 것보다는 메서드로 정확한 명칭을 주는게 좋아요!

오! 그게 확실히 더 가독성이 좋겠군요!
반영하겠습니다! ㅎㅎ

DongGeon0908 · 2022-01-13T02:33:11Z

src/main/java/InputNumbers.java

@@ -0,0 +1,11 @@
+import java.util.List;
+
+public interface InputNumbers {


굳굳굳굳굳!!!!

DongGeon0908 · 2022-01-13T02:33:39Z

src/main/java/InputNumbersList.java

+        if (inputNumberStr.isEmpty()) {
+            throw new IllegalArgumentException("입력된 숫자가 없습니다.");
+        }


앞과 동일하게 validate method를 빼는게 통일성을 가질 것 같습니다!

DongGeon0908 · 2022-01-13T02:34:35Z

src/main/java/Operator.java

+            .orElseThrow(() -> new IllegalArgumentException("입력된 연산자가 부정확합니다."));
+    }
+
+    public abstract int operate(int preNumber, int postNumber);


굳굳굳입니다!! 추상메서드를 통해서 앞으로 enum을 더 다양한 방법으로 사용할 수 있겠어용

사실 enum을 이렇게 사용하는 것에 대해서 고민이 많았습니다.
일단 Enum을 사용하는 객체와 강결합 되다보니 OCP 원칙을 지키기 어렵겠다는 판단에서 그냥 객체로 다 뺼까도 고민했는데...

깔끔한 코드의 유혹은 벗어나기 힘들더라구요 ㅎㅎ (Enum을 쓰는게 확실히 더 깔끔해져서 ㅎㅎ)

DongGeon0908 · 2022-01-13T02:38:09Z

조금의 수정만 있다면, 더 완벽해질 것 같습니다!!

그리고 TESTCODE를 작성하실 때 조금 더 다양한 조건을 따지는게 좋을 것 같아요~

darkant99 · 2022-01-13T04:00:03Z

src/main/java/Calculator.java

+        this.inputNumbers = inputNumbers;
+    }
+
+    public void runCalculator() {


메서드에 포함되는 클래스명은 생략할 수 있어요 😃
그리고 run() 보다는 calculate() 라는 메서드명이 명확한 의미를 가질것 같아요!

calculate()가 더 직관적이군요!
수정하겠습니다! ㅎㅎ

darkant99 · 2022-01-13T04:10:49Z

src/main/java/OperatorsQueue.java

+
+    private Queue<Operator> operators = new LinkedList<>();
+
+    public Queue<Operator> getOperators() {


operators를 생성자에서 한번에 초기화 하면 어떨까요?

원칙상 생성자에서 값을 초기화 하는게 더 안정적이겠네요 ㅎㅎ
그렇게 수정하겠습니다! 피드백 감사합니다!

darkant99 · 2022-01-13T04:12:46Z

src/main/java/Operators.java

+
+    Operator getNextOperator();
+
+    int operateAll(List<Integer> numbers);


List 대신 InputNumbers를 받아야 할것 같아요 😃

이 부분은 고민하다가
Operators와 InputNumbers 간에 의존성을 없애야 할 것 같다는 생각에 이런식으로 처리하게 되었는데...

생각해보니 Operators 자체에 검증된 값(InputNumbers)이 들어오도록 강제해야 하니까
여기서는 InputNumbers가 들어가야 하는게 맞겠군요!

피드백 감사합니다!

자료구조가 아닌 객체를 받아 이를 활용 해보시면 좋을것 같아요. 👍

darkant99 · 2022-01-13T04:13:53Z

src/main/java/OperatorsQueue.java

+    }
+
+    @Override
+    public Operator getNextOperator() {


public으로 선언된 이유가 테스트를 위해서라면 개선할 필요가 있을것 같아요!

darkant99 · 2022-01-13T04:18:18Z

src/main/java/OperatorsQueue.java

+        if (numbers.isEmpty()) {
+            throw new IllegalArgumentException("계산할 값이 없습니다.");
+        }
+        return numbers.stream()


getNextOperator()로 operator를 가져오지 않고, operator의 stream을 열어 연산을 진행해도 될것 같아요!

여기서 또 stream을 열어버리면 코드가 좀 복잡해보이지 않을까요??

stream을 두번 열지 않고도 다른 방법이 있을것 같아요!
또한 List numbers을 InputNumbers로 변경한다면 numbers의 stream을 열기 위해서 InputNumbers에서 stream을 반환하거나, List를 반환 해야 하는데 두가지 모두 좋지 못한 상황이에요 🤔

darkant99 · 2022-01-13T04:19:28Z

src/test/java/InputNumberTest.java

+import org.junit.jupiter.api.Test;
+
+class InputNumberTest {
+


테스트에 ParameterizedTest를 사용해 테스트 케이스를 늘려보세요!

darkant99 · 2022-01-13T10:28:16Z

src/main/java/Calculator.java

+        this.inputNumbers = inputNumbers;
+    }
+
+    public void calculate() {


이 메서드는 두가지 이상의 일을 하고 있어요.
메서드들이 하나의 일만 하도록 리팩토링 해보시면 좋을것 같아요.

darkant99 · 2022-01-13T10:31:21Z

src/main/java/Operator.java

+
+public enum Operator {
+    PLUS("+") {
+        @Override


람다식을 활용 해보시면 더 간결해질것 같아요 😃

darkant99 · 2022-01-13T10:36:18Z

src/main/java/Operators.java

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


현재 코드에서 일급 컬렉션을 추상화 하는건 과하다는 생각이 들어요🤔
아래 주소를 참고 해보세요!!

https://m.blog.naver.com/PostView.naver?isHttpsRedirect=true&blogId=complusblog&logNo=221163007357

DevRunner21 added 6 commits January 13, 2022 00:34

[docs] ReadMe에 요구사항을 정리하라

c59e3c5

[feat] 숫자와 연산자를 조합하여 결과를 계산하는 로직을 구현하라

32219cf

[refactor] Operator의 메서드를 변경하라

9361646

- serOperators 제거 - addOperator 추가

[feat] 입력한 숫자에 대한 일급객체를 추가하라

4ff0b01

[feat] 입출력 UI용 Class를 정의하라

d725677

[feat] 계산기 로직을 구현하라

1d2b270

DongGeon0908 approved these changes Jan 13, 2022

View reviewed changes

darkant99 reviewed Jan 13, 2022

View reviewed changes

DevRunner21 added 7 commits January 13, 2022 16:40

[refactor] 매직넘버를 상수로 전환하라

863533f

[refactor] 불필요한 else문을 제거하라

ff07771

[refactor] 출력 메시지를 상수로 전환하라

ca8f51b

[refactor] 조건문에서 조건은 긍정문으로 전환하라 (코드 가독성을 위해서)

a4d2560

[refactor] Validation Check로직을 별도의 메서드로 분리하라

e59a1fc

[refactor] 객체의 필드 값 초기화는 생성자에서 진행하도록 수정하라

ef5204e

[refactor] 메서드 명을 더 직관적이게 수정하라

d6f72da

darkant99 reviewed Jan 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

문자열 계산기 피드백 반영 #1

문자열 계산기 피드백 반영 #1

DevRunner21 commented Jan 12, 2022

DongGeon0908 left a comment

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DongGeon0908 Jan 13, 2022

DevRunner21 Jan 13, 2022

DongGeon0908 commented Jan 13, 2022

darkant99 Jan 13, 2022 •

edited

Loading

DevRunner21 Jan 13, 2022

darkant99 Jan 13, 2022

DevRunner21 Jan 13, 2022

darkant99 Jan 13, 2022

DevRunner21 Jan 13, 2022

darkant99 Jan 13, 2022

darkant99 Jan 13, 2022

darkant99 Jan 13, 2022 •

edited

Loading

DevRunner21 Jan 13, 2022

darkant99 Jan 13, 2022 •

edited

Loading

darkant99 Jan 13, 2022

darkant99 Jan 13, 2022

darkant99 Jan 13, 2022

darkant99 Jan 13, 2022

		Operators operators = new OperatorsQueue();
		InputNumbers inputNumbers = new InputNumbersList();


		public class ConsoleInput implements Input{

		private final String REQUEST_INPUT_MESSAGE = "계산식을 입력해주세요";

		@@ -0,0 +1,11 @@
		import java.util.List;

		public interface InputNumbers {


		private Queue<Operator> operators = new LinkedList<>();

		public Queue<Operator> getOperators() {


		Operator getNextOperator();

		int operateAll(List<Integer> numbers);

문자열 계산기 피드백 반영 #1

Are you sure you want to change the base?

문자열 계산기 피드백 반영 #1

Conversation

DevRunner21 commented Jan 12, 2022

DongGeon0908 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

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

DongGeon0908 commented Jan 13, 2022

darkant99 Jan 13, 2022 • 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

darkant99 Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkant99 Jan 13, 2022 • 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

darkant99 Jan 13, 2022 •

edited

Loading

darkant99 Jan 13, 2022 •

edited

Loading

darkant99 Jan 13, 2022 •

edited

Loading