Lotto step1 by Highjune · Pull Request #3040 · next-step/java-lotto

Highjune · 2023-05-01T04:50:40Z

안녕하세요! 새로운 리뷰어님. 잘 부탁드립니다^^

질문드릴 것이 있습니다.

validateDivine() 함수에서요!. 원래는 0으로 나누게 되면 자바가 알아서 ArithmeticException 에러를 발생시키는데요. 이런 경우에는 저처럼 바로 ==0 일 경우에 throw 로 에러 발생하는 것보다, try catch(ArithmeticException Exception) 로 실제로 발생했을 경우에 throw 에러 발생시키거나 다른 동작을 취하는 것이 나을까요?

무엇이든 가감없는 피드백 부탁드립니다!^^
감사합니다.

- 공백으로 구분해서 사칙연산을 수행할 수 있다.

- 사칙 연산이 아닌 문자가 아닌 경우 에러 발생

- 입력 값이 null이거나 빈 공백 문자일 경우 에러를 발생한다.

catsbi

안녕하세요 강준님 로또 미션을 함께하게 된 이한솔이라고 합니다.
첫 번째 미션인 문자열 계산기 잘 구현해주셨어요. 몇 가지 피드백이 있는데 확인하고 넘어가면 좋을 것 같아 코멘트 남겨드렸으니 확인 후 다시 리뷰요청 부탁드려요 👍

catsbi · 2023-05-01T23:58:28Z

+- [x] 공백으로 구분해서 사칙연산을 수행할 수 있다.
+- [x] 사칙연산이 아닌 경우 에러를 발생한다.
+- [x] 나눗셈일 경우 0으로 나누면 에러를 발생한다.
+- [x] 입력 값이 null이거나 빈 공백 문자일 경우 에러를 발생한다.


checklist 작성 👍

catsbi · 2023-05-01T23:59:25Z

