-
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주차 과제: 계산기 구현 미션 제출합니다. #156
base: Tommy
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 정말 수고하셨습니다~~
OOP를 적절하게 잘 사용하신것 같아서 전체적인 구조가 좋았습니다 👍
그래서 코드를 클린하게 작성하는 방법 위주로 리뷰를 달았습니다.
PR 유지한채로 리뷰 드린 부분 리팩토링해서 커밋하시면 될것 같습니다~
또 계산이 잘 되는지부터 여러 케이스를 확인해봤는데, 기대하는 값이 안나오는 경우가 좀 있어서 일단 계산 로직 수정이 필요해 보입니다.
계산 로직같은 부분은 개선이 필요해 보이는데, 우선 이번 리뷰부터 리팩토링한 이후에 다시 개선해보도록 해요! (라이브 리뷰 할수도...)
메뉴 선택시
0
1
+ x
abc x
a + a x
더하기
1 + 1
0 + 0
0 + 1
1 + 0
-1 + 2
-2 + 1
-1 + 0
빼기
2 - 1 x
1 - 2 x
0 - 0
1 - 0 x
0 - 1
-1 - 1
-1 - 0
곱하기
1 * 1
1 * 2
0 * 1
1 * 0
0 * 0
-1 * 2
-1 * 0
나누기
1 / 2
1 / 3
2 / 1
1 / 1
0 / 0 x
1 / 0 x
-1 / 2
-2 / 1
-1 / 3
-1 / 0 x
Input input = new Console(); | ||
Output output = new Console(); | ||
CalculateEngine calculateEngine = new CalculateEngine(); | ||
CalculateRepository calculateRepository = new CalculateRepositoryImpl(); | ||
new Calculator(calculateEngine, input, output, calculateRepository).run(); |
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 클래스가 input + output 인터페이스를 모두 구현하고 있는데, 따로 의존성을 주입해줄 필요가 있을까요?
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로 넣을까 각각의 Interface로 넣을 까 고민 했었는데
Console로 넣는게 바람직 한 것 같습니다!
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로 넣는게 바람직 한 것 같습니다!
하나의 클래스가 여러 책임을 가지고 있는것 같아요. 계산기 과제에서 나눠야 할 관심사(책임)이 무엇인지 고민이 필요해 보입니다.
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올릴 때 수정하여 올리도록 하겠습니다 감사합니다!
String stringCondition = validateInput(); | ||
Condition condition = validateCondition(stringCondition); |
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.
메소드 네이밍을 보고 함수 내부에서 발생하는 동작을 바로 유추하기 쉽지 않을것 같아요.
validateInput
메소드 내부에서 실질적으로 수행하는건
- 조건 안내 문자열을 출력
- 사용자가 조건 입력
- 조건을 검증
이렇게 세가지를 수행하는데, 네이밍이 '입력을 검증한다' 라는 의미만 담고 있어서 그런것 같습니다.
그리고 사용자가 입력한 '조건 숫자'를 검증하는 행위 또한 validateInput
과 validateCondition
메소드에서 똑같이 수행하는것 같습니다.
사용자에게 입력을 받고, 그 입력을 한번에 enum으로 변환해 검증하는게 가독성 측면에서 좋아 보입니다!
|
||
private String validateInput() { | ||
String stringCondition = input.printCondition().orElse("wrong"); | ||
if (stringCondition == "wrong") { |
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.
if (stringCondition == "wrong") { | |
if ("wrong".equals(stringCondition)) { |
String 비교는 equals로 해야합니다! (동일성 vs 동등성)
그리고 null이 들어갈 수 있는 값을 equals 메소드의 파라미터로 넣어주는 습관을 가져야 NPE를 방지할 수 있습니다~~
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.
org.apache.commons.lang3.StringUtils.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.
감사합니다!
public enum Condition { | ||
LOOKUP, CALCULATE, BREAK; | ||
|
||
public static Optional<Condition> decideCondition(String condition) { | ||
if (condition.equals("1")) { | ||
return Optional.of(LOOKUP); | ||
} | ||
if (condition.equals("2")) { | ||
return Optional.of(CALCULATE); | ||
} | ||
if (condition.equals("3")) { | ||
return Optional.of(BREAK); | ||
} | ||
return Optional.empty(); | ||
} | ||
} |
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.
-
매직 넘버
1, 2와 같이 자체적으로 의미를 알아볼 수 없는 숫자을 매직넘버라고 합니다.
다른 개발자가 코드를 봤을 때, 해당 숫자의 의미를 바로 알아채기 힘들기 때문에 이런 숫자는 의미를 부여해 주는게 좋습니다. -
의미 없는 null 사용 지양
decideCondition
메소드에서 정해진 숫자가 아닐경우엔 null을 반환하는데, 이런 의미없는 null의 사용은 지양해야합니다.
알 수 없는 값일 경우를 나타내는 enum을 추가하는게 어떨까요? -
enum에서의 분기처리
현재 if문을 사용해서 조건에 해당하는 enum을 반환해주고 있는데요, stream을 사용해서 더 깔끔한 로직을 작성할 수 있습니다.
1~3번 피드백을 모두 반영하면 아래와 같은 코드가 됩니다!
(첫 리뷰라서 best practice 예시를 남겨드려용)
enum 활용법에 대해 더 공부해보시면 좋을것 같습니다~~!
public enum Condition { | |
LOOKUP, CALCULATE, BREAK; | |
public static Optional<Condition> decideCondition(String condition) { | |
if (condition.equals("1")) { | |
return Optional.of(LOOKUP); | |
} | |
if (condition.equals("2")) { | |
return Optional.of(CALCULATE); | |
} | |
if (condition.equals("3")) { | |
return Optional.of(BREAK); | |
} | |
return Optional.empty(); | |
} | |
} | |
@RequiredArgsConstructor | |
public enum Condition { | |
LOOKUP("1"), | |
CALCULATE("2"), | |
BREAK("3"), | |
EMPTY(null); | |
private final String num; | |
public static Condition decideCondition(String condition) { | |
return Arrays.stream(Condition.values()) | |
.filter(conditionEnum -> conditionEnum.num.equals(condition)) | |
.findAny() | |
.orElse(EMPTY); | |
} | |
} |
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.
제 코드가 best practice 인지는 모르겠지만 ^^ 저라면 이렇게 할 것 같아요.
@RequiredArgsConstructor
public enum Condition {
LOOKUP("1"),
CALCULATE("2"),
BREAK("3");
private final String input;
public static Optional<Condition> convert(String condition) {
return Arrays.stream(Condition.values())
.filter(conditionEnum -> conditionEnum.input.equals(condition))
.findAny();
}
}
- 의미있는 네이밍 : num? 입력값에 따라 메뉴를 결정하는 상황이 아닐지
- 의미 없는 null 사용 지양 : 말 그대로 의미없는 null사용을 지양하기 위해서 더욱이 Optional 을 활용할 것 같아요. 만약 Condition.values() 같은 메소드를 다른곳에서 사용할 상황일때
EMPTY(null)
같은 부분이 있다면 그에 맞는 별도 처리(혹은 예외처리) 가 발생할것 같아요. - Optional 사용 : 리턴 타입이 Optional인 경우 사용하는 쪽에서 NPE을 사전에 방지할수 있을 것 같습니다. 명시적인 오류 발생 전달 (compile level)
- 그리고 메뉴 입력값을 1,2,3 숫자로 받아야만 하는 상황이라면 이런식의 변환작업이 있기 위해 위에
input
같은 코드가 있어야만 하지만 그렇지 않다면 입력값을 문자열로 받는건 어떨지 궁금하네요.
public enum ErrorCode { | ||
BAD_CONDITION; | ||
|
||
public static void printError(ErrorCode errorCode) { | ||
if (errorCode == BAD_CONDITION) { | ||
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.
enum 필드값을 추가해서 enum과 메시지를 매핑시키는게 깔끔할것 같아요~
BAD_CONDITION("잘못된 값을 입력했습니다. 다시 확인해주세요");
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.
또는 constant class
for (int i = 0; i < equation.size(); i++) { | ||
String s = equation.get(i); | ||
|
||
if (s.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.
만약 변수 s
가 null이라면 null.equals()
가 되기 때문에 NPE가 발생합니다.
위에서도 말씀드렸듯이 파라미터 부분에 s
를 넣어주는게 안전합니다~~!
} | ||
} | ||
equation.add(num); | ||
equation.remove(""); |
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.
-1 + 2 와 같이 음수가 들어있는 경우 [, -, 1 , +]와 같이 분할이 되어 빈 문자열이 생겨 삭제 로직을 추가해주었습니다.
|
||
import java.util.List; | ||
|
||
public interface CalculateRepository { |
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.
지금은 계산 로직 저장을 Memory 상에서 저장을 하고 있습니다.
그렇지만 추후에 Database를 연결하거나, Repository 저장 로직을 바꿀 수 있는 가능성이 충분히 많습니다.
그럴 때 유연하게 Repository를 바꾸기 위해 Interface로 추상화 하였습니다.
import java.util.List; | ||
|
||
public class CalculateRepositoryImpl implements CalculateRepository { | ||
private static final List<History> histories = new ArrayList<>(); |
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.
HashMap을 사용해서 뽑아낼 것 같습니다.
CalculateEngine calculateEngine = new CalculateEngine(); | ||
|
||
@ParameterizedTest | ||
@CsvSource({"1 + 1,2.0", "4 * 2 + 10,18.0", " 1 * 3 + 4 / 2,5.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.
엣지케이스를 더 많이 추가하면 좋을것 같습니다!
Input input = new Console(); | ||
Output output = new Console(); | ||
CalculateEngine calculateEngine = new CalculateEngine(); | ||
CalculateRepository calculateRepository = new CalculateRepositoryImpl(); | ||
new Calculator(calculateEngine, input, output, calculateRepository).run(); |
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로 넣는게 바람직 한 것 같습니다!
하나의 클래스가 여러 책임을 가지고 있는것 같아요. 계산기 과제에서 나눠야 할 관심사(책임)이 무엇인지 고민이 필요해 보입니다.
} | ||
|
||
private void runCalculator() { | ||
while (STOPPER) { |
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.
입력에 따라 반복문을 계속 돌릴지 말지를 결정하는것으로 보입니다.
중단을 의미하는 입력이 왔을때 바로 break로 빠져나와도 무방할것으로 보이네요.
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 String validateInput() { | ||
String stringCondition = input.printCondition().orElse("wrong"); | ||
if (stringCondition == "wrong") { |
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.
org.apache.commons.lang3.StringUtils.equals 을 사용하는 방법도 있고요.
import java.util.Optional; | ||
import java.util.Scanner; | ||
|
||
public class Console implements Input, Output { |
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과 output두개를 모두 구현받을 경우 console입장에서는 두가지의 책임을 가지지 않을까요?
} | ||
|
||
private String validateInput() { | ||
String stringCondition = input.printCondition().orElse("wrong"); |
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 을 사용했다면 그에 걸맞는 메소드를 사용하는게 좋아보여요.
e.g. ifPresent
public enum Condition { | ||
LOOKUP, CALCULATE, BREAK; | ||
|
||
public static Optional<Condition> decideCondition(String condition) { | ||
if (condition.equals("1")) { | ||
return Optional.of(LOOKUP); | ||
} | ||
if (condition.equals("2")) { | ||
return Optional.of(CALCULATE); | ||
} | ||
if (condition.equals("3")) { | ||
return Optional.of(BREAK); | ||
} | ||
return Optional.empty(); | ||
} | ||
} |
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.
제 코드가 best practice 인지는 모르겠지만 ^^ 저라면 이렇게 할 것 같아요.
@RequiredArgsConstructor
public enum Condition {
LOOKUP("1"),
CALCULATE("2"),
BREAK("3");
private final String input;
public static Optional<Condition> convert(String condition) {
return Arrays.stream(Condition.values())
.filter(conditionEnum -> conditionEnum.input.equals(condition))
.findAny();
}
}
- 의미있는 네이밍 : num? 입력값에 따라 메뉴를 결정하는 상황이 아닐지
- 의미 없는 null 사용 지양 : 말 그대로 의미없는 null사용을 지양하기 위해서 더욱이 Optional 을 활용할 것 같아요. 만약 Condition.values() 같은 메소드를 다른곳에서 사용할 상황일때
EMPTY(null)
같은 부분이 있다면 그에 맞는 별도 처리(혹은 예외처리) 가 발생할것 같아요. - Optional 사용 : 리턴 타입이 Optional인 경우 사용하는 쪽에서 NPE을 사전에 방지할수 있을 것 같습니다. 명시적인 오류 발생 전달 (compile level)
- 그리고 메뉴 입력값을 1,2,3 숫자로 받아야만 하는 상황이라면 이런식의 변환작업이 있기 위해 위에
input
같은 코드가 있어야만 하지만 그렇지 않다면 입력값을 문자열로 받는건 어떨지 궁금하네요.
public enum ErrorCode { | ||
BAD_CONDITION; | ||
|
||
public static void printError(ErrorCode errorCode) { | ||
if (errorCode == BAD_CONDITION) { | ||
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.
또는 constant class
|
||
public interface CalculateRepository { | ||
|
||
void save(History 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.
save한 결과에 따라 처리할 상황은 없을까요?
|
||
@ParameterizedTest | ||
@ValueSource(strings = {"1", "2", "3", "4", "%"}) | ||
@DisplayName("Condition이 정상적으로 반환 되는지 조회") |
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.
@DisplayName("Condition이 정상적으로 반환 되는지 조회") | |
@DisplayName("Condition이 정상적으로 반환 된다.") |
@mentor-tyler @hanjo8813 Input Output 관심사 통일처음에 Input / Output이라고 했지만 결과적으론 Console에서 받아서 출력을 하는 역할을 담당하므로 메서드명 변경기존에 모호했던 메서드 이름을 변경했습니다. 계산 조건 검증 로직 이동기존에 Calculator 에서 사용자가 입력한 계산 조건을 검증 했던데 반해 == 사용 대신 equals 사용String 비교 시 == 대신 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.
아직 피드백 사항을 모두 반영하시진 않은것 같지만 중간 리뷰 한번 추가하겠습니다~!
@RequiredArgsConstructor | ||
public enum Condition { | ||
LOOKUP("1"), | ||
CALCULATE("2"), | ||
BREAK("3"); | ||
|
||
private final String input; | ||
|
||
public static Optional<Condition> convert(String condition) { | ||
return Arrays.stream(Condition.values()) | ||
.filter(conditionEnum -> conditionEnum.input.equals(condition)) | ||
.findAny(); | ||
} | ||
} |
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.
굿입니다 !! 👍
@CsvSource( | ||
{"1 + 1,2.0", "0 + 0,0.0", "-1 + 2,1.0", "-1 + 0, -1.0", | ||
"2 - 1,1.0", "1 - 2,-1.0", "-1 - 1, -2.0", | ||
"1 / 2,0.5", "2 / 1,2.0","-2 / 1, -2.0", | ||
"4 * 2 + 10,18.0", " 1 * 3 + 4 / 2,5.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.
👍
String condition = scanner.nextLine(); | ||
return Condition.convert(condition).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.
원래 null로 처리하는 코드가 Exception을 던지도록 변경되니 훨씬 자연스러운것 같습니다! 👍
import java.util.Scanner; | ||
|
||
@RequiredArgsConstructor | ||
public class ConsoleImpl implements Console { |
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 메소드가 모두 계산기의 주요 로직에 관여하고 있어 보입니다
따라서 ConsoleImpl 보다는 CalculatorConsole 등의 네이밍이 더 적절해보이네요~!
calculateRepository.getHistory() | ||
.forEach(h -> System.out.println("Equation : " + h.getEquation() + " | result : " + h.getResult())); |
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.format 메소드를 쓰는방법도 알아두세요~~!
calculateRepository.getHistory() | |
.forEach(h -> System.out.println("Equation : " + h.getEquation() + " | result : " + h.getResult())); | |
calculateRepository.getHistory() | |
.forEach(h -> System.out.println(String.format("Equation : %s | result : %s", h.getEquation(), h.getResult())); |
try { | ||
condition = console.getCondition(); | ||
} catch (IllegalArgumentException e) { | ||
System.out.println(e.toString()); | ||
} | ||
return condition; |
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.
Exception으로 처리하기 반영하셨군요! 좋아요~ 👍
📌 과제 설명
👩💻 요구 사항과 구현 내용
[add ErrorCode and Console]
Error 발생 시 사용할 Class와 Input, Output 담당을 할 Console을 구현했습니다.
[ Calculator 기본 기능 추가 ]
계산 기능을 뺀 전체적인 틀을 구현했습니다.
[ 게산 로직 및 저장 기능 추가]
계산 로직을 구현했으며, 계산 후 저장하는 기능을 구현했습니다.
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
전반적으로 매운 맛으로 피드백 부탁드립니다..!🙏