-
Notifications
You must be signed in to change notification settings - Fork 156
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
[4기 - 김예슬] 1~2주차 과제: 계산기 구현 미션 제출합니다. #195
base: kys0411
Are you sure you want to change the base?
Conversation
String expression -> 후위표기식으로 변환
후위표기식으로 변환된 결과가 담긴 class
@@ -0,0 +1,44 @@ | |||
package org.programmers.constant; |
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.
Operator가 enum이긴 하지만,
역할상으로 calculator 패키지에 있는것이 덜 어색해보입니다!
|
||
public class ExpressionValidator { | ||
|
||
public boolean isOperator(String symbol) { |
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.
일반적으로 Validator는 주로 검증할때 사용합니다.
Operator 란 객체가 존재하는데 Operator 인지 여부를 알 수 있는 isOpertator라는 메소드가
Validator에 존재해야 할까? Opertator.isOpertator(symbol) 이 조금 더 응집되어 있다고 생각합니다.
Stack<String> calculateStack = new Stack<>(); | ||
|
||
for (String element : param.getPostfix()) { | ||
if (validator.isOperator(element)) { |
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.
일반적으로 Validator는 주로 검증할때 사용합니다.
Operator 란 객체가 존재하는데 Operator 인지 여부를 알 수 있는 isOpertator라는 메소드가
Validator에 존재해야 할까? Opertator.isOpertator(symbol) 이 조금 더 응집되어 있다고 생각합니다.
@@ -0,0 +1,15 @@ | |||
package org.programmers.constant; | |||
|
|||
public enum ExpressionSymbol { |
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.
LGTM
public abstract ExpressionResult calculate(ExpressionParam param); | ||
|
||
protected double getCalculationResult(String symbol, double operand1, double operand2) { | ||
return Operator.find(symbol).get().getAnswer(operand1, operand2); |
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.
메서드 체이닝 사용시에는
Operator.find(symbol)
.get()
.getAnswer(operand1, operand2);
처럼 메서드마다 개행시켜주는것이 가독성에 더 좋습니다!
return priority; | ||
} | ||
|
||
public double getAnswer(double operand1, double operand2) { |
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.
getAnswer 보다 좋은 네이밍이 있을것 같아요..
답을 얻는다?.. 제가 보았을때는 조금 애매하다고 생각하는데 예슬님은 어떻게 생각하시나용
return input; | ||
} | ||
|
||
public Output getOutput() { |
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.
Console은 입 출력하기 위한 클래스로 보여요.
output에 의존하고 있는데, 출력을 하려면 이 output()을 내보내고 output을 직접 사용하는 것이 좋을까 생각이 듭니다.
output에 의존하는것은 감추고, print 하는 메소드를 만들어 위임시키는 것은 어떻게 생각하세요?
|
||
@Override | ||
public void printAnswer(double answer) { | ||
System.out.println(answer + "\n"); |
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.lineseparator()
라는 메소드가 존재합니다! 한번 알아보시겠어요?
|
||
void printConsole(); | ||
|
||
void printAnswer(double answer); | ||
|
||
void printError(); | ||
|
||
void printHistory(Map<Long, ExpressionResult> history); |
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 interface Input { | ||
|
||
int inputNumber(); | ||
|
||
String inputExpression(); | ||
} |
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.
이 Input 인터페이스도 입력에 대한 추상화 같아요.
구체적으로 기능에 의존하는것보다 범용으로 입력받을 수 있게 고민해보시겠어요!
@Override | ||
public void printHistory(Map<Long, ExpressionResult> history) { | ||
for (long i = history.size() - 1; i >= 0; i--) { | ||
System.out.println(history.get(i).toString()); | ||
} | ||
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.
혹시 왜 역순으로 출력하는지 예슬님의 의도를 알 수 있을까요?
일반적으로 Map은 순서가 보장되지 않는 자료구조입니다.
때문에, printHistory라는 메소드에서 이런 불필요한 로직이 보이는것 같아요.
순서대로 출력해야 한다면
레포지토리에 조건을 주고, 순서를 보장하는 자료구조인 List로 받으면 다음과 같이 줄여 출력만 할 수 있지 않을까요?
@Override
public void printAll(List<ExpressionResult> results) {
Collections.reverse(results); // or printAll을 호출하는 곳에서 reverse해서 넘겨준다면?
for (ExpressionResult result : results) {
System.out.println(result.toString());
}
System.out.println();
}
public boolean isNumber(String number) { | ||
if (number.length() == 1) { | ||
return number.charAt(0) >= '0' && number.charAt(0) <= '9'; | ||
} | ||
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.
혹시라도, Number가 2자 이상이라면 무조건 false를 반환할거같아요
String ten = "10";
String level = "99";
다른 좋은 방법이 없을까요?
private String makeExpression() { | ||
return expression | ||
+ ExpressionSymbol.BLANK_SYMBOL.getSymbol() | ||
+ ExpressionSymbol.EQUAL_SYMBOL.getSymbol() | ||
+ ExpressionSymbol.BLANK_SYMBOL.getSymbol() | ||
+ answer; | ||
} |
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.
좋은데요?
식의 출력에 대한 포맷의 책임을 가져간거같아요.
toString()이 다른곳에 쓰일수도 있다면
public 으로 만들어서 사용해도 될거같아요
|
||
import java.util.List; | ||
|
||
public class ExpressionParam { |
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.
LGTM
} | ||
} | ||
|
||
private void addToPostfix(List<String> postfix, String exp) { |
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.
exp 라는 파라미터 이름이 모호해 보입니다.
구체적으로 작성하면 읽을때 혼란이 없을것 같아요
import org.programmers.expression.ExpressionParam; | ||
import org.programmers.expression.ExpressionResult; | ||
|
||
public class CalculatorManagement { |
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.
LGTM
import org.programmers.io.Console; | ||
import org.programmers.repository.Repository; | ||
|
||
public class AppController { |
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 getAllAndPrint() { | ||
console.getOutput().printHistory(repository.getAll()); |
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.
console이 의존하고 있는 output이라는 객체를 그냥 넘겨받음으로써 이런 코드가 나타나는거 같아요.
이러면 그냥 output을 주입받아 사용해도 될것 같단 생각이 드네요.
output을 넘겨주지말고, console 내에서 output을 이용하는 다른 메서드를 사용하는 것은 어떨까요?
public static Selection find(int code) { | ||
return Arrays.stream(Selection.values()) | ||
.filter(selection -> selection.code == code) | ||
.findAny() | ||
.orElseThrow(() -> 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.
LGTM
this.expression = expression; | ||
} | ||
|
||
public static Optional<Operator> find(String symbol) { |
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.
Optional good
|
||
public abstract class Calculator { | ||
|
||
protected final ExpressionValidator validator; |
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.
ExpressionValidator 클래스 내부를 보면
isNumber, isOperator라는 메소드만 존재하는거 같아요.
validator에 isOperator 메소드가 있는것보다 Operator의 역할 이라는 생각이 들어요.
그렇다면, Validator는 util이 될 수도 있고 Calculator에서 ExpressionValidator에 의존하지 않아도 될것 같단 생각이 듭니다.
number.append(element); | ||
|
||
} else { | ||
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.
Converter에서 Expression에 대한 검증을 하네요!?
그렇다면.. ExpressionValidator가 적절한 클래스인가를 다시 고민해볼 수 있을것 같습니다.
@DisplayName("계산 성공 테스트") | ||
void calculateSuccess() { | ||
ExpressionValidator validator = new ExpressionValidator(); | ||
Calculator calculator = new PostfixCalculator(validator); | ||
Converter converter = new PostfixConverter(validator); | ||
|
||
ExpressionParam param = converter.convert("3+4*5"); | ||
ExpressionResult expressionResult = calculator.calculate(param); | ||
double result = expressionResult.getAnswer(); | ||
|
||
assertThat(result).isEqualTo(23); | ||
} |
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.
일반적으로, 테스트코드에서 TDD나 BDD에 의한 given - when - then 패턴을 사용하여 나누면 테스트 코드의 가독성이 올라가는 경향이 있습니다.
https://brunch.co.kr/@springboot/292
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
class OperatorTest { | ||
|
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.
LGTM
@Test | ||
@DisplayName("후위표기식 변환 테스트") | ||
void toPostfixTest() { | ||
Converter converter = new PostfixConverter(new ExpressionValidator()); | ||
|
||
String expression = "3+4*2"; | ||
String expression2 = "15-5*2"; | ||
|
||
ExpressionParam param = converter.convert(expression); | ||
assertThat(param.getPostfix().get(1)).isEqualTo("4"); | ||
assertThat(param.getPostfix().get(3)).isEqualTo("*"); | ||
|
||
ExpressionParam param2 = converter.convert(expression2); | ||
assertThat(param2.getPostfix().get(0)).isEqualTo("15"); | ||
} |
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.
@ParameterizedTest로 반복되는 테스트에 코드의 중복성을 줄여 테스트 할수 있어요!
@Test
@DisplayName("후위표기식 변환 테스트")
@ParameterizedTest
@CsvSource({
"3+4*2, 4, *",
"15-5*2, 15, -"
})
void toPostfixTest(String expression, String expectedValue1, String expectedValue2) {
// given
Converter converter = new PostfixConverter(new ExpressionValidator());
// when
ExpressionParam param = converter.convert(expression);
// then
assertThat(param.getPostfix().get(1)).isEqualTo(expectedValue1);
assertThat(param.getPostfix().get(3)).isEqualTo(expectedValue2);
}
같은 방식으로요 !
|
||
repository.save(result); | ||
|
||
assertEquals(repository.getAll().get(0L).getAnswer(), 2.0); |
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.
어떤곳은
import static org.junit.jupiter.api.Assertions.*;
을 사용하시고,
어떤곳은
import static org.assertj.core.api.Assertions.assertThat;
을 사용하시는데 통일성을 맞춰주는 것이 좋을것 같아요!
두 라이브러리 중 어떤것이 더 편리할지 학습해보시는것이 좋습니다!
} | ||
|
||
@Override | ||
public Map<Long, ExpressionResult> getAll() { |
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.
Map을 통째로 return하는것이 좋을까 고민 해보시겠어요!
List vs Map ?
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.
전체 객체를 리턴하기에 Map은 좀 아쉽네요. 루프를 돌기 원활한 컬렉션은 아니라서
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.
안녕하세요 예슬님. 피드백 반영하시고 개선하여 피알 올리느냐 고생하셨습니다.
구조적으로 깔끔해 보이는데 위치가 애매하거나, 없앨 수 있는 부분이 조금 보여요!
자료구조 선택에 있어서도 고민해보시는게 좋을거 같아요! Map이라던가 List라던가요!
둘의 차이점과 장단점을 학습하신 후 상황에 맞게 적합하게 사용하면 좋을것 같습니다.
객체는 책임을 잘 나누신거 같은데, 메소드도 추상화를 같이 해주면 좋을것 같습니다!
또한 의존하고 있는 객체를 그냥 반환해서 사용할건지, 그 의존 객체를 이용해서 기능을 만들어 제공할것인지에 대해
어떤 구조가 더 나을지도 고민해보시면 좋을거삭ㅌ아요!
—
사용자로부터 1부터 3까지의 숫자를 입력받는 것을 private static final 상수로 클래스 안에 정의하였었는데,
해당 클래스 내에서만 사용하는 상수라서 그렇게 구현했던 것 같습니다.
이를 피드백을 받고 enum으로 변경하였는데, enum으로 정의할 경우 다른 클래스에서도 접근해서 사용할 수 있을 것 같은데 이러한 점이 단점이 되지는 않는지 궁금합니다..!
그리고 클래스 안에 enum을 정의하는 것은 어떻게 생각하시는지도 궁금합니다!
enum이 무엇인지, static final로 정의한 상수랑 어떤 차이가 있는지 더 학습해 보시면 좋을것 같습니다.
Enum이 다른 클래스에서 접근할 수 있는 것이 항상 단점이라고 말할 수는 없을거 같아요.
고정된 상수 집합을 명확하게 표현한것이 enum이니까요.
enum은 또 타입 safe 합니다. 특정 enum에 속한 상수란것을있으니까요!
어떻게 사용하느냐에 따라 단점이 될 수도 있고 아닐수도 있다고 생각을 해요.
예를들어, 구체적인 책임을 주지않고 중구난방으로 이곳저곳에서 사용하게 된다면, 해당 enum을 바꾸게 되었을때 그 enum을 사용하는 모든곳에 변경이 전파될 수 있겠죠?
또한 enum이 특정 클래스와 밀접하게 관련된 상수 집합을 표현하기 위한 좋은 방법이라고 생각해요!
관련된 상수를 더 의미있게 표현할 수 있거든요!
고드 가독성과 관리에 더 좋다고 생각합니다.
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 ExpressionResult calculate(ExpressionParam param) { |
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.
요 메소드가 조금 행위의 구분이 덜 진행됐다고 느끼는게
메소드 정의는 잘 해주셨습니다만 계산과 계산과정에서의 알고리즘이 너무 많은 것 같아요.
알고리즘이 많다는건 역할분리를 못했다는 결과로써 기인할 수도 있다고 느껴져요.
지금은 계산하는 로직이 후위표기 후 연산하는 postfix와 혼재되어 있는데
명확히 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.
제가 오해를 했군요... ExpressionParam이 후위표기식 파라미터였군요...!
어쨌든 위 내용은 의도파악을 위해 남겨놓겠습니다 ㅎㅎ
|
||
public interface Repository { | ||
|
||
void save(ExpressionResult object); |
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.
object 말고 다른 이름으로 해야겠죠?ㅎㅎ
} | ||
|
||
@Override | ||
public Map<Long, ExpressionResult> getAll() { |
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.
전체 객체를 리턴하기에 Map은 좀 아쉽네요. 루프를 돌기 원활한 컬렉션은 아니라서
@Override | ||
public void printError() { | ||
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.
메시지에 비해 메소드명이 모호한편입니다.
printWrongMenuNumberError() 이런식으로 좀더 상세하게 표현해도 될것같아요.
📌 과제 설명
사칙연산 계산기를 구현한 어플리케이션입니다. 사용자로부터 1부터 3까지의 숫자를 입력 받습니다.
1을 선택한다면 -> 계산 history가 출력됩니다.
2를 선택한다면 -> 사용자로부터 계산식을 입력받고, 그에 따른 결과를 콘솔에 출력합니다.
3을 선택한다면 -> 프로그램이 종료됩니다.
👩💻 요구 사항과 구현 내용
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
저희팀의 다른분들이 작성한 질문은 제외하고 작성하겠습니다!
사용자로부터 1부터 3까지의 숫자를 입력받는 것을 private static final 상수로 클래스 안에 정의하였었는데,
해당 클래스 내에서만 사용하는 상수라서 그렇게 구현했던 것 같습니다.
이를 피드백을 받고 enum으로 변경하였는데, enum으로 정의할 경우 다른 클래스에서도 접근해서 사용할 수 있을 것 같은데 이러한 점이 단점이 되지는 않는지 궁금합니다..! 그리고 클래스 안에 enum을 정의하는 것은 어떻게 생각하시는지도 궁금합니다!