[빙봉] 온보딩 코드리뷰 요청합니다. #17

aegis1920 · 2020-02-07T08:32:56Z

No description provided.

Onboarding

dave915

안녕하세요. 리뷰를 맡은 데이브입니다!
몇가지 피드백 남겼습니다.
고민해 보시고 도움이 필요하시면 DM주세요 😁

dave915 · 2020-02-07T12:20:04Z

src/main/java/ExceptionHandler.java

+/*        for (int i = INDEX_INIT; i < inputStrings.length; i++) {
+            if (checkIndividual(i, inputStrings[i]) == false) {
+                return false;
+            }
+        }*/


필요없는 주석은 지우면 나머지 코드볼때 보기 편할 것 같아요!

이 부분을 지우지 않은 이유가 있는데요!

저의 checkString() 메서드는 공백으로 split한 문자열 중 하나라도 틀리면 false를 return하는 메서드입니다. 그래서 처음에는 주석처리했던 코드인 for문에 if문으로 했는데 depth 2가 되기에 depth 1을 지키고 싶어 stream()의 allMatch를 쓰게되었습니다.

주석처리된 코드를 분리해 depth 1으로 만들고 싶은데 조언을 주실 수 있을까요? (checkIndividual() 메서드의 인자로 인덱스와 문자열이 같이 들어가기에 메서드 분리가 힘들었습니다)

저는 개인적으로 stream()을 쓰지 않고 최대한 메서드의 분리로 depth 1을 구현하고 정 안될 때 stream()을 써서 depth 1을 구현하고 있습니다. 제가 하고 있는 노력에 대해서 방법이 틀리진 않은 건지 궁금합니다 :)

굳이 나누자면

int i = 0; while(inputStrings.length != i && checkIndividual(i, inputStrings[i])) { i++; } return inputStrings.length == i;

이런 식으로도 할 수 있겠네요.
근데 저라면 빙봉이 하신 for-loop 이나 stream을 사용할 것 같습니다. 무엇을 하고싶은지 명시적이지 않거든요...

두번째 질문주신 부분에 대해선 잘 하고 계신 것 같습니다. 다양한 방법으로 생각해 보고 개발 해 보는것이 중요한 것 같아요. 지금 코딩 할때 아니라고 생각했던 부분을 다른곳에서 코딩할때 사용하게 될 때도 있거든요 ㅎㅎ
잘하고 계신겁니다 👍

넵 이해했습니다. 감사합니다!

dave915 · 2020-02-07T12:26:01Z

src/main/java/Calculator.java

+    private static void calculate(double nowNumber) {
+        if (nowSign.equals("+")) {
+            plus(nowNumber);
+            return;
+        }
+        if (nowSign.equals("-")) {
+            minus(nowNumber);
+            return;
+        }
+        if (nowSign.equals("*")) {
+            multiply(nowNumber);
+            return;
+        }
+        if (nowSign.equals("/")) {
+            divide(nowNumber);
+            return;
+        }
+    }
+
+    private static void plus(double nowNumber) {
+        returnValue += nowNumber;
+    }
+
+    private static void minus(double nowNumber) {
+        returnValue -= nowNumber;
+    }
+
+    private static void multiply(double nowNumber) {
+        returnValue *= nowNumber;
+    }
+
+    private static void divide(double nowNumber) {
+        returnValue /= nowNumber;
+    }


현재 클래스명이 Calculator이긴하는데 실제 숫자 계산 말고도 많은 역활을 담당하고 있는것 같습니다.
계산만 할 수 있도록 클래스 분리를 해보는 건 어떨까요?

dave915 · 2020-02-07T12:34:00Z

src/main/java/ExceptionHandler.java

