-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kth/계산기 1차 코드 리뷰 #2
base: kth/main
Are you sure you want to change the base?
Conversation
private SelectValidation selectValidation = new SelectValidation(); | ||
|
||
private static InputView inputView = new ConsoleInputView(); | ||
private InputView inputView = new ConsoleInputView(); | ||
|
||
private static ExpressionInputValidation expressionInputValidation = new ExpressionInputValidation(); | ||
private ExpressionInputValidation expressionInputValidation = new ExpressionInputValidation(); | ||
|
||
private static Output output = new ConsoleOutput(); | ||
private Output output = new ConsoleOutput(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 객체들을 외부에서 주입받는 형식이 아닌 기본 생성자만 사용하여 내부에서 생성하는 형태네요. DI를 하지 않은 이유가 있을까요?
private final int CHECK = 1; | ||
private final int CALCULATE = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여긴 static을 지우신 이유가 뭘까요?
@@ -47,7 +47,7 @@ public static void run() { | |||
} | |||
} | |||
|
|||
private static void compute(String expression) { | |||
private void compute(String expression) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 보니 Calculator에 compute기능이 있네요.
compute 내부 메서드를 잘 살펴보시겠어요? 객체에게 메세지를 보내는 것 같나요..?
해당 객체들은 메서드가 호출되어 스택이 올라갈 떄마다 힙영역에 매번 새로운 객체가 저장하겠네요.
이러한 방법이 좋은 방법인 것 같나요?
태훈님 지금 피드백해드리는데 피드백 해주는 것만 보지마시고 저번에 말씀드렸던 것 처럼 태훈님은 전체적인 코드를 다 손봐야해요.
객체지향적으로 구조를 다시 잡으시죠
// private static final String PLUS = "+"; | ||
// private static final String MINIS = "-"; | ||
// private static final String MULTIPLY = "*"; | ||
// private static final String DIVIDE = "/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 주석들은 뭔가요?
Stack<String> operatorStack = new Stack(); | ||
String[] splitExpression = expression.split(" "); | ||
Arrays.stream(splitExpression).forEach(str -> { | ||
String value = str; | ||
int operatorPriority = operatorPriority(value); | ||
Operator operator = Operator.stringToOperator(str); | ||
int operatorPriority = operatorPriority(operator); | ||
int value = Integer.parseInt(str); | ||
if (operatorPriority == -1) { | ||
postfix += value + " "; | ||
} else if (operator.isEmpty()) { | ||
operator.add((value + " ")); | ||
} else if (operatorStack.isEmpty()) { | ||
operatorStack.add((value + " ")); | ||
} else { | ||
while (!operator.isEmpty() | ||
&& operatorPriority(operator.peek().substring(0, 1)) >= operatorPriority) { | ||
postfix += operator.pop(); | ||
while (!operatorStack.isEmpty() | ||
&& operatorPriority(Operator.stringToOperator(operatorStack.peek().substring(0, 1))) | ||
>= operatorPriority) { | ||
postfix += operatorStack.pop(); | ||
} | ||
operator.add((value + " ")); | ||
operatorStack.add((value + " ")); | ||
} | ||
}); | ||
while (!operator.isEmpty()) { | ||
postfix += operator.pop(); | ||
while (!operatorStack.isEmpty()) { | ||
postfix += operatorStack.pop(); | ||
} | ||
return postfix; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
읽기 좋은 메서드란 메서드내에 코드가 5줄이면 가장 읽기 좋고 10줄 이내여야 합니다.
태훈님이 직접 코드를 읽어보세요. 바로 눈에 들어오나요?
public int operatorPriority(Operator operator) { | ||
switch (operator) { | ||
case "+": | ||
case "-": | ||
case PLUS: | ||
case MINUS: | ||
return 1; | ||
case "*": | ||
case "/": | ||
case MULTIPLY: | ||
case DIVIDE: | ||
return 2; | ||
} | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각 상수 객체 별로 반환되어야 하는 값이 정해져있는 것 같아요.
이러한 값들도 enum 안에 넣는게 어떤가요?
public static boolean checkExpressionValue(String expression) { | ||
Matcher matcher = EXPRESSIONREGEX.matcher(expression); | ||
if (!matcher.matches()) { | ||
throw new IllegalArgumentException("잘못된 식 입력입니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
띄어쓰기
public static boolean checkSelectValue(String select) { | ||
Matcher matcher = OPTIONREGEX.matcher(select); | ||
if (!matcher.matches()) { | ||
throw new IllegalArgumentException("잘못된 옵션 선택입니다"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
띄어쓰기
public static boolean checkOperatorValue(String operator){ | ||
Matcher matcher = OPERATORREGEX.matcher(operator); | ||
if (!matcher.matches()) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
띄어쓰기
firstOperand = result.pop(); | ||
secondOperand = result.pop(); | ||
result.push(secondOperand + firstOperand); | ||
break; | ||
case MINUS: | ||
firstOperand = result.pop(); | ||
secondOperand = result.pop(); | ||
result.push(secondOperand - firstOperand); | ||
break; | ||
case MULTIPLY: | ||
firstOperand = result.pop(); | ||
secondOperand = result.pop(); | ||
result.push(secondOperand * firstOperand); | ||
break; | ||
case DIVIDE: | ||
firstOperand = result.pop(); | ||
secondOperand = result.pop(); | ||
result.push(secondOperand / firstOperand); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
읽어보시면 중복되는 로직이고 이전에 연산자에 대한 연산 로직도 구현을 해두셨던 것 같은데 괴리감이 느껴지지않나요?
|
||
public class InfixToPostfixConverter { | ||
|
||
private String postfix = ""; | ||
private StringBuilder postfix = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변수명이 무엇을 저장하는건지 와닿지 않아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?? 커밋이 InputView OutputView 통합인데 통합된 부분이 안보이는데요..?
"100 200 + 300 + : 600"}, delimiter = ':') | ||
public void postFixAddCalculate(String expression, int expectResult) { | ||
Accumulator postFixAccumulator = new PostFixAccumulator(); | ||
int result = postFixAccumulator.calculate(expression); | ||
Assertions.assertEquals(expectResult, result); | ||
} | ||
|
||
@ParameterizedTest | ||
@DisplayName("뺼셈") | ||
@CsvSource(value = {"1 2 - 3 -: -4", "10 30 - 20 -: -40", "300 200 - 1 - : 99"}, delimiter = ':') | ||
public void postFixMinusCalculate(String expression, int expectResult) { | ||
Accumulator postFixAccumulator = new PostFixAccumulator(); | ||
int result = postFixAccumulator.calculate(expression); | ||
Assertions.assertEquals(expectResult, result); | ||
} | ||
|
||
@ParameterizedTest | ||
@DisplayName("곱셈") | ||
@CsvSource(value = {"1 2 * 3 *: 6", "10 20 * 30 *: 6000", | ||
"100 200 * 300 * : 6000000"}, delimiter = ':') | ||
public void postFixMultiplyCalculate(String expression, int expectResult) { | ||
Accumulator postFixAccumulator = new PostFixAccumulator(); | ||
int result = postFixAccumulator.calculate(expression); | ||
Assertions.assertEquals(expectResult, result); | ||
} | ||
|
||
@ParameterizedTest | ||
@DisplayName("나눗셈") | ||
@CsvSource(value = {"100 10 / 1 / : 10", "10 2 / : 5", "10000 20 / 10 / : 50"}, delimiter = ':') | ||
public void postFixDivideCalculate(String expression, int expectResult) { | ||
Accumulator postFixAccumulator = new PostFixAccumulator(); | ||
int result = postFixAccumulator.calculate(expression); | ||
Assertions.assertEquals(expectResult, result); | ||
} | ||
|
||
@ParameterizedTest | ||
@DisplayName("사칙연산") | ||
@CsvSource(value = {"5 3 2 * + 8 4 / - : 9", "7 4 * 2 / 3 + 1 - : 16", | ||
"9 5 - 2 * 6 3 / +: 10"}, delimiter = ':') | ||
public void PostFixCalculate(String expression, int expectResult) { | ||
PostFixAccumulator calculator = new PostFixAccumulator(); | ||
int result = calculator.calculate(expression); | ||
Assertions.assertEquals(expectResult, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given when then 분리해주세요
|
||
public class Repository { | ||
|
||
private List<String> list = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 이건 리스트로 그대로 가져가실 생각이신가봐요
} | ||
} | ||
|
||
private void selectOptions(ConsoleInput input, int select) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 명은 분명히 selectOptions 무언가 반환하는게 아닌 내부에서 행위를 수행하고 있네요.
selectInput = bufferedReader.readLine(); | ||
} catch (IOException e) { | ||
throw new IllegalArgumentException("잘못된 옵션 선택입니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOException이 뭔가요? 여기서 어떻게 잘못된 옵션 선택인지 알 수 있나요?
expressionInput = bufferedReader.readLine(); | ||
} catch (IOException e) { | ||
throw new IllegalArgumentException("잘못된 식 입력입니다."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 동일하네요
for (Operator operator : Operator.values()) { | ||
if (operator.getValue().equals(value)) { | ||
return operator; | ||
} | ||
} | ||
throw new IllegalArgumentException("잘못된 입력값 입니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 스트림으로 변경 할 수 있지 않을까요?
|
||
@Override | ||
public void print(List<String> result) { | ||
result.stream().forEach(System.out::println); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 한줄에는 점 하나 지켜주세요
private List<String> list = new ArrayList<>(); | ||
|
||
public void store(String expression, String result) { | ||
StringBuilder formattedExpression = new StringBuilder(expression).append(" = ").append(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 한 줄에 점하나 지켜주세요.
|
||
public class PatternValidator { | ||
|
||
private static final Pattern EXPRESSIONREGEX = Pattern.compile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수 표기시 단어 분리 시점은 _ 로 분리합니다.
ex ) EXPRESSION_REGEX
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.CsvSource; | ||
|
||
public class ChangToPostfixTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스 명이 잘못되었네요. 또한 ChangeToPostfixTest로 할 의도였다면 저번에도 얘기했지만 클래스명은 명사입니다.
|
||
private Repository repository; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쓸데없는 개행
}); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쓸데없는 개행
} | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쓸데없는 개행
System.out.print("선택 : "); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쓸데없는 개행
private void processToken(Stack<String> operatorStack, String token) { | ||
if (PatternValidator.checkOperatorValue(token)) { | ||
handleOperator(operatorStack, token); | ||
} else if (!PatternValidator.checkOperatorValue(token)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅎㅎ.. 이 코드가 if else 랑 뭐가 다른가요
public static void print(Exception e) {
System.out.println(e.getMessage());
} 정도로 가능하지 않을까싶어요.
코딩에 대한 거부감이 너무 강하게 들었습니다.
또한 경험의 부족인건지 잘못된 공부법인지 모르겠는데 공부한 부분을 코드로 나타내기 힘들다고 느꼈습니다.
질문이 많아서 얘기가 길어졌는데 태훈님이 지금 위에서 말씀하신 부족하다고 생각하는 부분. 그런 부분을 찾아서 공부하세요. 다만 디자인 패턴은 양이 많아요. 그리고 디자인 패턴을 익히기 가장 좋은 방법은 직접 코드로 해당 디자인 패턴을 안 보고 어디에 그림을 그려보고 코드로 구현해보는 겁니다. 자료구조를 처음 공부 할 때를 떠올리면 좋겠네요. 지금 태훈님에게 가장 중요한건 기존의 습관들을 버리고 새로운 습관들이 자리잡도록 하는 겁니다. 만약에 이게 너무 하기 싫다. -> 저 개발자 하기 싫어요. 랑 다를게 없습니다. |
@@ -14,7 +14,7 @@ public void store(String expression, String result) { | |||
} | |||
|
|||
public List<String> getResult() { | |||
return list; | |||
return new ArrayList<>(list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new ArrayList를 이용하여 반환해주는 것도 좋지만 Collections.unModifiableList(list);
list.stream().toList(); 라는 것도 있습니다. 공부해보면 좋을 것 같아요.
📌 과제 설명
선택 옵션 입력 : 1 :결과 조회 2 계산
1번을 선택했을 시 이전 결과를 전부 조회 할 수 있습니다.
2번을 선택했을 시 중위 표현식을 후위 표현식으로 변환하여 계산하고 저장합니다
👩💻 요구 사항과 구현 내용
객체를 크게 계산기, 누산기, 입력, 입력 뷰, 출력, 저장소로 나눴습니다.
입력 뷰와 출력은 콘솔이 아닌 다른 방식의 확장을 가정하여 인터페이스로 구현했습니다.
옵션 입력 try catch를 이용한 예외 처리와 정규식을 통한 화이트 리스트 방식으로 검증을 진행하였습니다.
식 입력 또한 try catch를 이용한 예외 처리와 정규식을 통한 화이트 리스트 방식으로 검증을 진행하였습니다.
식 입력 시 중위 표기법을 후기 표기법으로 변경하여 계산하는 방식으로 구현했습니다.
저장은 LinkedHashMap을 통해 구현했습니다.
테스트는 @ParameterizedTest를 이용하여 다양한 케이스의 예제를 테스트 했습니다.
커밋이 뒤죽박죽이라 아직 커밋 별로 정리하기는 좀 어려운거 같습니다. 익숙해 지도록 하겠습니다!
✅ 피드백 반영사항
맞춤법검사기 결과 영역
✅ PR 포인트 & 궁금한 점
ConsoleInput Class에서 잘못된 입력 값이 들어왔을 때 서로 다른 메시지를 주기 위해서 같은 로직인 메서드를 2개 만들었습니다. 반복적인 코드가 좋지 않다고 생각이 들었는데요. 이걸 한 개의 메서드로 바꾸고 매개변수를 추가해서 잘못된 코드가 들어왔을 때 메시지를 추가해 주는 게 좋을까요?
잘못된 입력이 들어왔을 때 IllegalArgumentException을 던지는데 사실 저는 잘못된 매개변수 같은 경우는 컴파일 시점에서 다 잡아주니 저 예외보단 커스텀 예외를 만들어서 처리하는 게 더 직관적이라고 생각이 드는데 직렬화 문제와 실무에서 혼잡성 문제가 있다고 하셔서 사용하지 않았는데 뭐가 더 좋을까요?
람다를 사용하라고 하셨는데 간단하고 재사용성도 좋지만 함수형 인터페이스를 사용해서 람다를 쓰면 오히려 유지 보수에 안 좋지 않나요? 혹시 람다 사용을 추천해 주신 이유가 궁금합니다
피드백 해주셨을 때 코드 한줄 한줄 근거를 가지고 짜야 된다라고 말해주셨는데 기존엔 그냥 구현에만 중점을 두고 객체지향이란 점을 생각을 하지 않고 짰습니다. 리팩토링 하는 과정에서 제가 객체지향, SOLID, 디자인 패턴의 개념이 너무 부족하기도 하고 습관을 한순간에 바꾸려고 하는 부분에서 코딩에 대한 거부감이 너무 강하게 들었습니다. 또한 경험의 부족인건지 잘못된 공부법인지 모르겠는데 공부한 부분을 코드로 나타내기 힘들다고 느꼈습니다.좋은 방법 아시면 추천 부탁드려요! 조금씩 피드백 받으면서 변화해보도록 하겠습니다!