-
Notifications
You must be signed in to change notification settings - Fork 155
[4기 - 신재윤] 1~2주차 과제: 계산기 구현 미션 제출합니다. #192
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주차 과제: 계산기 구현 미션 제출합니다. #192
Conversation
| public List<CalculationHistory> findAll() { | ||
| return new ArrayList<>(store.values()); |
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 vs List
왜 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.
사실, 관례적으로 List<> findAll()을 사용했던 것 같아서, 늦게나마 이유에 대하여 찾아보았고
아래와 같은 이유로 Map 보다는 List가 적절하다고 생각합니다.
- 계산 이력이라는 특성 상 순서대로 반환하고 싶었습니다. Map은 순서가 보장되지 않기 때문입니다.
- 특정 값에 접근하는 상황이라면 Map이 빨랐겠지만, 현재 상황에서는 List가 빨라서 입니다.
- List는 인덱스가 있지만, Map은 인덱스가 없어서 iterator로 뽑아야합니다.
| @Override | ||
| public String toString() { | ||
| return id + " : " + expression + " = " + 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.
toString() 오버라이드 괜찮네요.
혹시 기왕 이런 객체를 만들었다면, toString()을 오버라이드 하지 않고 다른 추가 기능을 만들어줘도 되지 않을까요?
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()을 오버라이드 하였는데,
계산 이력을 반환한다는 기능을 추가해보겠습니다.
| @@ -0,0 +1,33 @@ | |||
| package com.programmers.calculator.domain.vo; | |||
|
|
|||
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.
Expression이 왜 CharSequence를 구현하고 있는지 재윤님의 의도가 궁금합니다.
CharSequence가 무엇일까요?
length, charAt, subSequence가 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.
정규식에 넣어야하는 부분에서 요구타입이 CharSequence 였기에 구현했는데,
구현하면서도 Expression이 구현하는 것이 맞는가하는 의문이 있었습니다.
흑구님의 피드백에서도 나왔는데 String에서 CharSequence를 구현하고 있다는
기본적인 내용을 다시 한번 상기했습니다.
| import java.util.List; | ||
|
|
||
| public class FourArithmeticCalculator implements Calculator { | ||
|
|
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 CalculationResult calculate(Expression expression) { | ||
| List<String> tokens = RegexEnum.parseToTokens(expression); | ||
| List<String> postfix = converter.convert(tokens); | ||
| return evaluator.evaluate(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.
evaluator는 무슨 역할을 하는 친구인가요?
해석해보면 평가자..라는 의미같은데 그냥 위임한 정도인거같아요
이유가 있으실까요?
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의 calculate 메서드는 logic으로 판단하였고
계산에 대한 실질적인 behavior는 evaluator에서 진행하여 위임했습니다.
파싱한 expression을 converter로 후위표기식으로 변경한 이후
evaluator에서 적절한 후위표기식인지, 수식에 오류가 있지는 않는지를 판단하고
계산하는 역할이어서 나누게 되었습니다.
| @DisplayName("repository 정상적으로 저장되는지 확인") | ||
| @Test | ||
| void repository_right_work() { | ||
|
|
||
| //given | ||
| Expression expression1 = new Expression("1 + 2 * 3"); | ||
| Expression expression2 = new Expression("3 + 2 * 3 / 3"); | ||
| CalculationResult calculationResult1 = new CalculationResult(new BigDecimal(7)); | ||
| CalculationResult calculationResult2 = new CalculationResult(new BigDecimal(5)); | ||
|
|
||
| CalculationHistory result1 = new CalculationHistory(expression1, calculationResult1); | ||
| CalculationHistory result2 = new CalculationHistory(expression2, calculationResult2); | ||
|
|
||
| //when | ||
| repository.save(result1); | ||
| repository.save(result2); | ||
|
|
||
| //then | ||
| List<CalculationHistory> all = repository.findAll(); | ||
| /** | ||
| * 1. size check | ||
| * 2. reposity.contains(r1, r2) | ||
| * 3. reposity.get(0) = 7 | ||
| * 4. reposity.get(1) = 5 | ||
| */ | ||
| assertThat(all.size()).isEqualTo(2); | ||
| assertThat(all).contains(result1, result2); | ||
| assertThat(all.get(0)).isEqualTo(result1); | ||
| assertThat(all.get(1)).isEqualTo(result2); | ||
| } |
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.
@ParameterizeTest에 대해 알아보세요!
반복되는 코드의 중복을 줄일 수 있습니다.
| CalculationResult operand2 = new CalculationResult(new BigDecimal(0)); | ||
|
|
||
| // when | ||
| Assertions.assertThatThrownBy(() -> division.getFunction() |
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 import 하셨으면 통일성 맞춰주는게 좋아요!
| assertThat(operator.getSymbol()).isEqualTo(operatorChar); | ||
| } | ||
|
|
||
| @DisplayName("연산자가 아닌 값들을 파라미터로 넘겨주면, 예외가 발생") |
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 java.util.List; | ||
|
|
||
| public class CalculatorController { |
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.
제가 던지기만 하고 예외처리를 제대로 하지 않았었네요.
처리하도록 하겠습니다 !!
|
|
||
| import java.util.List; | ||
|
|
||
| public interface Converter { |
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.
저는 뭔가 Convert가 inptu String을 Expression 객체로 변환시켜주는 역할인줄 알았어요!
내부 구현을 알 필요 없이 input을 넘기면 식 객체로 반환되게끔요!
postfix prefix infix 상관없이 인터페이스만 사용해서요!
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라는 이름은 오해의 소지가 있는 것 같아서 명확하게
NotationConverter와 같은 방식으로 변경해보겠습니다.
| SUBTRACTION('-', 10, CalculationResult::subtract), | ||
| MULTIPLICATION('*', 100, CalculationResult::multiply), | ||
| DIVISION('/', 100, (o1, o2) -> { | ||
| if (o2.getValue().equals(BigDecimal.ZERO)) { |
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.
o2가 만약 null이 들어오면 npe가 발생할 수 있을것같아요
Objects.equals에 대해 알아보세요 !
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.
Objects.equals() 메서드를 통해 null이 들어오더라도 NPE를 방지하고
안전하게 동등성 비교를 수행하는 것을 알게 되었습니다. 알려주셔서 감사합니다 !!
- 두 개의 객체가 모두 null인 경우, true를 반환하여 두 개의 null 객체는 동등하다고 판단
- 두 개의 객체 중 하나만 null인 경우, false를 반환하여 null과 다른 객체는 동등하지 않다고 판단
- 두 개의 객체가 모두 null이 아닌 경우, equals() 메서드를 사용하여 객체의 동등성을 검사
| import java.math.BigDecimal; | ||
| import java.math.RoundingMode; | ||
|
|
||
| public class CalculationResult { |
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 java.math.RoundingMode; | ||
|
|
||
| public class CalculationResult { | ||
| private BigDecimal value; |
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.
값 객체이면서 바뀔 일이 없다면 final 고려 해보시는게 어떨까요?
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.
이 부분도 동일하게 final에 대하여 개념이 부족했던 것 같습니다.
(또, Expression에는 private final 했으면서 여기는 안한것이,,, 죄송함니다)
| } | ||
|
|
||
| public String inputOption() { | ||
| output.write("\n\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() 라는 메소드가 존재합니다~!
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()를 이용하여 OS 구분 없이 일정한 개행을
제공할 수 있다는 점을 알게 되었습니다.
추가로, String newLine = System.getProperty("line.separator");
같은 형태로 조금 더 깔끔하게 newLine을 추가할 수 있을 것 같아서 적용해보았습니다.
devYSK
left a comment
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, Evaulator, Converter의 관계가 조금 애매한거 같긴 해요!
과하게 나누었거나, 서로 뭔가 꼬여있는 듯한 느낌이 들어요. 이부분은 다시 한번 고민해보시면 좋을거같아요.
또한 예외 처리하는 부분이 없는데, 어디서 처리해야 하고 어떻게 하면 좀 더 깔끔하게 할 수 있을지 생각해보세요!
값 객체를 만드셔서 사용했는데, 어디서부터 값객체가 전달되어야 할까를 고민해 보시면 좋을거같습니다.
테스트 코드 꼼꼼하게 짜신거 같아서 보기 좋습니다.
리팩토링할때도 테스트 코드가 도움이 많이 될거에요!
—
logic behavior data를 최대한 고려해봤는데, 사실 아직도 감이 잘 잡히지 않습니다.
최대한 추상화해보려고 했는데, 역할이 너무 작나? 과한 추상화인가? 라는 고민이 계속 생겼습니다.
아직 감이 잘 안잡히실거라 생각합니다.
SOLID를 생각하면서 설계해보시면 도움이 될 것 같습니다.
추상화의 목적은 무엇인가? 어떤 문제를 해결하기 위한 것이지? 어떤 장단점이 있을까
—
테스트 코드가 미흡하여, 영수님 강의를 듣고 감이 조금 잡혀서 작성해봤는데, 방식이 맞는지 모르겠습니다.
저는 꼼꼼하게 잘 작성하신거 같습니다.
조금더 스킬풀하게 사용하려면 많이 사용해보시면 점점 늘 수 있다고 생각합니다.
이것저것 테스트 해보고, 다른 사람은 어떻게 테스트 했을까 보시면서 흡수해보세요!
—
홍섭님 피드백을 봤을 때, 공학용 계산기와 사칙연산만 가능한 계산기를 고려해보려 했습니다.
- 현재, Operator enum에 사칙연산이 모두 몰아두고 enum의 values()를 반복돌리는 메서드를 정의해놔서 enum에 정의되어 있다면 반환해주고 없으면 예외를 던지는 상태입니다.
- enum에 기능만 추가하면 되기 때문에 공학용 계산기가 추가되는건 문제가 없습니다.
- enum에 다른 기능을 추가하면 사칙연산만 가능한 계산기가 다른 기능도 사용할 수 있는 상황이 되어버립니다.
- 이에 대하여 생각한 해결 방법은 아래와 같습니다.
* 공학용 계산기 enum을 하나 더 만들 때, 인터페이스를 이용해 확장 가능한 Enum Type이 되도록 리팩토링 하는 것 입니다. 아이템38 - 인터페이스 활용 Enum Type 확장를 참고하였습니다.
* enum을 없애고 FourArithmeticStrategy 라는 인터페이스를 구현하는 각각 사칙연산이 있고, 공학용 전략 인터페이스를 하나 만들어서 거기에 심화적인 기능을 추가하고, 기본 계산기에는 사칙연산 전략만 적용, 공학용 계산기에는 사칙연산전략 + 심화전략 적용 이렇게 가보려고 합니다.
* 어떠한 방법이 더 괜찮을지 궁금합니다.
두 가지 방법 모두 좋은거 같습니다. 상황에 따라 적절하게 사용할 수 있을것 같아요.
어떤 방법이 재윤님이 만드는 시스템의 목적과 목표에 더 부합하느냐 차이일것 같습니다.
인터페이스를 활용하여 enum을 확장하면 직관적이며 타입 안정성을 유지할 수 있지만, 모든 enum 상수가 동일한 동작을 가져야 하는경우에는 적합하지 않겠죠
예를들면, 제곱, 제곱근등의 연산이 추가될경우 기본 사칙연산에는 사용되지 않을 수 있으니까요
전략패턴을 사용하면 사칙연산이랑 공학용 계산기 전략을 분리하여 각각 다른 전략을 사용할 수 있지만,
서로 독립적인 클래스로 구현되어야 하므로 개발난이도와 코드 복잡성이 증가할 수 있다는 단점이 있다고 생각합니다.
|
영수님이 잘 답변해주셔서 제가 더 첨언할게 없네요;;;
|
|
흑구님 영수님 안녕하세요. pre 기간 동안 너무 감사했습니다. 강의에서 나온 코드를 따라치는 것이 아닌 직접적으로 짜본 경험은 이번이 처음인데 또, 한번씩 던져주시는 질문에 대한 고민을 해볼 때 지식들이 통합되는 과정이 즐거웠습니다. 앞으로도 logic -> behavior -> data와 추상화는 절대로 안까먹을 것 같습니다. |
📌 과제 설명
👩💻 요구 사항과 구현 내용
return new Pattern()부분이었습니다.😭 1차 자기주도적 리뷰 피드백
영수님 피드백
Expression클래스의isNumber()메서드)Calculator클래스에서 필드로expression을 가지는 것에 의문이 듭니다.Expression클래스가 너무 비대하고 특히converToPostfix()메서드를 최대한 쪼개봅시다.if-else보단switch를 이용하여 리팩토링 해봅시다.흑구님 피드백
InputConsole과OutputConsole이 마치 2개의 콘솔처럼 보입니다.😤 피드백 기반 리팩토링
✅ PR 포인트 & 궁금한 점
PR 포인트
궁금한 점