[4기 - 황준호] 1~2주차 과제: 계산기 구현 미션 제출합니다. #191
Conversation
|
|
||
| public CalculatorController(Console console) { | ||
| this.console = console; | ||
| this.service = new CalculatorService(); |
There was a problem hiding this comment.
이 Service는 왜 주입을 안받고 직접 생성하시는지 이유가 궁금합니다.
| exitProgram(); | ||
| return; | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
혹시 왜 매우 넓은 범위의 Exception을 통째로 catch하신지 이유가 궁금합니다.
| } | ||
| } | ||
|
|
||
| private Selection getCode() { |
There was a problem hiding this comment.
getCode() 보다는 조금더 명확한 의미가 좋을거같아요.
일반적으로 저는 get을 프로퍼티나 필드의 값을 가져올때 사용합니다.
사용자로부터 입력을 받아온다면?
| import com.programmers.blackdog.domain.expression.Expression; | ||
|
|
||
| public interface AbstractCalculator { | ||
| int calculate(Expression expression); |
There was a problem hiding this comment.
계산 결과가 단순 int value로 리턴되기보다 객체로 바라볼 순 없을까요?
| public final class Regex { | ||
|
|
||
| private Regex(){ | ||
| throw new AssertionError("인스턴스화 할 수 없습니다!"); |
There was a problem hiding this comment.
충분히 잘못된 상황임을 인지하기 어려운 예외 클래스 인거같아요.
다른 예외는 무엇이 있을까요?
| public static final String DELIMITER; | ||
| public static final String REGEX; | ||
|
|
||
| static { |
| public final class StringUtil { | ||
|
|
||
| private StringUtil() { | ||
| throw new AssertionError("Util 클래스 생성 불가합니다."); |
| ArithmeticOperators(String operator, int priority) { | ||
| this.operator = operator; | ||
| this.priority = priority; | ||
|
|
| return Arrays.stream(values()) | ||
| .filter(operator -> operator.getOperator().equals(token)) | ||
| .findAny() | ||
| .orElseThrow(); |
| package com.programmers.blackdog.domain; | ||
|
|
||
| @FunctionalInterface | ||
| public interface Operable { |
| public static final String PREFIX = "\\"; | ||
| public static final String SUFFIX = ""; | ||
|
|
||
| public String generateWithOperator(ArithmeticOperators... arithmeticOperators) { |
There was a problem hiding this comment.
ArithmeticOperators...
와
ArithmeticOperators[]
는 동작할 때 어떤 차이가 있을까요?
|
|
||
| import static com.programmers.blackdog.domain.ArithmeticOperators.values; | ||
|
|
||
| public class RegexGenerator { |
| public interface CalculatorRepository { | ||
| void save(String calculatedResult); | ||
| List<String> findAll(); | ||
| } |
There was a problem hiding this comment.
소소한팁인데.. 너무 딱딱 붙어있으면 보기 힘들때가 있어용! 가독성을 높여보면 어떨까요?
There was a problem hiding this comment.
이건 아마 IDE에서 띄워주셔서 이렇게 커밋된걸수도 있을듯 합니다 ㅎㅎ (= 본인..)
| this.calculatorRepository.save(generateTotalResult(expression, result)); | ||
| } | ||
|
|
||
| private String generateTotalResult(String expression, int result) { |
There was a problem hiding this comment.
expression과 result를 합쳐 String 으로 만드는것보다는,
객체로 만들고 저장하면 어떨까요?
String 타입보다는 값들을 다루기 쉽지 않을까요?
|
|
||
| // 계산 및 저장 로직 담당 | ||
| public class CalculatorService implements Service { | ||
| private static final String EQUAL = " = "; |
There was a problem hiding this comment.
EQUAL 이라는 친구가 이곳에 private static으로 존재해야 싶기도 합니다.
어디 있는것이 적당할까요?
혹시 generateTotalResult()와 연관이 있진 않을까요?
| public class CalculatorController { | ||
|
|
||
| private final Service service; | ||
| private final Console console; |
There was a problem hiding this comment.
MVC 패턴과 Layered 아키텍처로 설계하신거 같아요!
Controller는 어떤 역할을 할까요? 준호님의 생각이 궁금합니다.
| private final List<String> calculatedData; | ||
|
|
||
| public MemoryCalculatorRepository() { | ||
| this.calculatedData = new ArrayList<>(); | ||
| } |
There was a problem hiding this comment.
field에
private final List<String> calculatedData; = new ArrayList<>();와 생성자로 new 하는것은 어떤 차이가 있을까요?
| } | ||
|
|
||
| @Override | ||
| public void save(String calculatedResult) { |
There was a problem hiding this comment.
calculatedResult를 String 타입으로 저장한다면,
나중에 특정 계산 결과인 result로만 값을 꺼내고 싶은 경우에 어떻게 해야 할까요..?
|
|
||
| @Override | ||
| public List<String> findAll() { | ||
| return Collections.unmodifiableList(calculatedData); |
| public String read() { | ||
| try { | ||
| return bufferedReader.readLine(); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
예외를 다시 던지실때는 발생한 예외를 cause 로 같이 넣어주는것이 좋습니다!
기존 예외를 cause로 포함시킴으로써 디버깅 및 처리에도 도움이 되고, 스택 트레이스를 추적할 수 있거든요.
| try { | ||
| return Integer.parseInt(inputView.read()); | ||
| } catch (NumberFormatException e) { | ||
| throw new NumberFormatException("잘못된 값을 입력하셨습니다."); |
| public void print(int result) { | ||
| System.out.println(result); | ||
| } | ||
|
|
||
| @Override | ||
| public void print(List<String> list) { | ||
| list.forEach(System.out::println); | ||
| } | ||
|
|
||
| @Override | ||
| public void print(String string) { | ||
| System.out.println(string); | ||
| } |
| import static org.assertj.core.api.Assertions.assertThatNoException; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| class StringUtilTest { |
There was a problem hiding this comment.
성공과 실패에 대한 테스트 좋습니다.
조금 더 다양한 케이스를 줘봐도 될거같아요. a11나 12345a1304 등과 같은?
| } | ||
|
|
||
| @DisplayName("0으로 나누면 예외를 발생한다.") | ||
| @Test |
There was a problem hiding this comment.
저처럼 당황하실까바
https://careerly.co.kr/comments/70433
|
|
||
| import static org.assertj.core.api.Assertions.*; | ||
|
|
||
| class InfixExpressionTest { |
|
|
||
| import java.util.List; | ||
|
|
||
| public interface Service { |
There was a problem hiding this comment.
네이밍이 아쉬운거같아요.. 무슨 Service인지..
혹시 Service도 인터페이스를 사용한 이유가 궁금합니다.
There was a problem hiding this comment.
안녕하세요 준호 님. 라이브 코드리뷰 피드백 반영하시느냐 고생 많으셨습니다.
pr도 깔끔하고 정성스럽게 올려주셔서 읽기 편했습니다! LGTM
아쉬운 부분들이 있어 리뷰로 몇자 남겼습니다! 확인 부탁드립니다.
설계에 있어 준호님이 꼼꼼하게 노력하신 면이 보입니다.
클래스는 많지만 코드를 읽기가 편했어요!
그리고 테스트를 꼼꼼히 짜신것으로 보이는데요 아주좋아요!
그런데, service, repository, operator 등등에 대한 테스트가 빠진것 같아 아쉬운 부분이 있습니다.
같이 채워보면 어떨까요?
단위테스트도 중요하지만, 여러 객체가 맞물려 함께 협력하는 것을 테스트하는 통합테스트도 중요합니다.
—
- 추상화를 시킴에 있어서 추상클래스를 사용하는 방법과 인터페이스를 사용하는 방법중 어떤 방법이 좀 더 괜찮은 방법인지 모르겠습니다.
- 추상 클래스를 통한 상속은 is-a 관계일 때만 사용해야 하는 것으로 알고 있습니다.
- 다중 상속 및 상태값 유무의 차이점은 알고 있습니다.
- 개인적으로 상속을 사용하면 상태 값을 가질 수 있는데 부모 클래스에서 예외가 발생한다면 응집도가 낮아 진다고 생각하여 인터페이스 사용이 더 괜찮다 생각하는데 어떻게 생각하시나요?
말씀하신대로, is-a, has-a 관계를 생각한다면, 상속과 포함에 대해 조금 더 생각하기 쉬워집니다!
추상클래스와 인터페이스의 차이점, 둘의 추구하는 바가 무엇인가를 생각해보시면 설계에 조금 더 도움이 될거에요.
아무래도 인터페이스가 조금 더 유연성을 높이기는 좋을거에요
저는 오히려 상속을 사용하면 응집도가 조금더 높아진다고 생각하는데 이유는 다음과 같습니다.
부모클래스에서 예외가 발생하는 경우 하위 클래스가 해당 예외를 처리하도록 강제하면 되기 때문이죠.
인터페이스는 구현체 클래스들이 동일한 동작을 보장하고 일관성을 유지해야 할 때 사용하는 것이 좋고,
상속은 상태 값을 가질 수 있으므로 오히려 응집도를 높일 수 있다고 생각해요. 때문에 공통 동작을 재사용하거나 확장할때에는 상속을 이용하는것이 나을 수 있겠죠
둘은 이런 차이가 있으므로 상황에 따라 목적에 맞게 사용하시면 될 것 같습니다.
—
- Controller의 run() 안에서 indent가 깊어 짐에 따라서 메서드를 분리 하려 했는데, indent 만을 줄이기 위한 리펙토링일 뿐 메서드로 try 안의 코드를 boolean type으로 분리시키는 것이 의미 있다 생각하지는 않아 리펙토링 했는데 어떻게 생각하시나요?
저는 가독성이 좋지 않다면 분리시키는 것이 의미가 있다고 생각합니다.
예를들어 다음과 같이도 사용할 수 있겠네요.
준호님이 보시기에는 가독성이 어떠신가요?
public void run() {
boolean isRunning = true;
while (isRunning) {
try {
isRunning = handleCode();
} catch (Exception e) {
console.printErrorMessage(e.getMessage());
}
}
}
private boolean handleCode() {
switch (getCode()) {
case CHECK_DATA:
printAllPreviousData();
break;
case CALCULATE:
printCalculatedResultAndSave();
break;
case EXIT:
exitProgram();
return false; // 종료 조건을 만족할 때 false 반환
}
return true; // 종료 조건을 만족하지 않을 때 true 반환
}—
- 입력 값을 double로 받자니 정규식 부터 완전 수정해야 해서.. 감당이 안될 것 같아 정수형으로 범위를 제한 시키는 것은 유지했습니다.
어떤 부분이 감당이 안되는걸까요? 완전 수정해야 한다고 해야 할 일을 안하는게 맞는걸까요?
앞으로 개발하면서 매우 자주 수정해야 하는 경우가 생길거라고 생각합니다.
수정 요구사항이 들어와도 감당 안된다고 개발을 안하는건 아니라고 생각합니다.
생각을 조금 바꾸셔서 수정에 쉽게 구현하는건 어떠신가요?
해당 계산 결과를 다루는 결과값을 객체로 만든다면 내부적으로 어떻게 동작하는지는 알 필요 없이 수정할 수 있지 않을까요?
—
엘리강트 오브젝트 책에 따르면 -er로 끝나는 이름을 사용하지 마세요 라는 파트가 있습니다.
클래스의 이름은 무엇을 하는지(what he does)가 아니라 무엇(what he is) 인지에 기반해야 한다고 나와있어 InfixExpression 클래스에서 중위 표기식을 후위 표기식으로 변환하는 책임을 할당했지 ~Converter 또는 ~Parser와 같은 클래스 사용을 지양했습니다. 이 부분에 대해서는 어떻게 생각하시나요?
(위와 같이 클래스 이름을 짓지 말라는 이유는 책에 따르면 객체가 아니라 데이터를 다루는 절차의 집합일 뿐이라 객체 지향적이지 못하다는 이유라고 책에서는 말하고 있습니다)
잘하신거같아요! 명확하게 이름을 지으면 역할과 책임에 대해 이해하기도 쉬워지는 것 같습니다.
가독성과 유연성도 향상되고 팀원들이 봤을 때 이해하기 빠르면 효율성을 향상시킨거라고 생각해요!
좋은 이름을 짓기 위해 여러 정보를 참조하여 도전해보시는 모습 좋습니다! 앞으로도 노력해주세요
| private void printAllPreviousData() { | ||
| List<String> calculatedData = service.findAll(); | ||
| console.printExpressions(calculatedData); | ||
| } |
There was a problem hiding this comment.
데이터라고 �명명하는 것은 메소드 기준에서는 다소 추상적인것 같아요. 데이터가 어떤 데이턴지 구현명을 붙이는건 어떨까요? 이 코드 내에서 데이터가 아닌 부분이 없듯이 계산된 데이터 라는 부분이 중간에 계산된 부분인지 결과인지 파악하기 어려운 것 같습니다.
| package com.programmers.blackdog.domain; | ||
|
|
||
| @FunctionalInterface | ||
| public interface Operable { |
| import com.programmers.blackdog.domain.expression.Expression; | ||
|
|
||
| public interface AbstractCalculator { | ||
| int calculate(Expression expression); |
There was a problem hiding this comment.
계산 결과가 단순 int value로 리턴되기보다 객체로 바라볼 순 없을까요?
| public int calculate(Expression expression) { | ||
| String postfixExpression = expression.getPostfixExpression(); | ||
| return calculateWithPostfixExpression(postfixExpression); | ||
| } |
There was a problem hiding this comment.
이 계산기의 구현은 postfix 기반의 계산에만 고려되어 있군요! 만약 xxfix의 계산이 추가된다면 private 메소드를 이 클래스 내에 추가해야 것 같은데
클래스를 새로 생성하여 주입하고 자동으로 변경될 수 있게 해보셔요 ㅎㅎ
| public String getPostfixExpression() { | ||
| StringBuilder postfixExpression = new StringBuilder(); | ||
| Stack<ArithmeticOperators> stack = new Stack<>(); | ||
|
|
||
| String[] tokens = this.expression.split(DELIMITER); | ||
| for (String token : tokens) { | ||
| convertToPostfix(postfixExpression, stack, token); | ||
| } | ||
| appendAllExistingElement(postfixExpression, stack); | ||
| return postfixExpression.toString().trim(); | ||
| } |
There was a problem hiding this comment.
postfix 표현식으로 Expression 자체가 스스로를 변화하는게 좋을까요?
아니면 타 구현에 의해 Expression이 변화되는게 좀더 맞을까요?
제가 Calculator 클래스에 남긴 리뷰내용과 일치하는 부분이기도 합니다 ㅎㅎ
| import static com.programmers.blackdog.domain.ArithmeticOperators.isNotOperator; | ||
| import static com.programmers.blackdog.domain.constant.Regex.*; | ||
|
|
||
| public class InfixExpression implements Expression { |
There was a problem hiding this comment.
살짝 애매한게 Expression은
data인가요? logic인가요?
둘다인가요?
| public interface CalculatorRepository { | ||
| void save(String calculatedResult); | ||
| List<String> findAll(); | ||
| } |
There was a problem hiding this comment.
이건 아마 IDE에서 띄워주셔서 이렇게 커밋된걸수도 있을듯 합니다 ㅎㅎ (= 본인..)
| } | ||
|
|
||
| @DisplayName("0으로 나누면 예외를 발생한다.") | ||
| @Test |
There was a problem hiding this comment.
저처럼 당황하실까바
https://careerly.co.kr/comments/70433
|
추상화를 시킴에 있어서 추상클래스를 사용하는 방법과 인터페이스를 사용하는 방법중 어떤 방법이 좀 더 괜찮은 방법인지 모르겠습니다. Controller의 run() 안에서 indent가 깊어 짐에 따라서 메서드를 분리 하려 했는데, indent 만을 줄이기 위한 리펙토링일 뿐 메서드로 try 안의 코드를 boolean type으로 분리시키는 것이 의미 있다 생각하지는 않아 리펙토링 했는데 어떻게 생각하시나요? 입력 값을 double로 받자니 정규식 부터 완전 수정해야 해서.. 감당이 안될 것 같아 정수형으로 범위를 제한 시키는 것은 유지했습니다. (위와 같이 클래스 이름을 짓지 말라는 이유는 책에 따르면 객체가 아니라 데이터를 다루는 절차의 집합일 뿐이라 객체 지향적이지 못하다는 이유라고 책에서는 말하고 있습니다) |
📌 과제 설명
핵심 기능 목록은 다음과 같습니다.
1. 저장기능
2. 계산기능
3. 계산 제한 기능
👩💻 요구 사항과 구현 내용
.gitignore파일 설정을 통해 의미 없는 코드는 커밋 이력에서 제외 하였습니다.기능목록
README에 작성하고 domain 객체들은 TDD로 개발하였습니다.OOP에 따라 확장성 있는 코드를 작성하기 위해 노력하였습니다.
MVC 패턴을 적용해 개발하였습니다.
모든 domain 객체들에 대해서 테스트를 작성하였습니다.
가독성이 높고 깨끗한 코드를 작성하기 위해 노력하였습니다.
정규식을 학습하고, 사칙연산을 제한하는 기능을 구현하였습니다.
계산 이력을 List로 데이터 저장기능을 만들었습니다.
연산자들을 enum으로 모으고 연산 기능 인터페이스를 override 함으로서 응집도 높게 연산자들을 관리 할 수 있게 구현하였습니다.
꼼꼼한 예외 처리를 해 주었습니다.
도메인 객체의 경우 private 생성자를 제외한 모든 메서드를 테스트 하여 테스트 커버리지를 높혔습니다.
테스트 커버리지는 다음과 같습니다.
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
is-a관계일 때만 사용해야 하는 것으로 알고 있습니다.run()안에서 indent가 깊어 짐에 따라서 메서드를 분리 하려 했는데, indent 만을 줄이기 위한 리펙토링일 뿐 메서드로 try 안의 코드를 boolean type으로 분리시키는 것이 의미 있다 생각하지는 않아 리펙토링 했는데 어떻게 생각하시나요?엘리강트 오브젝트책에 따르면-er로 끝나는 이름을 사용하지 마세요라는 파트가 있습니다.클래스의 이름은
무엇을 하는지(what he does)가 아니라무엇(what he is)인지에 기반해야 한다고 나와있어 InfixExpression 클래스에서 중위 표기식을 후위 표기식으로 변환하는 책임을 할당했지~Converter또는~Parser와 같은 클래스 사용을 지양했습니다. 이 부분에 대해서는 어떻게 생각하시나요?(위와 같이 클래스 이름을 짓지 말라는 이유는 책에 따르면 객체가 아니라 데이터를 다루는 절차의 집합일 뿐이라 객체 지향적이지 못하다는 이유라고 책에서는 말하고 있습니다)
흑구님 영수님
꼼꼼한 리뷰 항상 감사드립니다! 많이 배우겠습니다! 👍 😁