Skip to content
Open
7 changes: 4 additions & 3 deletions src/main/java/com/programmers/App.java
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
package com.programmers;

import com.programmers.engine.JavaCalculator;
import com.programmers.engine.model.Result;
import com.programmers.engine.model.ResultManager;
import com.programmers.engine.module.BasicCalculator;

public class App {
public static void main(String[] args) {
Console console = new Console();
Result result = new Result();
ResultManager resultManager = new ResultManager();
BasicCalculator bc = new BasicCalculator();

new JavaCalculator(console, console, result, bc).run();
new JavaCalculator(console, console, resultManager, bc).run();
}
}
70 changes: 0 additions & 70 deletions src/main/java/com/programmers/BasicCalculator.java

This file was deleted.

17 changes: 17 additions & 0 deletions src/main/java/com/programmers/Console.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.programmers.engine.io.Input;
import com.programmers.engine.io.Output;

import java.util.Map;
import java.util.Scanner;

public class Console implements Input, Output {
Expand All @@ -28,4 +29,20 @@ public String getExpression() {
System.out.println("계산하고자 하는 식을 입력해주세요!");
return sc.nextLine();
}

@Override
public void readAllResults(Map<Integer, String> map) {
if (map.isEmpty()) {
System.out.println("계산 이력이 없습니다.");
return;
}
System.out.println();
map.forEach((key, value) -> System.out.println(value));
System.out.println();
}

@Override
public void printAnswer(int answer) {
System.out.println(answer);
}
}
29 changes: 20 additions & 9 deletions src/main/java/com/programmers/engine/JavaCalculator.java
Original file line number Diff line number Diff line change
@@ -1,29 +1,40 @@
package com.programmers.engine;

import com.programmers.BasicCalculator;
import com.programmers.engine.module.BasicCalculator;
import com.programmers.engine.io.Input;
import com.programmers.engine.io.Output;
import com.programmers.engine.model.Result;
import com.programmers.engine.model.Menu;
import com.programmers.engine.model.ResultManager;
import lombok.AllArgsConstructor;

@AllArgsConstructor
public class JavaCalculator implements Runnable{
private final Input input;
private final Output output;
private final Result result;
private final ResultManager resultManager;
private final BasicCalculator bc;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

output.showMenu();
switch (input.selectMenu()) {
case 1 -> result.readAllResults();
case 2 -> result.save(bc.doCalculate(input.getExpression()));
case 3 -> isExecutable = false;
Menu menu = Menu.matchMenu(input.selectMenu());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


switch (menu) {
case LOOK_UP -> output.readAllResults(resultManager.readAllResults());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Comment on lines +23 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
4 changes: 4 additions & 0 deletions src/main/java/com/programmers/engine/io/Output.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package com.programmers.engine.io;

import java.util.Map;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

void printAnswer(int answer);
}
22 changes: 22 additions & 0 deletions src/main/java/com/programmers/engine/model/Menu.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.programmers.engine.model;

import java.util.Arrays;

public enum Menu {
LOOK_UP("1"),
CALCULATE("2"),
EXIT("3");

private final String menu;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Menu(String menu) {
this.menu = menu;
}

public static Menu matchMenu(int number) {
return Arrays.stream(values())
.filter(v -> number == Integer.parseInt(v.menu))
.findFirst()
.orElseThrow(IllegalStateException::new);
}
}
35 changes: 35 additions & 0 deletions src/main/java/com/programmers/engine/model/Operator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.programmers.engine.model;

import lombok.Getter;

import java.util.Arrays;
import java.util.function.BiFunction;

@Getter
public enum Operator {
ADD("+", 1, (a, b) -> a + b),
SUBTRACTION("-", 1, (a, b) -> a - b),
MULTIPLY("*", 2, (a, b) -> a * b),
DIVIDE("/", 2, (a, b) -> b / a);

private final String symbol;
private final int priority;
private final BiFunction<Integer, Integer, Integer> action;

Operator(String symbol, int priority, BiFunction<Integer, Integer, Integer> action) {
this.symbol = symbol;
this.priority = priority;
this.action = action;
}

public static Operator matchOperator(String operator) {
return Arrays.stream(values())
.filter(v -> operator.equals(v.symbol))
.findFirst()
.orElseThrow(IllegalArgumentException::new);
}

public int calculate(int a, int b) {
return action.apply(a, b);
}
}
22 changes: 0 additions & 22 deletions src/main/java/com/programmers/engine/model/Result.java

This file was deleted.

17 changes: 17 additions & 0 deletions src/main/java/com/programmers/engine/model/ResultManager.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package com.programmers.engine.model;

import java.util.HashMap;
import java.util.Map;

public class ResultManager {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private final Map<Integer, String> results = new HashMap<>();
private int key = 0;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

public Map<Integer, String> readAllResults() {
return results;
}
Comment on lines +14 to +16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
16 changes: 16 additions & 0 deletions src/main/java/com/programmers/engine/module/BasicCalculator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.programmers.engine.module;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


import com.programmers.engine.module.convert.AnswerConverter;
import com.programmers.engine.module.convert.PostfixConverter;
import java.util.List;

public class BasicCalculator {
private final PostfixConverter postfixConverter = new PostfixConverter();
private final AnswerConverter answerConverter = new AnswerConverter();

public int doCalculate(String expression) {
List<String> postfixList = postfixConverter.convertInfixToPostfix(expression);
return answerConverter.convertPostfixToAnswer(postfixList);
Comment on lines +9 to +13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package com.programmers.engine.module.convert;

import com.programmers.engine.model.Operator;

import java.util.List;
import java.util.Stack;

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("/")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

st.push(Integer.parseInt(now));
continue;
}
Operator operator = Operator.matchOperator(now);
int x = st.pop();
int y = st.pop();
Comment on lines +17 to +18

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

st.push(operator.calculate(x, y));
}
return st.pop();
}
}
Comment on lines +8 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.programmers.engine.module.convert;

import com.programmers.engine.model.Operator;
import java.util.ArrayList;
import java.util.List;
import java.util.Stack;

public class PostfixConverter {

public List<String> convertInfixToPostfix(String expression) {
String[] strArr = expression.split(" ");
List<String> output = new ArrayList<>();
Stack<String> st = new Stack<>();

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()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

output.add(st.pop());
}
st.push(now);
continue;
}
output.add(now);
}
while (!st.isEmpty()) output.add(st.pop());
return output;
}
}
15 changes: 0 additions & 15 deletions src/test/java/BasicCalculatorTest.java

This file was deleted.

Loading