[라빈] 온보딩 - 학습 테스트, 단위 테스트 #3

giantim · 2020-02-07T06:31:43Z

안녕하세요
우아한 테크코스 2기 라빈입니다.
온보딩 미션 리뷰 부탁드립니다.
감사합니다.

kingbbode

안녕하세요 :) 잘 구현해주셨네요.
몇 가지 피드백 남겨두었습니다.
확인 부탁드려요!

kingbbode · 2020-02-08T02:31:53Z

src/main/java/calculator/Calculator.java

+
+    private double calculateByOperator(double left, double right, String operator) {
+        double result = 0d;
+        if (operator.equals("+")) {


존재할 가능성이 큰 것으로부터 사용되는 것이 안전합니다. :)

"+".eqauls(operator)

kingbbode · 2020-02-08T02:37:48Z

src/main/java/calculator/Input.java

+        System.out.print("계산식을 입력하시오: ");
+        String inputLine = scanner.nextLine();
+        try {
+            validator.isValidInput(inputLine);


is 접두어를 갖는 메서드는 boolean 타입을 반환하는 것이 컨벤션입니다 :)

kingbbode · 2020-02-08T02:39:42Z

src/main/java/calculator/Input.java

+            validator.isValidInput(inputLine);
+        } catch (IllegalArgumentException e) {
+            System.out.println(e.getMessage());
+            return inputString();


계속해서 잘못된 문자열을 입력한다면 콜스택이 쌓이게 되겠네요.
out of memory 가 발생할 수 있는 코드는 피하는 것이 좋습니다.
콜스택이 쌓이지 않도록 풀어보는 것은 어떨까요?

저는 사용자가 정상 문자열을 입력할 때 까지 입력을 계속해서 받도록 하기 위해 try catch 문을 이용하였습니다. 근데 이 방법에서 콜스택이 쌓이는 부분을 고려하지 못했네요. 수정한 방법으로는 사용자가 비정상적인 문자열을 입력하면 계산기가 종료되도록 구현했습니다.

kingbbode · 2020-02-08T02:45:29Z

src/main/java/calculator/Input.java

+
+import java.util.Scanner;
+
+public class Input {


Input 이란 클래스명과 inputString, splitString 이란 메서드명으로는, 이 클래스와 메서드가 이 무엇을 입력받고, 무엇을 처리하는지 알기 어렵네요 :)

추상적이거나 광범위한 네이밍보다는 역할과 의도를 드러낼 수 있는 네이밍을 해보는 것이 어떨까요?

클래스와 메소드의 이름을 짓는 것이 정말 어려운것 같습니다. 왜냐하면 의도를 나타내도록 이름을 짓다보니까 이름이 길어져서 가독성이 떨어지는것 처럼 느껴집니다. 제가 변경한 클래스와 메소드 이름을 보시고 다시 한번 피드백 해주실 수 있나요? :)

과거에는 축약어를 사용하는 등으로 코드 양을 줄이려고 했다면, 요즘은 네이밍을 있는 그대로 풀어서 작성하여 가독성을 높이려는 노력을 현업에서 더 많이 하고 있습니다
이름이 길어진다고 가독성이 떨어지진 않습니다. :)

kingbbode · 2020-02-08T02:49:20Z

src/main/java/calculator/Validator.java

+
+import java.util.ArrayList;
+
+public class Validator {


추상적이거나 광범위한 네이밍보다는 역할과 의도를 드러낼 수 있는 네이밍을 해보는 것이 어떨까요?

kingbbode · 2020-02-08T03:00:58Z

src/main/java/calculator/Validator.java

+        }
+    }
+
+    private void checkIsDoubleNumber(String s) {


check, is 가 함께 있는 네이밍이 어색하네요 :)

kingbbode · 2020-02-08T03:04:13Z

src/main/java/calculator/Validator.java