+    }
+
+    private static boolean checkSign(String inputString) {
+        if (inputString.equals("+")) {


연산가능한 연산자들을 한 곳에 모아두고 inputString이 그곳에 포함되어 있는지 확인해보는 건 어떨까요?

dave915 · 2020-02-07T12:41:56Z

src/main/java/Calculator.java

+    }
+
+    private static void check(int i, String value) {
+        if (i % 2 == EVEN) {


홀수 짝수를 구분 할 수 있는 값을 상수로 뺀 점 좋았습니다.
하지만 더 나아가서 어떻게 하면 i % 2 == EVEN 이 짝수를 판별하기 위한 조건이란걸 한눈에 알게 할 수 있을까요?

dave915 · 2020-02-07T12:44:23Z

src/main/java/Calculator.java

+        for (int i = INDEX_INIT; i < values.length; i++) {
+            check(i, values[i]);
+        }
+        if (returnValue == (int) returnValue) {


강제 형 변환 보단 Math 클래스를 이용해 보는건 어떨까요?

dave915 · 2020-02-07T12:51:08Z

src/test/java/study/ExceptionHandlerTest.java

+    public static String inputHandler() {
+        try {
+            return checkInputHandler(scanner.nextLine());
+        } catch (InputMismatchException | IllegalArgumentException e) {
+            System.out.println("입력값을 확인해주세요.");
+            scanner = new Scanner(System.in);
+            return inputHandler();
+        }
+    }


테스트 클래스 안에 서비스 코드를 복사해서 테스트 하면 어떤 단점이 있을지 생각해보시면 좋을 것 같아요!
도움이 필요하시면 DM주세요 😀

처음에 복사하고 코딩하느라 테스트 클래스 안에 서비스 코드가 있는 걸 신경쓰지 못 했던 것 같습니다 :) 단점이라고 생각한다면 가독성이나 완전히 중복된 코드, 패키지의 분리 의미 자체가 없어지는 것? 정도 생각이 듭니다.

dave915

빙봉! 전체적으로 많이 좋아지셨네요!
고생했어요 👍
몇가지 피드백과 질문주신 부분에 대해 답변 남겼으니 확인해보세요 😀

dave915 · 2020-02-09T23:27:52Z

src/main/java/calculator/util/ExceptionHandler.java

+        if (inputString.equals("+")) {
+            return true;
+        }
+        if (inputString.equals("-")) {
+            return true;
+        }
+        if (inputString.equals("*")) {
+            return true;
+        }
+        if (inputString.equals("/")) {
+            return true;
+        }
+        return false;


List에 Calculator가 지원가능한 연산자들을 넣어두고 inputString이 해당 리스트에 속해있는지 체크해보면 어떨까요?

dave915 · 2020-02-09T23:28:51Z

src/main/java/calculator/domain/Operator.java

+    PLUS("+", nowNumber -> Calculator.returnValue += nowNumber),
+    MINUS("-", nowNumber -> Calculator.returnValue -= nowNumber),
+    MULTIPLY("*", nowNumber -> Calculator.returnValue *= nowNumber),
+    DIVIDE("/", nowNumber -> Calculator.returnValue /= nowNumber);


현재 Operator가 Calculator를 참조하고있는데 calcualte(double nowNumber)가 returnValue까지 받으면 Operator는 Calculator를 신경 안써도 되지 않을까요?

dave915 · 2020-02-09T23:29:07Z

src/main/java/calculator/util/ExceptionHandler.java

+
+
+


불필요한 공백은 지워주는게 좋아요.
코드 전체적으로 불필요한 공백을 지워보세요.

dave915 · 2020-02-09T23:29:26Z

src/main/java/calculator/util/ExceptionHandler.java

+    }
+
+    public static boolean checkIndividual(int i, String inputString) {
+        if (i % 2 == EVEN) {


i % 2 == EVEN 보다

if (isEven(i)) { return checkSign(inputString); } public boolean isEven(int number) { return i % 2 == 0; }

이런식으로 하는게 더 명시적이지 않을까요?
그리고 여러군데에서 공통으로 쓸 수 있을 것 같아보이네요!

dave915

피드백 반영하신 부분 확인했습니다!
처음보다 많이 나아진 코드 보기 좋습니다. 👍
이만 머지할게요! 수고하셨습니다.

dave915 · 2020-02-10T14:12:18Z

src/main/java/calculator/domain/Calculator.java

+    public static boolean isOdd(int number) {
+        return number % 2 == ODD_NUMBER;
+    }
+
+    public static boolean isEven(int number) {
+        return number % 2 == EVEN_NUMBER;
+    }


dave915 · 2020-02-10T14:12:35Z

src/main/java/calculator/domain/Operator.java

+    PLUS("+", (nowNumber, returnValue) -> returnValue + nowNumber),
+    MINUS("-", (nowNumber, returnValue) -> returnValue - nowNumber),
+    MULTIPLY("*", (nowNumber, returnValue) -> returnValue * nowNumber),
+    DIVIDE("/", (nowNumber, returnValue) -> returnValue / nowNumber);


dave915 · 2020-02-10T14:45:43Z

src/main/java/calculator/util/ExceptionHandler.java

+    private static List<String> operatorList = Arrays.asList("+", "-", "*", "/");
+
+    public static String checkInputHandler(String input) {
+        if (checkString(input.split(DELIMITER)) == true && checkUndefinedValue(input) == true) {


저는 보통 if 안에 들어가는 조건식 메소드 명을 명시적으로 짓고, 따로 ture/false체크를 안하는 편입니다.
이중 체크라는 생각이 들어서요...

if (checkString(input.split(DELIMITER)) && checkUndefinedValue(input))

우테코 과정을 진행하면서 다른 분들과 의견을 나누어 보세요!

dave915 · 2020-02-10T14:46:40Z

src/main/java/calculator/util/ExceptionHandler.java

+    public static boolean checkSign(String inputString) {
+        if (operatorList.contains(inputString)) {
+            return true;
+        }
+        return false;


아래처럼 줄여도 되지 않을까요?

public static boolean checkSign(String inputString) { return operatorList.contains(inputString); }

Hamliet and others added 20 commits February 5, 2020 13:25

[add] String 클래스에 대한 학습 테스트 - 요구사항1

0c4e779

[add] String 클래스에 대한 학습 테스트 - 요구사항2

c924385

[add] String 클래스에 대한 학습 테스트 - 요구사항3

c82d818

[add] Set Collection에 대한 학습 테스트 - 요구사항1

6806957

[add] Set Collection에 대한 학습 테스트 - 요구사항2

8a1e511

[add] 문자열 계산기 - README.md 파일 작성

9ff5ba2

[add] 문자열 계산기 - 사용자로부터 입력받기

1b360a2

[add] 문자열 계산기 - 홀수 인덱스와 짝수 인덱스로 나누는 함수

addb5c7

[add] 문자열 계산기 - 부호에 따라 계산하는 함수 구현

47cf992

[add] 문자열 계산기 - 사칙연산에 해당하는 각 함수 구현

3de7961

[add] 문자열 계산기 - 입력값을 확인하는 핸들러 구현

09addca

[mod] 문자열 계산기 - 정수형으로 계산하던 것을 실수형으로 확장

889a3c6

[add] 문자열 계산기 - 홀수 인덱스와 짝수 인덱스로 나누는 함수 단위 테스트

d1ea865

[add] 문자열 계산기 - 부호에 따라 계산하는 함수 단위 테스트

0b53410

[add] 문자열 계산기 - 사칙연산에 해당하는 각 함수 단위 테스트

c295110

[add] 문자열 계산기 - 입력값을 확인하는 핸들러 단위 테스트

750daac

[add] 문자열 계산기 - 0으로 나눌 경우 처리

a517723

[mod] 문자열 계산기 - 입력값을 확인하는 핸들러 단위 테스트

8133b04

Merge pull request #1 from aegis1920/onboarding

c2994fc

Onboarding

mod : @DisplayName 추가

1576729

dave915 suggested changes Feb 7, 2020

View reviewed changes

aegis1920 added 7 commits February 8, 2020 14:40

mod : 패키지, 클래스 분리 및 view 클래스 생성

ba76416

mod : util 패키지 추가

b0881d4

mod : 강제 형변환 -> Math 클래스 이용

11373d9

del : 테스트 클래스 안에 있는 서비스 코드

917bd64

mod : 입출력 분리

21c17f3

add : Operator를 Enum으로 분리, refactor : 함수명과 Calculator 클래스 리펙토링

187f90b

add : 테스트코드 재작성 및 static으로 인한 get set 메서드 추가

41209de

dave915 suggested changes Feb 9, 2020

View reviewed changes

add : isEven, isOdd 메서드 추가

141f92b

aegis1920 added 2 commits February 10, 2020 15:23

refactor : checkSign 메서드

838427e

refactor : Operator 인자 수정

7df937b

dave915 approved these changes Feb 10, 2020

View reviewed changes

dave915 merged commit b1cefbb into woowacourse:aegis1920 Feb 10, 2020

[빙봉] 온보딩 코드리뷰 요청합니다. #17

[빙봉] 온보딩 코드리뷰 요청합니다. #17

Uh oh!

Conversation

aegis1920 commented Feb 7, 2020

Uh oh!

dave915 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!

Choose a reason for hiding this comment

Uh oh!

dave915 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!

dave915 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!

3 participants