-
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주차 과제 : 계산기 미션 제출합니다. #158
base: heenahan
Are you sure you want to change the base?
Conversation
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.
과제하느라 고생하셨습니다!
희나님이 궁금한 점으로 남겨두었던 부분은 pr 코멘트로 달아놓았어요.
확인하시다보면 어떤 부분인지 아실 것 같습니다! 🙇
for (String expression : list.keySet()) { | ||
Integer result = list.get(expression); | ||
System.out.println(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.
for (String expression : list.keySet()) { | |
Integer result = list.get(expression); | |
System.out.println(expression + " = " + result); | |
} | |
list.keySet().forEach(expression -> { | |
Integer result = list.get(expression); | |
System.out.println(expression + " = " + result); | |
}); | |
} |
stream을 사용해보세요~
public void run() { | ||
try (BufferedReader br = new BufferedReader(new InputStreamReader(System.in))) { | ||
while (true) { | ||
outputView.init(); |
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.
초기화를 외부에서 해주는 것이 아닌 class가 생성되는 시점에 초기화할 수 있도록 하는건 어떨까요?
@@ -0,0 +1,63 @@ | |||
package com.programmers.calculator; | |||
|
|||
import java.util.*; |
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.
아 ide에서 자동으로 와일드카드로 변경되지 못하도록 바꾸겠습니다!!
|
||
import com.programmers.controller.CalculatorController; | ||
|
||
public class App { |
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.
아주 사소한 의견입니다만,
CalculatorApplication 과 같은 구체 네이밍은 어떨까요?
if (num.equals(1)) view(); | ||
else if (num.equals(2)) calculate(br); | ||
else { | ||
outputView.inputInOneAndTwo(); | ||
continue; | ||
} |
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.
isValidInput 에서 isEmpty인지, 숫자만 입력되는지 검증하고 있는데요,
isValidInput에서 1,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.
넵 메뉴를 관리하는 enum을 생성하였습니다!! 메뉴 enum 안에서 입력 검증하도록 바꾸었습니다.
|
||
public List<String> changeInfixToPostfix(List<String> infix) { | ||
List<String> postfix = new ArrayList<>(); | ||
Stack<String> stack = new Stack<>(); |
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 Storage { | ||
|
||
private final static Map<String, Integer> map = new ConcurrentHashMap<>(); |
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.
동시성을 고려하여 ConcurrentHashMap을 사용하신부부은 잘하셨습니다! 다만,
유저가 동일 계산식을 n 번 수행하고, 조회할 경우 어떤 결과가 보여지는게 좋다고 생각하시나요?
동일 계산식이니 중복 제거하여 1번 보여줌
동일 계산식이나 히스토리를 모두 보여주는게 좋으니 n번 보여줌.
위 코드라면 중복은 제거하고 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.
팀 미팅에서 중복 저장이 안된다는 것은 자료구조가 잘못되었다고 말씀하셔서 List로 바꾸었습니다!!
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
class AppTest { | ||
@Test void appHasAGreeting() { |
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.
취향차이라고 생각합니다..만!
@test
void appHasAGreeting()
처럼 뉴라인을 해주시는건 어떻게 생각하시나용?
@DisplayName("연산자 우선 순위 계산") | ||
void isPriority() { | ||
// given | ||
String[] operator = { "+", "-", "*", "/" }; |
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.
희나님 코드라면
String[] operator = { "+", "-", "*", "/" }; 부분을
String[] operator = { "!", "%", "*", "/" }; 아래처럼 수정해도 해당 코드는 통과할거에요.
테스트 코드가 잘못된것이 아니라, 비즈니스 로직이 잘못되었기 때문이에요.
만약 테스트 코드를 작성할 때, 정상적인 케이스만 짜는게 아니라 비정상적인 케이스도 같이 짰다면 (String[] operator = { "+", "-", "*", "/" };와 같은 경우), 잠재적인 버그를 찾아낼 수 있지 않았을까요?
이런 부분이 테스트 코드의 순기능이라고 생각해요! 다음번엔 비정상적인 케이스에 대해서도 고려하며 테스트 짜보는걸 추천해요~
추가로 @ParameterizedTest를 사용하면 여러개의 테스트를 한번에 돌릴 수 있어요~
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를 사용하여 테스트를 작성해보았습니다ㅎㅎ
} return true; | ||
} | ||
|
||
private void calculate(BufferedReader br) throws IOException { |
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.
희나님! 과제하시느라 수고많으셨습니다! 💯
Enum을 활용해서 상태와 행위를 한 곳으로 모아주신 점이나 커밋 단위를 작게 하려고 하는 노력이 보여서 좋았습니다.
대신 현재 빌드툴을 gradle로 선택해서 사용하셨는데요. build.gradle
파일이 없다보니 의존성을 땡겨오지 못해서 실행을 못해주네요. 같이 올려주시길 바래요~
몇가지 코멘트 남겼는데 확인해주세요~~ 👍
객체지향적으로 구현한다는 것이 너무 어렵습니다ㅠㅠ 메서드를 더 잘게 쪼개야 하는지.. 하나의 클래스에 너무 많은 책임이 있는지 피드백 부탁드립니다..
일단 메서드를 잘게 쪼개다 보면 메서드가 점차 많아지게 될 건데요. 여기서 고민해봐야 할 점이 해당 메서드가 꼭 이 객체에게 있어야할까를 잘 생각해보시면 좋을 것 같아요.
예를 들어 Calculator
라는 객체에서 �파싱부터 유효성검사, 계산까지 전부 진행했을 때 메서드가 잘게 쪼개져 있다면 수많은 메서드가 생겨나게 될 건데요. 여기서 파싱은 다른 객체에게 책임을 줘보다던지 계산만 진행하는 객체를 만들어본다던지 등등 분리해줄 수 있을 것 같아요.
처음부터 완벽하게 설계하고 들어가야지! 라고 생각하지 않고 일단 구현해놓고 여러 가지로 분리해보는 것도 좋은 방법이라고 생각합니다.
테스트 코드를 나름대로 작성해봤는데.. 피드백 부탁드립니다!! 테스트 코드의 이름을 더 구체적으로 작성해야 하는지 읽기 쉬운 테스트코드인지 더 다양한 경우의 테스트를 해봐야하는지 궁금합니다!!
테스트 코드 이름이 구체적이라고 해서 읽기 쉬운 테스트코드는 아니지만 중요하다고 생각합니다. 결국 다른 사람이 테스트 코드를 보았을 때 하나의 명세가 될 수 있기 때문인데요. 좀 더 구체적으로 DisplayName()
을 적어주면 좋습니다. 또는, 메서드이름을 한글로 적어주어도 좋구요.
또한, 좀 더 다양한 케이스를 테스트 해보았으면 합니다. 현재 해피케이스와 예외케이스를 모두 진행해주신 점 너무 좋은데요. 한 케이스에 대해서만 진행하는 것이 아닌 여러 케이스에 대해 진행해보았으면 합니다.
예외를 제대로 처리한 것인지 궁금합니다.
희나님이 어떤 기준을 잡고 예외를 처리하고 있는지를 먼저 여쭤보고 싶어요.
현재 예외가 발생하면 잡아서 outputView의 메서드를 통해 화면에 출력해주게 되는데요.
이럴 경우 예외 상황이 들어나면 계속 catch문이 늘어날 수 있을 것 같아요.
어떻게 하면 더 간단하게 처리해줄 수 있을까요?
private Storage storage = new Storage(); | ||
|
||
public void run() { | ||
try (BufferedReader br = new BufferedReader(new InputStreamReader(System.in))) { |
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
객체에서 입력 관련된 객체를 초기화해주지 않은 이유가 있으실까요?
희나님은 InputView
가 어떤 책임을 가지고 있다고 생각하고 구현하셨을까요?
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는 사용자로부터 입력을 받는 책임을 가지고 있습니다..!! 다시 생각을 해보니 입력 객체는 InputView에서 관리하는게 맞다고 생각해서 InputView에 BufferedReader를 초기화해주는 것으로 바꾸었습니다..!!
public Map<String, Integer> findAll() { | ||
return 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 객체 자체를 반환해주고 있는데요.
필드에 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.
스트림을 사용하여 새로운 자료구조를 반환하도록 만들었습니다.
Integer op2 = stack.pop(); | ||
|
||
Operator calc = Operator.of(s); | ||
Integer result = calc.getFunc().apply(op2, op1); |
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.
get을 사용해서 다시 꺼낼 필요없이 calc.doCalculate(op2, op1);
처럼
해당 객체에게 메시지를 보내는 방식으로 바꾸면 좋을 것 같아요.
굳이 내부 구현을 외부에 노출해줄 필요는 없어보입니다.
public Integer getPriority(String operator) { | ||
// * 와 / 는 우선순위 높음 | ||
if (operator.equals("*") || operator.equals("/")) return 1; | ||
// 그 외 낮음 | ||
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.
Operator라는 객체를 통해 상태와 행위를 모아주신 점 아주 좋습니다. 💯
다만 제 생각에 우선순위도 하나의 상태라고 생각할 수 있을 것 같은데요.
Operator에게 책임을 주는 것 어떠실까요~?
|
||
for (String s : postfix) { | ||
// 숫자일 경우 스택에 넣음 | ||
if (s.matches("\\d+")) stack.push(Integer.parseInt(s)); |
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.
숫자인지 판단하는 로직을 메서드로 추출하면 좀 더 읽기 편해질 것 같아요. 또한, 판단하는 정규식을 상수로 만들어줘도 좋구요.
추가로 matches()의 내부 구현을 보면 Pattern
을 계속 생성하기 때문에 비효율적이게 됩니다.
미리 Pattern
객체를 만들어놓고 재사용하면 어떨까요??
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class 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.
희나님이 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.
Expression 객체는 입력으로 수식을 검증하고 문자열 수식을 리스트로 형 변환하는 책임을 주었습니다!!
private void view() { | ||
Map<String, Integer> list = storage.findAll(); | ||
outputView.viewList(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
라고 나타내주고 있는데요. 사실 Map이기 때문에 List라고 나타내주는 건 조금 애매한 것 같아요.
대신 viewResults()
와 같은 메서드명을 사용하거나 Map -> 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.
넵!! 의미 있는 네이밍을 사용하도록 하겠습니다..!!
DIVIDE("/", (a, b) -> { | ||
try { | ||
return a / b; | ||
} catch (ArithmeticException e) { | ||
throw e; | ||
} | ||
}), | ||
MUTIPLY("*", (a, b) -> { return a * b; }); |
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.
현재 예외가 발생하면 잡아서 다시 throw를 해주고 있는데요.
굳이 똑같은 예외를 발생시킬거면 잡을 필요가 있을까요?
어떤 목적으로 구현하셨는지 궁금합니다.
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.
멘토님 말씀을 보고 의미가 없는 코드라고 생각해 ArithmeticException이 발생하면 custom exception인 DivisionByZeroException에 메시지를 담아 던지도록 바꾸었습니다..!!
plugins { | ||
// Apply the foojay-resolver plugin to allow automatic download of JDKs | ||
id 'org.gradle.toolchains.foojay-resolver-convention' version '0.4.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.
요고는 어떤 플러그인일까요?? 👀
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.
엇.. 이건 제가 추가한 것이 아닌 그냥 gradle 프로젝트 생성할 때부터 있었던 플러그인입니다..!! 찾아보니 JVM을 다운로드하기 위한 저장소를 제공하는 플러그인이라고 합니다.. 정확히 무엇인지는 모르겠습니다ㅠㅠ
- 저장 시간을 key로 사용 - 상태를 그대로 반환이 아닌 새로운 자료구조를 생성하여 반환
- 선택지에 대한 유효성 검사
✅ 피드백 반영사항
|
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 Formula { | ||
|
||
private static final Pattern FORMULA_REGEX = Pattern.compile("([0-9] [+|\\-|*|/] )+[0-9]"); | ||
private static final String EQUAL = " = "; | ||
|
||
private final Calculator calculator; | ||
|
||
private String formula; | ||
private Integer result; | ||
|
||
public Formula(String formula) { | ||
isValid(formula); | ||
this.formula = formula; | ||
this.calculator = new 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.
Formula
는 어떻게 보면 데이터만 가지고 있는 객체인듯 한데요.
Calculator
를 의존하신 이유가 있을까요??
} | ||
|
||
public String expression(BufferedReader br) throws IOException { | ||
return br.readLine(); | ||
public String formula() { |
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.
자바 컨벤션에서 메서드는 동사를 사용합니다!
동사를 사용하도록 습관을 들이면 좋을 것 같아요~!
참고 자료 : https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/
public List<String> view() { | ||
return storage.findAll(); | ||
} |
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.
view보다는 좀 더 의미있는 네이밍이 좋을 것 같아요~
예를 들어 getAllCalculatorHistories
와 같이 말이죠~
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!
📌 과제 설명
👩💻 요구 사항과 구현 내용
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점