+        for (int i = 1; i < splitedInput.length; i = i + 2) {
+            String operator = splitedInput[i];
+            String number = splitedInput[i + 1];
+            if (operator.equals("/") && number.equals("0")) {


이곳에 있는 "/" 과 operators 에 있는 "/" 는 같은 의미일 것 입니다.
만약 작성하는 프로그램에서 나누기 연산의 기호를 변경하려고 한다면 수정해야할 곳이 두 곳이 될 것 입니다. 한 곳이라도 놓치면 의도한데로 동작하지 않을 것 입니다.

kingbbode · 2020-02-08T03:12:28Z

src/main/java/calculator/Calculator.java

+    }
+
+    private double calculateByOperator(double left, double right, String operator) {
+        double result = 0d;


각 단계에서 명시적 반환을 한다면 불필요한 선언은 필요없지 않을까요?

kingbbode · 2020-02-08T03:13:56Z

src/main/java/calculator/Calculator.java

+        if (operator.equals("-")) {
+            result = left - right;
+        }
+        if (operator.equals("*")) {
+            result = left * right;
+        }
+        if (operator.equals("/")) {
+            result = left / right;
+        }


if 나열과 else if 를 사용 함의 차이점이 무엇일까요?

이 코드에서 if가 나열되는 것이 의도와 맞는 것인가요?

kingbbode · 2020-02-08T05:17:45Z

src/test/java/calculator/CalculatorTest.java

+
+import java.util.stream.Stream;
+
+public class CalculatorTest {


MethodSource 이 외에도 여러가지 ParameterizedTest 를 위한 Source 들이 제공됩니다 :)

MethodSource 로 테스트 코드를 작성했었던 이유는 이전에 테스트를 할 때 Double.toString() 을 이용해서 테스트를 해보려고 했었기 때문이었습니다. 목적에 맞는 Source 를 사용하도록 코드를 변경했습니다. 잘 몰랐던 부분인데 감사합니다 :)

… 이름 변경

…umber -> checkCorrectDoubleNumber, checkIsOperator -> checkCorrectOperator 로 메소드 이름 변경

… / Validator 에서 is 로 시작하는 메소드 명 변경 / 공백으로만 이루어진 문자열 입력 시 예외 출력 / Input 클래스의 기능을 명확히 설명할 수 있도록 클래스 이름과 메소드 이름을 변경

… into onboarding

kingbbode

안녕하세요.
아쉬운 점이 있어 코멘트를 추가해두었습니다 :)
확인 부탁드려요!

kingbbode · 2020-02-09T11:31:59Z

src/main/java/calculator/Operator.java

+package calculator;
+
+public enum Operator {
+    Plus("+"), Minus("-"), Multiplication("*"), Division("/");


enum 변수는 대문자로 작성하는 것이 컨벤션입니다 :)

kingbbode · 2020-02-09T11:33:56Z

src/main/java/calculator/ValidityInspector.java

+        operators = new ArrayList<>();
+        operators.add(Operator.Plus.getValue());
+        operators.add(Operator.Minus.getValue());
+        operators.add(Operator.Multiplication.getValue());
+        operators.add(Operator.Division.getValue());


저장해두지 말고 Operator 에게 물어보는 것은 어떨까요?

Operator.contains(String operator)

kingbbode · 2020-02-09T11:35:42Z

src/main/java/calculator/Calculator.java

+        if (Operator.Plus.getValue().equals(operator)) {
+            return left + right;
+        } else if (Operator.Minus.getValue().equals(operator)) {
+            return left - right;
+        } else if (Operator.Multiplication.getValue().equals(operator)) {
+            return left * right;
+        } else if (Operator.Division.getValue().equals(operator)) {
+            return left / right;
+        }
+        return 0d;


Operator 에 대한 연산은 Operator 가 알고 있는 것이 좋지 않을까요?

Operator.find(operator).calculate(left, right)

객체에게 묻는 것보다 요청하는 코드를 작성하는 것이 객체지향적인 코드를 작성하는 방법 중 하나입니다.

https://martinfowler.com/bliki/TellDontAsk.html

객체지향에서 굉장히 중요한 원칙이니 참고하시면 좋을 듯 합니다 :)

올려주신 링크 참고해서 묻는 것이 아닌 요청하는 코드에 대해서 예전보다 더 정확하게 이해하게 되었습니다! :) 다시 리뷰 부탁드립니다. 감사합니다.

…ate() 메소드를 만들어 입력한 연산자와 좌항, 우항을 계산하게 수정

kingbbode

안녕하세요.
요구사항 잘 만족하여 이만 머지하겠습니다 :) 👍
수고하셨습니다!

kingbbode · 2020-02-10T09:44:53Z

src/main/java/calculator/Operator.java

        return this.symbol;
    }
+
+    public static Operator find(String operator) {


잘못된 입력에 대한 예외는 처리해줬으면 더 좋았겠네요 :)

giantim and others added 9 commits February 5, 2020 15:06

add : 요구사항 1 - split 했을 때 contains()를 활용해 변환 값이 맞는지 검증하는 테스트 코드 작성

ed7ba12

add : 요구사항 2 - substring()을 활용해 올바른 문자열이 반환되는지 테스트

b7f56bf

add : 요구사항 3 - charAt() 메소드를 활용해 범위를 넘어가는 예외 테스트

4646843

add : 요구사항 1 - size() 메소드를 활용해 Set의 크기를 확인하는 테스트

3994dee

add : 요구사항 2 - JUnit의 ParameterizedTest를 이용해 중복 코드를 제거

db637c0

add : 요구사항 3 - @CsvSource를 활용해 입력 값에 따라 결과 값이 다른 경우의 테스트

1da46a8

add : 문자열 계산기 기능 구현 및 테스트 코드 작성

43d67f6

add : README 수정

75191ca

Update README.md

f5b3b85

kingbbode suggested changes Feb 8, 2020

View reviewed changes

giantim added 8 commits February 8, 2020 19:57

refact : operator를 enum 클래스로 만들어서 관리 / Validator -> ValidityInspector…

19caade

… 이름 변경

refact : if 문의 나열 -> if ~ else if 문으로 변경 / 조건문의 단계별로 return

006eded

refact : 입력 문자열의 공백 또는 null 검사 메소드를 하나로 통일 / 매직넘버 제거 / checkIsDoubleN…

470960a

…umber -> checkCorrectDoubleNumber, checkIsOperator -> checkCorrectOperator 로 메소드 이름 변경

modify : gitignore 수정

b3fbab4

refact : 여러 클래스에서 공통적으로 사용하는 0, 공백, 빈 문자열을 상수 클래스로 분리

061557d

refact : 변경한 메소드에 맞게 테스트 코드 수정 / MethodSource -> ValueSource 로 변경

9d2cac8

Merge branch 'onboarding' of https://github.com/giantim/java-calculator…

d5d53d5

… into onboarding

kingbbode suggested changes Feb 9, 2020

View reviewed changes

refact : Operator 를 컨벤션에 맞게 이름 수정 / Operator.find() 와 Operator.calcul…

4f46d30

…ate() 메소드를 만들어 입력한 연산자와 좌항, 우항을 계산하게 수정

kingbbode approved these changes Feb 10, 2020

View reviewed changes

kingbbode merged commit 6fe685c into woowacourse:giantim Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[라빈] 온보딩 - 학습 테스트, 단위 테스트 #3

[라빈] 온보딩 - 학습 테스트, 단위 테스트 #3

giantim commented Feb 7, 2020

kingbbode left a comment

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

giantim Feb 8, 2020

kingbbode Feb 8, 2020

giantim Feb 8, 2020

kingbbode Feb 9, 2020

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

kingbbode Feb 8, 2020

giantim Feb 8, 2020

kingbbode left a comment

kingbbode Feb 9, 2020

kingbbode Feb 9, 2020

kingbbode Feb 9, 2020

giantim Feb 9, 2020

kingbbode left a comment

kingbbode Feb 10, 2020


		import java.util.stream.Stream;

		public class CalculatorTest {

[라빈] 온보딩 - 학습 테스트, 단위 테스트 #3

[라빈] 온보딩 - 학습 테스트, 단위 테스트 #3

Conversation

giantim commented Feb 7, 2020

kingbbode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingbbode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingbbode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment