[4기 - 정성현] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #169
Conversation
There was a problem hiding this comment.
안녕하세요 성현님!
java 시작하신지 얼마 안되셨다고 하셨지만 이해도가 깊으신 것 같아서 놀랐습니다.
리뷰 확인해보시고 리팩토링해보시길 바랍니다~!
계산기에서 통과하지 못하는 케이스는 아래 노션에 정리해두었습니다.
https://www.notion.so/backend-devcourse/6cbd3fae15f94a96acc398bc5a436035
질문주신 부분에 대한 제 생각은 아래에 답변을 적을게요.
첫 번째 커밋을 통해서 전반적인 요구 기능을 구현하고, 다음 커밋을 통해서 메서드와 클래스를 분리함으로써 객체지향적인 코드를 작성하고자 했습니다. 이러한 방향성이 "객체지향적인 코드로 계산기 구현하기"라는 문제의 요구사항에 맞는 것인지, 혹시 잘못된 접근 방법은 아닌지 궁금합니다!
객체지향에 익숙하지 않다면 먼저 절차지향적으로 코딩한 후에 리팩토링하는 방법도 괜찮다고 생각합니다.
좀 더 숙련되면 먼저 코드를 치기 전에 클래스 설계를 고민하게 되는 자신을 발견하게 됩니다.
다음으로 "기능을 Commit 별로 잘게 쪼개고, Commit 별로 설명" 하라는 요구사항이 있었는데, 아래와 같은 커밋이 해당 방침을 준수한 것인지, 아니라면 어떻게 수정하면 좋을지 궁금합니다!
현재 커밋 메시지 내용이 좀 포괄적인 느낌이 있는데요, 계산기에서 정확히 어떤 기능을 구현했는지 바로 알기 어렵습니다.
커밋 메시지는 최대한 자세히 적어주시고, 커밋은 많이해도 되니 기능별로 잘게 쪼개서 커밋해보시면 될것같습니다.
(설명이 힘들어 예시를 적어놓겠습니다..ㅋㅋㅋ)
- 메뉴 입력 검증 클래스 구현
- 입출력 클래스 구현
- 계산기에서 ~~한 오류 수정
아 추가로 PR 생성하실 때 assignees를 자신으로 지정하는 습관을 들여야 합니다!
| public static void main(String[] args) { | ||
| Console console = new Console(); | ||
| History history = new History(); | ||
| new Calculator(console, console, history).run(); |
There was a problem hiding this comment.
- 하나의 Console 객체로 input, output 의존성을 따로 주입해야할 이유가 있을까요?
- 만약 따로 주입해야 한다고 생각하신다면, Console 객체가 input + output 인터페이스를 모두 구현할 이유가 있을까요?
There was a problem hiding this comment.
리뷰 감사합니다! 먼저
- 하나의 Console 객체로 의존성을 따로 주입한 이유는 이 객체가 입력과 출력이라는 두 가지 역할을 담당한다고 생각했기 때문입니다!
- 요구 사항 분석을 통해, 현재 시스템에서 입력과 출력이 모두 콘솔을 통해 이루어진다고 생각해 Console 객체가 Input과 Output 인터페이스를 모두 구현하게끔 만들었습니다. 그러나 말씀 주신 내용을 고민해 보니, 현재 한 클래스가 다양한 책임을 지고 있는 같아 이 부분은 수정이 필요할 것 같습니다..!
| import java.util.Map; | ||
|
|
||
| @RequiredArgsConstructor | ||
| public class Calculator implements Runnable { |
There was a problem hiding this comment.
Calculator 클래스가 run이라는 메서드를 통해 어떤 동작을 실행할 수 있는 객체임을 명시하려는 의도로 Runnable 인터페이스를 구현했습니다. 하지만 현재 해당 프로그램이 단일 스레드로 동작하는 만큼, 메인 메서드에서 Calculator 메서드를 직접 호출하는 방식을 사용하는 것이 보다 나을 것 같습니다!
There was a problem hiding this comment.
제 코멘트는 그냥 질문이였습니다 ㅎㅎ
다수의 사용자가 동시에 계산기를 사용한다는 요구사항이 있다면.. Runnable을 상속해 새로운 스레드로 실행시키는 방법도 괜찮을것 같아요~~
| switch (selectPage) { | ||
| case "1" -> list(); | ||
| case "2" -> calculate(); | ||
| default -> printer.println("Invalid page!"); | ||
| } |
There was a problem hiding this comment.
switch 표현식을 사용하셨군요~ 좋습니다! (이 문법은 jdk 몇 버전에서 새로 나왔을까요?)
1, 2와 같이 자체적으로 의미를 알아볼 수 없는 숫자을 매직넘버라고 합니다.
다른 개발자가 코드를 봤을 때, 해당 숫자의 의미를 바로 알아채기 힘들기 때문에 이런 숫자는 의미를 부여해 주는게 좋습니다.
(범석님 리뷰 참고)
enum과 같은 값으로 따로 빼주는게 어떨까요?
각 case 구문에서 break가 걸려있지 않아 while문을 돌때마다 모든 case 구문이 실행되고 있습니다.
break를 추가해주세요~
There was a problem hiding this comment.
스위치 표현식이 본격적으로 도입된 것은 Java 14 부터 인 것으로 알고 있습니다!(..!)
말씀 주신 대로 수정하도록 하겠습니다!
| } | ||
| } | ||
|
|
||
| public void calculate() { |
There was a problem hiding this comment.
하나의 메소드에서 너무 많은 일을 하고 있는것 같습니다.
기능을 적절하게 나눠서 메소드로 분리하면 좋을것 같아요~
|
|
||
| public void calculate() { | ||
| String formula = scanner.nextLine(); | ||
| if (!Pretreatment.validateFormula(formula)) return; |
There was a problem hiding this comment.
아무 액션 없이 리턴하는것보다 적절한 오류 문구를 추가하는게 좋을 것 같습니다.
이건 Pretreatment 클래스쪽에서 다시 리뷰할게요!
| public class Console implements Input, Output { | ||
| private final Scanner scanner = new Scanner(System.in); | ||
| @Override | ||
| public String nextLine(){ | ||
| return scanner.nextLine(); | ||
| } | ||
| @Override | ||
| public String nextLine(String prompt){ | ||
| System.out.print(prompt); | ||
| return scanner.nextLine(); | ||
| } | ||
|
|
||
| @Override | ||
| public void println() { | ||
| System.out.println(); | ||
| } | ||
| @Override | ||
| public void println(String prompt) { | ||
| System.out.println(prompt); | ||
| } | ||
| } |
There was a problem hiding this comment.
input과 output 자체가 scanner / print를 단순히 래핑하고 있다는 생각이 드네요.
계산기라는 기능(도메인)에 한정된 input과 output을 만든다면 어떻게 하면 좋을지 고민해보시고... 리팩토링해보면 좋을것 같습니다!
| public static boolean validateFormula(String formula) { | ||
| if (!formula.matches("^(-*\\d\\s[+\\-*/]\\s)+\\d$")) { | ||
| System.out.println("Invalid formula!"); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
해당 메소드가 bool 값을 리턴하니 네이밍은 isValidFormula가 더 적절해 보입니다!
다만, 수식 포맷을 검증하고 bool 값을 리턴하는게 아니라 Exception을 throw 해보는건 어떨까요? (실습삼아)
던져진 예외를 메소드 사용부에서 catch해서 사용하는 방식으로 변경해보세요!
( + 커스텀 Exception을 만들어 보는것도 추천합니다)
| @@ -0,0 +1,21 @@ | |||
| package me.kimihiqq; | |||
There was a problem hiding this comment.
유틸성 클래스는 utils라는 패키지에 모아놓는것이 국룰입니다 ㅋㅋㅋ
| public class Pretreatment { | ||
|
|
There was a problem hiding this comment.
유틸 클래스는 static하게만 존재하는게 좋습니다.
따라서 private 생성자로 외부에서 객체를 생성하지 못하도록 막아버리는 게 안전합니다!
SonarLint가 추천해준 코드입니다. (플러그인 까셨죠?)
private Pretreatment() {
throw new IllegalStateException("Utility class");
}| public Map<Integer, String> getAll() { | ||
| return history; | ||
| } |
There was a problem hiding this comment.
history 자체를 반환하게 되면 메소드 사용자 측에서 history 객체를 받고 값을 임의로 조정하는 등의 몹쓸짓?을 할 수도 있습니다.
따라서 History 클래스 자체를 일급 컬렉션으로 만드는게 좋습니다.
(일급 컬렉션이란?)
history 객체를 그대로 리턴하는게 아니라, 깊은 복사를 통해 리턴하거나 다른 형태로 변환하여 리턴하는게 안전합니다.
| return option; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
감사합니다! 현재 코드는 NullPointerException을 발생시킬 위험이 있는 것 같습니다. 수정하도록 하겠습니다!
| int i = 0; | ||
| while (i < terms.size()) { |
There was a problem hiding this comment.
이럴 경우엔 차라리 for문 혹은 향상된 for문을 쓰거나 terms을 stream으로 처리할것 같아요.
| return Arrays.stream(arr).collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public static void isValidFormula(String formula) { |
There was a problem hiding this comment.
리턴값 없이 검증만 실시하는 메소드 네이밍은 validateFormula가 더 적절할것 같아요.
is~가 붙으면 보통 boolean을 리턴합니다
There was a problem hiding this comment.
감사합니다. 해당 부분 수정하도록 하겠습니다!
| private long calculateOperation(String operator, long leftHandSide, long rightHandSide) { | ||
| switch (operator) { | ||
| case "*": | ||
| return leftHandSide * rightHandSide; | ||
| case "/": | ||
| return leftHandSide / rightHandSide; | ||
| case "+": | ||
| return leftHandSide + rightHandSide; | ||
| case "-": | ||
| return leftHandSide - rightHandSide; | ||
| default: | ||
| throw new IllegalArgumentException("Invalid operator"); | ||
| } | ||
| } |
There was a problem hiding this comment.
우선 소수점을 표현하기 위해 long보다 float, double, BigDecimal을 사용해주세요.
그리고 이 부분도 메뉴때와 마찬가지로 enum으로 분리하면 좋을것 같습니다.
enum 필드값에는 익명함수(람다)가 일급객체로 들어갈 수가 있는데요, (이펙티브 자바에서 나옵니다)
이를 활용해서 다음과 같이 깔끔한 코딩이 가능합니다.
public enum Operator {
MULTIPLY("*", (a, b) -> a * b),
DIVIDE("/", (a, b) -> a / b),
ADD("+", (a, b) -> a + b),
SUBTRACT("-", (a, b) -> a - b);
private final String operatorString;
private final BiFunction<Integer, Integer, Integer> calculation;
public double calculate(double leftHandSide, double rightHandSide) {
return calculation.apply(a, b);
}
}함수형 프로그래밍이 이런 코딩방식과 비슷하다고 보시면 됩니다.
There was a problem hiding this comment.
말씀주신대로 enum으로 분리하니, 코드가 훨씬 깔끔해진것 같습니다. 수정하도록 하겠습니다!
|
|
||
| import java.util.Optional; | ||
|
|
||
| public enum Option { |
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| public class CalculatorTest { |
There was a problem hiding this comment.
mocking까지 활용하시다니 매우 굿입니다!
다만 '결과'만 테스트하는것이 아닌 '과정' 또한 테스트가 필요합니다
(메뉴 입력 혹은 파싱 등등..)
그리고 엣지케이스가 부족한 것 같습니다.
비정상결과를 출력할 것 같은 테스트케이스도 고려하는게 좋습니다~
이번 과제보다는 다음 과제를 수행하실때 피드백을 고려해서 테스트해보세요!
There was a problem hiding this comment.
말씀 주신 방향으로, 엣지케이스와 과정을 고려해 테스트를 수정했습니다. 리뷰 감사합니다!
📌 과제 설명
미션의 요구사항에 맞춰 객체지향적인 코드에 기반한, 콘솔로 작동하는 계산기를 구현해 보았습니다!
👩💻 요구 사항과 구현 내용
이번 미션의 요구사항에 맞추어 다음과 같은 기능을 구현했습니다 :
먼저 첫 번째 커밋인 [요구기능 구현]을 통해서는 전반적인 요구 기능을 구현했으며, [메서드 및 클래스 분리] 커밋에서는 가독성과 유지 보수성을 향상시키고자, 메서드와 클래스를 분리했습니다. 마지막으로 계산기의 메서드를 검증하는 [테스트 코드] 커밋을 작성했습니다.
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
첫 번째 커밋을 통해서 전반적인 요구 기능을 구현하고, 다음 커밋을 통해서 메서드와 클래스를 분리함으로써 객체지향적인 코드를 작성하고자 했습니다. 이러한 방향성이 "객체지향적인 코드로 계산기 구현하기"라는 문제의 요구사항에 맞는 것인지, 혹시 잘못된 접근 방법은 아닌지 궁금합니다!
다음으로 "기능을 Commit 별로 잘게 쪼개고, Commit 별로 설명" 하라는 요구사항이 있었는데, 아래와 같은 커밋이 해당 방침을 준수한 것인지, 아니라면 어떻게 수정하면 좋을지 궁금합니다!
감사합니다!