+        int result = toInt(separatedString[0]);
+
+        for (int i = 0; i < separatedString.length - 2; i += 2) {
+            result = calculate(result, separatedString[i + 1].charAt(0), toInt(separatedString[i + 2]));


매개변수가 3개네요. 2개로 줄일 수 없을까요?

catsbi · 2023-05-02T00:01:21Z

+
+    public int calculateString(String input) {
+        validateInput(input);
+        String[] separatedString = StringUtil.splitString(input);


메서드 시그니처만 봐서는 문자열을 공백(blank)으로 나누는지 파악하기 힘듭니다.

xxxUtil같은 클래스 설계는 지양해주세요. 이름 자체가 말하듯 범용성을 내포하고 있기에 여러 책임이 같이 맞물리게되고, 특정 구현체들의 요구조건에 맞추다보면 코드가 난잡해지기 쉬운 객체가 되거든요. 해당 기능같은경우 최대한 JDK 기본라이브러리가 제공하는걸 사용하거나 자체적으로 구현 혹은 공통되는부분이 꽤 있다면 차라리 인터페이스와 구현체 혹은 열거타입을 활용할수 있습니다.

@catsbi 말씀하신 메서드 시그니처는 StringUtil의 splitString() 의 네이밍을 말씀하시는 것 맞을까요?

네 맞습니다!

catsbi · 2023-05-02T00:03:09Z

+    private int calculate(int firstOperand, Character operator, int secondOperand) {
+        if (operator == '+') {
+            return add(firstOperand, secondOperand);
+        }
+        if (operator == '-') {
+            return subtract(firstOperand, secondOperand);
+        }
+        if (operator == '*') {
+            return multiply(firstOperand, secondOperand);
+        }
+        if (operator == '/') {
+            validateDivine(secondOperand);
+            return divine(firstOperand, secondOperand);
+        }
+        throw new IllegalArgumentException("4칙 연산자가 아닌 문자가 입력되었습니다.");
+    }
+
+    private int add(int a, int b) {
+        return a + b;
+    }
+
+    private int subtract(int a, int b) {
+        return a - b;
+    }
+
+    private int multiply(int a, int b) {
+        return a * b;
+    }
+
+    private int divine(int a, int b) {
+        return a / b;
+    }


사실 사칙연산같은경우 4가지로 명확하기에 이렇게 작성을해도 문제가 안될 수 있습니다. 하지만, 일반적으로는 태그를 이용해 메서드내에서 분기하는것은 권장되지 않아요. 하나의 메서드가 책임져야 할 분기(조건)들이 너무 많아지기도하고, 차후 유지보수성도 떨어지거든요.

이런 로직의 경우 열거타입과 표준함수형 인터페이스 등을 활용해서 로직을 추상화 할 수 있을 것 같습니다.

catsbi · 2023-05-02T00:04:19Z

+
+    private void validateDivine(int divineNumber) {
+        if (divineNumber == 0) {
+            throw new IllegalArgumentException("0으로 나눌 수는 없습니다.");


원래 0으로 나눌 경우에 던져지는 예외는 ArithmeticException이죠.
이걸 다른 예외로 번역해서 혼동을 주기보다는 해당 예외(ArithmeticException)를 활용하되 메세지만 추가해주는 방법이 나을 수 있습니다.

catsbi · 2023-05-02T00:04:35Z

+    private int toInt(String stringTobeInt) {
+        return Integer.parseInt(stringTobeInt);
+    }


굳이 메서드로 뺄 이유가 없는 로직같습니다.

catsbi · 2023-05-02T00:05:29Z

+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+public class StringCalculatorTest {


테스트 케이스를 작성할 때 어떻게 케이스를 뽑아야할 지 모르겠을 때는 다음 링크를 참고해보시면 좋을 것 같아요(link)

- Integer.parseInt()로 굳이 나눈 메서드 삭제 - 함수 파라미터 3개 -> 2개 수정 - Number 클래스로 감싸기 - 사칙연산 Enum으로 정의 & 표준함수형 정의 - 테스트 2가지로 추가 - UI 분리 - 패키지 분리

catsbi

강준님 피드백 반영 잘 해주셨습니다!
추가적인 피드백 조금 남겼으나 미미한 것들이라 다음 미션에서 같이 반영해보면 좋을 것 같네요.

1단계는 여기서 merge하겠습니다.

catsbi · 2023-05-03T16:54:56Z

+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;


단 한줄짜리라도 블럭{} 생략은 지양해주세요 코드의 일관성과 가독성을 낮춥니다.
IDE에 의해 자동생성된 코드도 예외대상이 아닙니다.

catsbi · 2023-05-03T16:56:06Z

+
+    private static final String ERROR_CANNOT_DIVINE_BY_ZERO = "0으로 나눌 수는 없습니다.";
+    private static final String ERROR_CANNOT_FIND_ARITHMETICOPERATOR = "해당 사칙연산과 일치하는 연산자를 찾을 수 없습니다.";
+    private final Character arithmeticOperator;


primitive타입이 아닌 Wrappter타입을 쓴 별도의 이유가 있을까요?
자바에선 제네릭과 같이 특수한 경우가 아니라면 primitive 타입을 권장하는 편입니다.

@catsbi
아 딱히 없었습니다.
매번 고민이었는데

자바에선 제네릭과 같이 특수한 경우가 아니라면 primitive 타입을 권장하는 편입니다. => 너무 감사합니다.!

catsbi · 2023-05-03T16:57:00Z

+    }
+
+    private Number calculateEachNumber(String[] separatedString) {
+        double result = Integer.parseInt(separatedString[0]);


separatedString[0]이 숫자 타입이 아니거나 값이 없을 수 있습니다. 이에 대한 유효성 검증은 필요 없을까요?

Highjune added 3 commits May 1, 2023 13:12

feat(문자열 생성기): 최소의 기능 요구사항 구현

f8941ca

- 공백으로 구분해서 사칙연산을 수행할 수 있다.

test(StringCalculatorTest): 사칙연산 아닐 경우 에러 발생

a1b6def

- 사칙 연산이 아닌 문자가 아닌 경우 에러 발생

feat(StringCalculator): 입력값 유효성 체크

0a691f4

- 입력 값이 null이거나 빈 공백 문자일 경우 에러를 발생한다.

catsbi requested changes May 2, 2023

View reviewed changes

feat(All): step1의 피드백 반영

f439d9b

- Integer.parseInt()로 굳이 나눈 메서드 삭제 - 함수 파라미터 3개 -> 2개 수정 - Number 클래스로 감싸기 - 사칙연산 Enum으로 정의 & 표준함수형 정의 - 테스트 2가지로 추가 - UI 분리 - 패키지 분리

catsbi approved these changes May 3, 2023

View reviewed changes

catsbi merged commit c26a356 into next-step:highjune May 3, 2023

Conversation

Highjune commented May 1, 2023

Uh oh!

catsbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catsbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants