Step1(문자열계산기,자동차경주) 리뷰요청합니다. #2179

Miot2J · 2021-04-29T07:47:12Z

테스트 케이스 구현중 질문 사항을
RacingGameTest 에 주석으로 남겨 두었습니다.

리뷰 잘 부탁드립니다.

banjjoknim

안녕하세요? 이번 자동차 경주 게임을 리뷰를 맡게 된 '배재홍' 이에요! 😁
첫 미션 진행이 쉽지 않았을거란 생각이 드는데, 잘 해주셨네요!
코멘트가 많다고 느껴지실 수 있으나, 중복되는 부분들이 보이실거에요.
차근차근 같이 나아가면 되는 부분이니 천천히 고민하고 반영해보아요.
그리고, 질문 주신 부분들에 대해서 각각 코멘트를 달아두었으니 확인해보시면 좋을 것 같아요.
궁금한 점이 있으시면 DM을 보내주세요. 같이 고민해보면 좋을 것 같네요 ^^!
화이팅입니다! 🤞👍👏

README.md

banjjoknim · 2021-04-29T17:12:07Z

src/main/java/calculator/Calculate.java

@@ -0,0 +1,19 @@
+package calculator;
+
+public class Calculate {


클래스의 이름은 명사로 하는 것이 좋아요. 가급적이면 클래스, 메서드, 변수 등 모든 코드의 이름에서 의미가 드러나도록 하는 것이 좋아요. 결국, 코드 또한 글을 읽는 것과 마찬가지로 가독성있게 작성할 수록 독자가(미래의 내가 될 수도 있겠죠?) 이해하기 쉬워질거에요.
조금은 접근하기 쉬운 참고자료를 찾아봤어요. 가볍게 읽어보시면 좋을 것 같네요! 그리고 Google Java Style Guide의 5.2 Rules by identifier type 항목을 보시면 도움이 될 것 같아요!

banjjoknim · 2021-04-29T17:14:32Z

src/main/java/calculator/Calculate.java

+package calculator;
+
+public class Calculate {
+    public double addition(double firstNumber, double secondNumber) {


메서드의 이름은 동사로 시작하는 것이 좋아요. 메서드는 행위를 지정하고 있기 때문이에요.
마찬가지로 앞선 Calculate 클래스 명에 대한 코멘트에서 드린 참고자료를 보면서, 클래스와 메서드의 이름을 어떻게 지으면 좋을지 고민해보면 좋을 것 같아요!

banjjoknim · 2021-04-29T17:20:04Z

src/main/java/calculator/Calculator.java

+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.StringTokenizer;


사용되고 있지 않은 import 문이네요! 사용하지 않는 것들은 제거해주시는게 좋을 것 같아요!

banjjoknim · 2021-04-29T17:30:36Z

src/main/java/calculator/Calculator.java

+
+public class Calculator {
+
+    public void calculatorGenerate() {


현재 calculatorGenerate 메서드는 이름과는 다소 다른 기능들 또한 수행하고 있는 것 같아요! 입력하는 기능, 출력하는 기능이라던지.. 하나의 메서드에 밀집된 이러한 기능들을 각각 다른 메서드로 분리해서 이름에 걸맞는 행위를 하도록 하는게 어떨까요?

✔
분리하면 두개의 기능정도로 분리 할 수 있을거 같아요!
일단은 변수 명만 변경 해 보았습니다.

banjjoknim · 2021-04-30T08:48:08Z

src/test/java/RacingGameTest.java

+        // repeatCount를 10으로 하드 코딩 한 후 테스트를 실행했는데 10회를 돌았다는것을 검증 하려면
+        // repeatMoveCount 내부에 반복횟수를 저장하는 변수를 생성 하는 것 이외의 방법이 생각나지 않습니다.
+        // 따라서 대부분의 경우에 10회 반복 시 moveCount가 2이상일 확률이 높아 아래의 코드를 작성했는데
+        // 확률성 코드를 없애기 위해 새로운 변수를 생성 해야 하나요?


테스트 코드의 목적은 확실한 결과를 이끌어냄으로써 기능이 잘 동작하는 것을 보장하는 것도 포함되는데요, 여기서 확실한에 주목해야 해요. 말씀하시기를 moveCount가 2 이상일 확률이 높아 라고 하셨는데 이 말 자체가 불확실성을 포함하고 있다고 느껴져요. 확률성 코드를 없애기 위해서 어떻게 해야할까? 라는 질문은 앞서 말씀드린 moveCountChangeTest()의 코멘트와 함께 생각해보면 좋을 것 같아요.

그리고 테스트의 횟수를 따로 검증하지는 않아도 된다고 생각해요. 테스트의 목적이 확실한 결과의 도출이기 때문에, 혹여나 있을 예외 상황을 잡아내기 위해 테스트를 많이 반복한다고 생각이 되어요. 어느정도 신뢰할만한 표본의 숫자가 필요할 수는 있겠지만, 현재 미션에서 그러한 표본을 논할만큼 복잡한 기능을 다루고 있지는 않다는 생각이 들어요. 그렇기 때문에 테스트의 횟수 보다는 테스트를 진행하는 기능이 잘 동작하는지에 집중해서 테스트를 작성하면 좋을 것 같아요!

banjjoknim · 2021-04-30T08:49:33Z

src/main/java/racing/RacingGame.java

+        output.win(findWinner(carList));
+    }
+
+    private void repeatMoveCount(int count, List<Car> carList) {


직역하면 이동횟수를 반복한다,, 가 되는 것 같네요. 수행하는 기능과는 다소 어색한 이름이라고 느껴져요. 적절한 이름을 고민해보면 좋을 것 같아요!

✔
moveCountChange 를 여러번 수행하기에 repeatMoveCountChange 로 변경했습니다.

banjjoknim · 2021-04-30T08:51:28Z

src/main/java/racing/RacingGame.java

+        return carList;
+    }
+
+    private int makeRandomNumber() {


현재, 랜덤한 번호를 생성하기 위해서 new RandomNumber() 를 통해 인스턴스를 생성하고, 그 뒤에 랜덤한 번호를 생성하는 메서드를 호출하고 있어요. RandomNumber 클래스에서 이 메서드를 바로 호출할 수 있도록 해보는건 어떨까요?

✔
음 어짜피 게임이 진행될 때, makeRandomNumber 가 최소 1번 이상 실행될테니 static method 로 생성하는게 좋다는 뜻이 맞을까요?

실행과는 무관하게, RandomNumber 클래스는 상태(필드)를 갖고 있지 않아요. 그렇기 때문에 인스턴스를 생성하는 의미가 단순히 메서드를 호출하기 위한 것 외에는 존재하지 않게 돼요. 그렇다면, 굳이 인스턴스를 생성해서 메모리에 할당할 필요가 있을까요?

추가적으로, 인스턴스 메서드가 아닌 클래스 메서드로 구현하게 되면, 현재 RacingGame의makeRandomNumber()라는 메서드도 결국 굳이 선언하지 않아도 되지 않을까요?

banjjoknim · 2021-04-30T08:54:34Z

src/main/java/racing/RacingGame.java

+        return randomNumber.makeRandomNumber();
+    }
+
+    private void moveCountChange(List<Car> carList) {


각각의 자동차가 움직이는 기능을 구현하고 있어요. 그에 맞게, 조금 더 적절한 이름을 고민해보는건 어떨까요? 이름만 봐서는 단순히 자동차가 갖고 있는 moveCount를 변경하는 기능인 것 처럼 보여요.

✔
각각의 자동차가 가진 moveCount 를 변경하는 기능이 맞습니다.
움직임의 구현은 PrintNowDistance() 에서 하고 있어요

Output#printNowDistance()의 구현은 단순히 자동차의 위치에 따른 출력을 수행하고 있어요.
조금 더 파고 들어가보면, 여기서는 각각의 Car 객체가 move()메서드를 호출하여 스스로의 상태(moveCount)를 변화시키고 있어요.
그렇다면, moveCount를 변경하는 기능은 Car 객체가 갖고 있어야할 책임이 아닐까요? 자동차가 스스로 움직이는 걸까요, RacingGame이 자동차를 움직이는걸까요?
만약 그게 아니라 각각의 자동차가 움직이는 기능을 정의한거라면 이름이 과연 적절한지 고민해보면 좋을 것 같아요. 😀

banjjoknim · 2021-04-30T08:59:00Z

src/main/java/racing/RacingGame.java

+        return winnerList;
+    }
+
+    private int findWinnerCount(Car car, int winnerCount, List<String> winnerList) {


로직을 살펴보면 승자의 이동횟수를 찾고, 승자 리스트를 만드는 기능 두 가지를 수행하고 있는 것으로 보여요. 각각의 기능을 수행하는 각각의 다른 메서드로 분리해보는걸 고민해보면 좋을 것 같아요!

❓
이렇게 구현한 이유가 depth , 들여쓰기 가 1줄로 제한이 되어버려서
승자 이동 횟수와 승자리스트를 리턴으로 주는 각각의 메소드가 있다고 해도
결국 리스트를 순회하며 이동 횟수를 비교해야해서 당시에 저렇게 만들었습니다.

다시보니, findWinnerCount 라는 이름은 승자의 이동횟수가 아닌 승자의 수(몇 명인지)를 찾는다고 해석될 수도 있을 것 같네요. 😥

또한, 현재 findWinnerCount()가 수행하고 있는 기능은 여전히 승자의 이동횟수를 찾고, 승자 리스트를 만들고 있어요. 이 기능들을 각각의 메서드로 분리해볼 수 있을 것 같아요. 참고로 현재 미션의 요구사항에서 depth는 2까지 허용된답니다. 그러니 조금 더 자유롭게 구현하려고 시도해보면 좋을 것 같아요. 😀
아래처럼 for문 안에 if문이 있으면 depth는 2입니다.

private int findWinnerCount(Car car, int winnerCount, List<String> winnerList) { for() { if() { } } }

추후 미션의 진행을 원활히 하기 위해서, 메서드 분리를 반드시 고민해서 해주셔야 해요!

…ssionAndPrintResult로변경

Miot2J · 2021-05-02T11:30:27Z

리뷰중 일부 수정 완료했습니다

CarTest 에 @ParameterizedTest 구현완료 & Input,Output 에 static method 로 변경, 명확하지 않은 이름의 변수 클래스 새로 명명,특정 변수값 상수화 완료

banjjoknim

안녕하세요 재언님! 이번 자동차 경주 리뷰를 맡고 있는 '배재홍' 이에요 😁
적극적으로 미션을 진행하시는 모습이 보기 좋아요.
지난 리뷰에 비해 코멘트가 적어졌는데, 중복되는 부분에 대한 언급은 최대한 줄여봤어요.
따라서, 지금부터는 리뷰 요청을 하기 전에 이전 리뷰에서 드렸던 코멘트들을 반드시 반영한 뒤에 리뷰요청을 주셔야해요! 😨
코멘트가 달린 부분 뿐만 아니라, 비슷하게 적용되는 다른 부분들에 대해서도 의식적으로 고민하고, 변경해보도록 해요. 이후의 미션들에서도 이러한 부분들에 대해 코멘트를 반복해서 드리기가 힘들기 때문이랍니다.
그렇기 때문에 재언님 스스로가 의식적으로 신경쓰셔야 할 부분들이 될 거에요.
지속적으로, 의식적으로 연습을 반복해야 비로소 체득화되어 재언님의 것이 될 거라고 생각해요 😉

그리고 이제부턴 점점 생각할 것들이 많아질 거에요.
할 수 있는 만큼 고민하고 반영하려고 해봐요.
궁금하신 점이 있다면 편하게 DM 주세요!
화이팅이에요!! ^^!

banjjoknim · 2021-05-02T16:13:14Z

src/main/java/calculator/Calculator.java

+
+    public void calculatorGenerate() {
+        Calculator calculator = new Calculator();
+        Output output = new Output();


Output에 코멘트 달아두도록 할게요 :)

banjjoknim · 2021-05-02T16:28:22Z

src/main/java/calculator/Calculator.java

+        String mathExpression = input.mathExpressionInput();
+
+        if(mathExpression.trim().isEmpty()){
+            exception.inputNullException();


이부분에 대해서 한번 더 말씀을 드리자면, 예외 상황시에 바로 예외를 발생시키자! 라는 의도로 말씀드린거였어요! IllegalArgumentException()의 인자로 들어가는 메시지를 잘 정의한다면 굳이 Exception 클래스를 재정의한 뒤, 메서드를 구현하지 않아도 되지 않을까요? 이미 충분히 메시지에서 의도가 드러나고 있을테니까요 :)
또한, 바로 예외를 발생시키는 것과 Exception 클래스에 구현된 메서드들과의 기능 상의 차이가 있을까요?
적용가능한 다른 로직에도 모두 적용시켜보면 좋을 것 같아요 :)

추가적으로, 현재 로직상으로는 아래의 throw new IllegalArgumentException()이 실행되는 경우가 없을 것 같네요! 😂

banjjoknim · 2021-05-02T16:32:36Z

src/main/java/calculator/Calculator.java

+        if(mathExpression.trim().isEmpty()){
+            exception.inputNullException();
+        }
+        if (Pattern.matches("[^0-9+\\-*/]",mathExpression)){


이 부분은 FourRule 클래스에 코멘트를 달아두도록 할게요 :)
정규표현식을 그대로 두셨네요! 그렇다면 정규표현식에 의미를 가질 수 있는 이름을 부여해보는 건 어떨까요? 단순히 정규표현식만 놓고 본다면 해석하는데에 비용이 들어갈 것 같네요!
추가적으로, 입력값을 검증하고 예외를 발생시키는 것 또한 각각 하나의 기능으로 생각하고 분리할 수 있지 않을까요?

banjjoknim · 2021-05-02T16:47:09Z

src/main/java/calculator/Calculator.java

+//        String[] splitMathExpressionByChar = mathExpression.split("");
+        List<String> mathExpressionList = new ArrayList<String>();
+        String temp = "";
+        for (String s : splitMathExpression) {


추가적으로, s와 같은 의미가 드러나지 않는 변수가 보여요. 이러한 변수들은 앞으로 전부 의미를 갖도록 변경해주셔야 해요. 여전히 곳곳에 보이는 것 같은데 꼼꼼하게 확인해서 변경하시면 좋을 것 같아요. 😉

banjjoknim · 2021-05-02T16:54:02Z

src/main/java/calculator/Calculator.java

+        return mathExpression;
+    }
+
+    private List<String> sliceMathExpression(String mathExpression) {


sliceMathExpression() 메서드는 크게보면 입력받은 문자열을 split으로 분리한 뒤, 조건문에 맞으면 List에 추가하고 있어요. List에 추가하는 행위는 변환에 가깝다고 볼 수 있을 것 같아요. 그러면 현재 메서드의 이름이 기능과 잘 어울리는걸까요? 그와 동시에 분리와 변환이라는 2가지의 기능을 하고있는 건 아닐까요? 각각의 기능으로 분리해보면 어떨까요? 😀

banjjoknim · 2021-05-02T19:59:06Z

src/test/java/RacingGameTest.java

+    }
+
+    @Test
+    public void repeatCountTest() throws Exception {


현재 이 테스트는 실패하고 있네요. 😥
모든 테스트가 잘 동작하는지, 기능상의 이상은 없는지 반드시 확인한 뒤에 리뷰 요청을 해주세요!

banjjoknim · 2021-05-02T20:05:50Z

src/main/java/calculator/Calculator.java

+                integerOfMathExpression = "";
+                continue;
+            }
+                integerOfMathExpression = integerOfMathExpression.concat(s);


들여쓰기가 이상하게 되어있네요! 지난 리뷰에서는 이런 부분들을 전부 언급해드렸지만, 이후의 리뷰에서는 점점 언급이 줄어들거에요. 따라서 들여쓰기, 띄어쓰기와 같은 부분들에 대해서 언급하지 않아도 신경쓰셔야 해요!
인텔리제이에서 제공해주는 기능 중에는 자동 정렬 기능이 있어요. 단축키는 다음과 같아요.

코드 자동 정렬하기 (Reformat Code)

MacOS: Cmd + Opt + L

Win/Linux: Ctrl + Alt + L

참고자료를 보면 도움이 될 것 같아요! 😊

banjjoknim · 2021-05-02T20:13:50Z

src/main/java/racing/Car.java

+        if(name.length() > 5){
+            throw new IllegalArgumentException("이름은 5자 이내로 입력하세요.");
+        }


이름에 대한 유효성 검증 부분을 따로 분리해보면 어떨까요?

banjjoknim · 2021-05-02T20:21:37Z

src/main/java/calculator/Calculator.java

+        output.resultOutput(calculator.makeResult(strings));
+    }
+
+    private String returnEnteredMathExpression() {


현재 returnEnteredMathExpression() 메서드의 핵심 기능은 입력된 문자열이 올바른지 검증인 것 같아요. 그렇다면 핵심 기능에 맞게끔 적절한 이름을 붙여주면 좋지 않을까요?
또한, 예외를 발생시키는 것과 문자열을 반환하는 2가지의 기능을 수행하고 있네요! 각각을 분리해보면 좋을 것 같아요. :)

banjjoknim · 2021-05-02T20:31:24Z

src/test/java/RacingGameTest.java

+        // 자동차별 moveCount를 증가시켜주는 moveCountChange 함수 안에서 난수가 생성되는데 만약 모든 자동차 난수가 4미만이면
+        // randomCount가 증가되지 않아 모든 자동차 이동거리가 0이 되는 경우가 발생해
+        // 이럴 경우는 테스트 코드를 어떻게 작성해야 하는지 모르겠습니다.
+        for (int i = 0; i < carList.size(); i++) {


랜덤한 테스트에 대한 갈증이 있으실 것 같아서, 참고자료를 준비해봤어요.

calculator 패키지 내부의 의미없는 변수명들을 의미를 가지게 변경하였음,Exception 과 Input, Output 내부 메서드 들이 중복되게 호출되어 static 메서드로 변환함.

Exception.java -> ExceptionMessage.java 변경, CalculatorFunction.java 삭제, MathSymbol 생성 및 enumTest() 메소드 구현.

inputExpressionAndPrintResult()-> printResult(), makeSliceMathExpression() 으로 분리

Miot2J · 2021-05-03T17:02:55Z

-------------------------

------------------
------------------

------------------
------------------

------------------
-----------------

-----------------
-----------------
코멘트를 남겨주신 부분들을 적용하면서 의문점이 생겼던 부분에 제 의견을 남겼는데,
제대로 이해 하고 작성한 건지 궁금합니다!
찾기 힘드실까봐 캡쳐해서 올립니다😁
(나머지 부분은 큰 고민없이 수정했습니다)

banjjoknim

안녕하세요 재언님! 이번 리뷰를 담당하고 있는 '배재홍' 이에요.
점점 코드가 좋아지고 있는게 보이네요! 😍😍😍 🥳🥳🥳
일부만 변경하여 리뷰요청을 하신 만큼 조금 더 빠르게 진행하면 좋을 것 같아요 :)
변경해주신 부분들에 대하여 코멘트를 남겨드렸으니 확인해보시고
계속해서 진행해주시면 될 것 같아요.

궁금한 점이 있으시다면 편하게 DM 보내주세요! 😁😁😁

banjjoknim · 2021-05-04T05:24:01Z

src/main/java/calculator/ExceptionMessage.java

+    public static final String INPUT_NULL_MESSAGE = "값을 입력해 주세요.";
+    public static final String IS_NOT_MATH_EXPRESSION_MESSAGE = "올바른 수식을 입력해 주세요.";
+    public static final String THE_END_IS_NOT_NUMBER_MESSAGE = "수식의 끝은 숫자여야 합니다.";


점점 의미를 잘 드러내는 형태로 변하고 있군요!! 👍 👍 👍
다만, 첫 번재인 INPUT_NULL_MESSAGE는
처음보는 사람이 보기엔NULL을 입력해주세요..로 해석될 수 있을 것 같아요.
많이 좋아졌지만 조금만 더 적절한 이름을 고민해보면 좋을 것 같아요 :)

banjjoknim · 2021-05-04T05:25:28Z

src/main/java/calculator/MathSymbol.java

@@ -0,0 +1,18 @@
+package calculator;
+
+public enum MathSymbol {


Enum을 사용해주셨네요! 👍 👍 👍
지금 단계에서는 이정도로도 충분히 잘하셨다고 할 수 있을 것 같네요!
그러면 여기서, 살짝 고민을 더 해볼 수 있을 것 같아요.
입력된 수학기호(연산자)가 어떤 기호인지 판단하는 역할과 책임은 누구의 것일까요?

책임은 MathSymbol 이 가질까요??
아니면 계산기가 수학기호를 판단해야할까요?
만약 Enum이라면 Enum은상수들의 집합인데 책임이 있나요?🤔
어렵네요 ㅠㅠ

정말 단순하게, 아이스크림을 만드는 기계가 있다고 생각해봐요.
아이스크림을 만드는 기계는 재료가 어떤 녀석들인지(우유인지, 설탕인지 기타등등..) 반드시 알아야만 아이스크림을 만들 수 있을까요? 단순히 아이스크림을 만들기위한 재료를 넣기만 하면 만들어주지 않을까요? '객체는 주체성을 갖는다.' 라는 관점에서, 객체에게 메시지를 보낸다라고 생각해보면 좋을 것 같아요 :)

banjjoknim · 2021-05-04T05:33:14Z

src/main/java/calculator/Calculator.java

+        if (operator.equals(PLUS.getMathSymbol())) {
+            return firstNumber + secondNumber;
+        }
+        if (operator.equals(MINUS.getMathSymbol())) {
+            return firstNumber - secondNumber;
+        }
+        if (operator.equals(MULTIPLE.getMathSymbol())) {
+            return firstNumber * secondNumber;
+        }
+        if (operator.equals(DIVIDE.getMathSymbol())) {
+            return firstNumber / secondNumber;
+        }


입력된 연산기호가 어떤 연산기호인지 Calculator가 판단하고 있어요. 조금 더 세분화해서, 어떤 연산기호인지 판단하는 역할은 누구의 책임일까요? 고민해보면 좋을 것 같아요 :)
그리고, 각각의 계산 로직들을 이름을 붙여서 메서드로 만들면 좋지 않을까요?

banjjoknim · 2021-05-04T05:35:14Z

src/main/java/calculator/Calculator.java

+    private static final String LETTER = "[^0-9]";
+    private static final String THE_END_IS_NOT_NUMBER = "[0-9]*$";
+    private static final String IS_NOT_MATH_EXPRESSION = "[^0-9+\\-*/]";
+    private static final String MAKE_EMPTY = "";


MAKE_EMPTY 라는 이름이 다소 어색하게 느껴져요. 의미는 알겠으나, 할당에 사용하고 있으므로 좀 더 값에 가까운 의미를 갖도록 이름을 변경해보면 좋을 것 같다고 느껴지네요! 😀

banjjoknim · 2021-05-04T05:37:04Z

src/main/java/calculator/Calculator.java

+        throw new IllegalArgumentException(IS_NOT_MATH_EXPRESSION_MESSAGE);
+    }
+
+    public void printResult(){


Main에서 printResult()가 가장 먼저 호출되고 있네요! 호출 순서에 맞게 전체적인 메서드의 배치를 변경해보면 가독성이 좀 더 좋아질 것 같아요 :)

banjjoknim · 2021-05-04T06:05:03Z

src/main/java/calculator/Calculator.java

+    private static final String THE_END_IS_NOT_NUMBER = "[0-9]*$";
+    private static final String IS_NOT_MATH_EXPRESSION = "[^0-9+\\-*/]";
+    private static final String MAKE_EMPTY = "";
+    private static final int HAS_NUMBER = 1;


HAS_NUMBER 는 숫자인지 아닌지를 판별할 때 사용되는 것 같아요. 그렇다면 int 자료형이 아닌 boolean 타입을 사용해보는 건 어떨까요? 참고자료가 도움이 될 것 같네요 :)

이부분은 입력된 수식에 추가되지 않은 수식이 남아있다면
수식의 사이즈가 1이상임에 착안하여 만들어진 로직입니다.
변수이름을 좀 더 명확히 바꿔보았습니다.😢

banjjoknim · 2021-05-04T06:07:15Z

src/main/java/calculator/Calculator.java

+        return mathExpression.split("");
+    }
+
+    private List<String> convertArrayToList(String[] splitMathExpression) {


convertArrayToList라는 이름도 현재 상황에서는 나쁘진 않지만, 조금 더 직관적인 이름을 고민해보면 좋을 것 같아요 :)

banjjoknim · 2021-05-04T06:15:57Z

src/main/java/calculator/Calculator.java

+    private List<String> convertArrayToList(String[] splitMathExpression) {
+        List<String> mathExpressions = new ArrayList<>();
+        String integerOfMathExpression = MAKE_EMPTY;
+
+        for (String mathSymbol : splitMathExpression) {
+            if (Pattern.matches(LETTER, mathSymbol)) {
+                mathExpressions.add(integerOfMathExpression);
+                mathExpressions.add(mathSymbol);
+                integerOfMathExpression = MAKE_EMPTY;
+                continue;
+            }
+            integerOfMathExpression = integerOfMathExpression.concat(mathSymbol);
+        }
+        if (integerOfMathExpression.length() >= HAS_NUMBER) {
+            mathExpressions.add(integerOfMathExpression);
+        }
+        return mathExpressions;
+    }


중간에 반복문에서, mathSymbol에 splitMathExpression의 요소를 할당하고 있네요! 숫자도 mathSymbol 인지 고민해보면서 이름을 변경해보면 좋을 것 같아요. :)
그리고, 단순히 배열에서 리스트로 변환되는거라면, 굳이 연산기호인지, 숫자인지 검증을 할 필요가 있을까요? 만약 검증을 해야한다면 그건 검증 기능을 수행하는 메서드의 책임이 아닐까요? 😀 현재 로직상에서는 단순히 변환만을 수행해야 할 것 같네요!
참고자료가 도움이 될 것 같아요!

banjjoknim · 2021-05-04T06:29:03Z

src/test/java/calculator/CalculatorTest.java

+    }
+
+    @Test
+    void enumTest() {


MathSymbol의 테스트를 CalculatorTest에서 수행하고 있네요! 적절한 위치로 옮겨보는게 어떨까요? :)

banjjoknim · 2021-05-04T06:31:33Z

src/test/java/calculator/CalculatorTest.java

+    void additionTest() {
+        int first = 3;
+        int second = 4;
+        Assertions.assertThat(calculator.calculate(first,"+",second)).isEqualTo(7);
+    }
+
+    @Test
+    void subtractionTest() {
+        int first = 3;
+        int second = 4;
+        Assertions.assertThat(calculator.calculate(first,"-",second)).isEqualTo(-1);
+    }
+
+    @Test
+    void multiplicationTest() {
+        int first = 3;
+        int second = 4;
+        Assertions.assertThat(calculator.calculate(first,"*",second)).isEqualTo(12);
+    }
+
+    @Test
+    void divisionTest() {
+        int first = 12;
+        int second = 4;
+        Assertions.assertThat(calculator.calculate(first,"/",second)).isEqualTo(3);
+    }


현재 이 테스트들은 Calculator#calculate()를 테스트하고 있는데요~ 상황별로 나눠서 테스트를 잘 해주셨네요! 👍 👍 👍 그러면 Calculator#calculate() 메서드의 내부 로직도 이렇게 상황별로 메서드 분리를 해보면 어떨까요? :)

if(operator.equals(PLUS.getMathSymbol())) { return firstNumber + secondNumber; } --->
if(operator.equals(PLUS.getMathSymbol())) { return plus(firstNumber , secondNumber); }
메서드를 분리했더니 위와 같이 되었고, 가독성이 더 좋아졌는지 모르겠습니다...🤔
오히려 메서드 구현 부분으로 한번 더 찾아가야 되는데 이렇게 간단한 부분도 분리하는게 맞을까요??

기존 makeCar() 메서드에서 carList를 반환 했는데 makeCarList() 와 대부분 중복되었으며 Car 객체 생성 책임을 #RacingGame#makeCar()에서 가질 필요가 없었다.

validateLength() 메서드 추가

Miot2J · 2021-05-05T09:30:00Z

메서드를 분리하다보니 책임에 따른 분리를 떠나 가독성? 코드해석비용? 에 대한 궁금증이 생겼습니다.🤔😂

Calculator 의 연산자를 찾아서 계산 메서드를 호출하는 부분을 MathSymbol 의 findValidatedSymbol & operate 로 변경

banjjoknim

안녕하세요 재언님! 리뷰를 맡고 있는 '배재홍' 이라고 해요.
나날이 발전하는 모습을 보니 뿌듯하네요 :)
코멘트 보시고 반영하면서 궁금하신 점 있으시면 DM 날려주시면 될 것 같아요! 😁
1단계는 여기서 Merge 하도록 할게요.
2단계에서 찾아뵙겠습니다 ^^!

banjjoknim · 2021-05-06T07:00:07Z

src/main/java/calculator/Calculator.java

+    }
+
+    public double calculate(double firstNumber, String operator, double secondNumber) {
+        return findValidatedSymbol(operator).operate(firstNumber, secondNumber);


정적(static) 메서드는 클래스를 인스턴스화 하지 않고 바로 호출할 수 있어요. 참고자료를 보면 도움이 될 것 같네요!

static의 사용에대해 고민이 많았는데
이 부분은 static을 사용할 필요가 없다고 생각했습니다.
calculate 객체 없이 계산 기능을 사용할 수 있다는 것은 장점 이겠지만
현재 단순히 calculte() 만 사용할 일이 없다고 생각합니다.
하지만 public 으로 열어 둔것은 단순 계산 기능이 나중에 자주 호출 될 수도 있을거 같기 때문이고
그런일이 발생한다면 static으로 변경하는것을 고민 해 볼수 있지 않을까합니다.
(객체 생성을 통해 메소드 접근은 허용하되, 빈번하게 사용 된다면 static변경 고려)

banjjoknim · 2021-05-06T07:00:57Z

src/main/java/calculator/Calculator.java

+    }
+
+    private double makeResult(List<String> mathExpressions) {
+        double result = Double.parseDouble(mathExpressions.get(0));


get(0) 에서 0을 의미를 갖는 상수로 변경해보면 좋을 것 같아요 :)

banjjoknim · 2021-05-06T07:14:18Z

src/main/java/calculator/Calculator.java

+    }
+
+    private void isTheEndNotNumber(String mathExpression) {
+        if (Pattern.matches(THE_END_IS_NOT_NUMBER, mathExpression)) {


Pattern.matches 를 통해 입력값을 검증하고 있는데요~~ Pattern.matches()는 내부적으로 Pattern을 새로 생성한답니다. Ctrl을 누르고 메서드를 클릭하면 내부 구현을 살펴볼 수 있으니 참고하시면 좋을 것 같아요 :)
따라서, Pattern을 미리 생성해서 사용해보면 좋을 것 같네요! 😀
참고자료를 보시면 도움이 될 것 같아요 :)

절대 모를뻔한 정보네요... 신기합니다👍

banjjoknim · 2021-05-06T07:15:10Z

src/main/java/calculator/Calculator.java

+        if (Pattern.matches(NOT_NUMBER_PATTERN, mathSymbol)) {
+            return true;
+        }
+        return false;


여기엔 함정이 숨어있어요. 아래처럼 바꿀 수 있지 않을까요?

Suggested change

if (Pattern.matches(NOT_NUMBER_PATTERN, mathSymbol)) {

return true;

}

return false;

return Pattern.matches(NOT_NUMBER_PATTERN, mathSymbol);

banjjoknim · 2021-05-06T07:17:39Z

src/main/java/calculator/MathSymbol.java

@@ -0,0 +1,18 @@
+package calculator;
+
+public enum MathSymbol {


정말 단순하게, 아이스크림을 만드는 기계가 있다고 생각해봐요.
아이스크림을 만드는 기계는 재료가 어떤 녀석들인지(우유인지, 설탕인지 기타등등..) 반드시 알아야만 아이스크림을 만들 수 있을까요? 단순히 아이스크림을 만들기위한 재료를 넣기만 하면 만들어주지 않을까요? '객체는 주체성을 갖는다.' 라는 관점에서, 객체에게 메시지를 보낸다라고 생각해보면 좋을 것 같아요 :)

banjjoknim · 2021-05-06T08:48:52Z

src/test/java/RacingGameTest.java

+
+    @Test
+    @DisplayName("자동차 이름이 전부 5자 이내인 자동차 생성")
+    public void makeCarTest() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {


현재 이 테스트의 경우, 여러 개의 자동차 객체를 생성하고 있는 RacingGame#makeCar() 에 대한 테스트를 수행하고 있어요. 자동차 객체를 생성하고 List에 추가하는 기능을 수행하고 있는데, 실제 핵심 로직은 new Car()인 것 같다는 생각이 들어요. 그렇다면 @DisplayName에 명시한 자동차 이름이 5자 이내인 자동차 생성과는 다소 거리가 있어보이네요! 이 테스트는 CarTest에서 자동차를 생성하는 테스트로 변경할 수 있지 않을까요? :) 자동차 객체의 생성에서만 보면 CarTest에 좀 더 적합하지 않을까요? 😁
이와 더불어 테스트가 수정된다면, 연관성을 띄고 있는 다른 로직 및 테스트도 수정되어야 하겠죠? :)

banjjoknim · 2021-05-06T08:50:29Z

src/test/java/RandomNumberTest.java

+        //then
+        int number1 = randomNumber.makeOneRandomNumber();
+
+        Assertions.assertThat(number1).isBetween(1, 9);


Assertions.assertThat()에서, Assertions는 static import하여 사용하도록 해봐요 :)
비슷한 다른 부분들에도 적용해보아요.

banjjoknim · 2021-05-06T08:52:58Z

src/test/java/RacingGameTest.java

+
+    @Test
+    @DisplayName("자동차 이름중 일부가 5자 초과")
+    public void invalidMakeCarTest() {


자동차 객체를 생성하면서 이름의 길이가 5자 이상인지 검증하는 역할은 RacingGame과 Car 중 누구의 책임일까요? 그렇다면 테스트는 어디에 작성되어야 할까요?

banjjoknim · 2021-05-06T08:53:50Z

src/test/java/RacingGameTest.java

+        Car car = new Car(carNameArray[0]);
+        Car car2 = new Car(carNameArray[1]);
+        int car1BeforeDistance = car.getMoveCount();
+        int car2BeforeDistance = car2.getMoveCount();


car2BeforeDistance는 사용되고 있지 않은 변수네요!

banjjoknim · 2021-05-06T08:59:41Z

src/test/java/calculator/MathSymbolTest.java

+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class MathSymbolTest {


MathSymbolTest에서는 MathSymbol의 기능중에 getMathSymbol()만 테스트하고 있네요! 나머지 테스트도 작성해보면 좋을 것 같군요 :)

Miot2J added 18 commits April 26, 2021 22:39

Implement Calculator

6946e19

Implement Calculator

971c8fe

feat: 계산기 전부 구현

ece2a16

docs: 기능 목록 작성

1fc434f

feat: Input 기능 구현

f0bc23e

feat: output 기능 구현

4086fd1

test

53dfa0b

feat: 자동차 생성 기능 구현 & 단위 테스트 작성

af5ef88

feat: 한자리 난수 생성 기능 구현 & 단위 테스트 작성

cff3db6

feat: 난수가 4이상인 경우 자동차 전진 기능 구현 & 단위 테스트 작성

dab0e95

docs: 자동차 게임 로직 세부사항 수정

20b308e

feat: 자동차별 거리 이동 기능 구현 & 단위 테스트 작성

3a53522

feat: 게임 종료 시 우승자 확인 기능 구현 & 단위 테스트 작성

23188ea

docs: 자동차 게임 로직 수정

717351f

feat: 입력 횟수 만큼 자동차 거리 이동 반복 기능 구현 & 단위 테스트 작성

8787e4b

feat: 이동칸 출력 기능 추가 & 게임 종료 시 우승자 출력 기능 추가

7ff9e7a

feat: Main 추가 & 코드 정렬

6efffb4

merge: calculator

0ecaf72

banjjoknim requested changes Apr 30, 2021

View reviewed changes

Miot2J added 4 commits May 2, 2021 17:22

refactor: Calculate -> CalculatorFunction 으로 명사화

5a09d40

refactor: Ouput의 메서드를 static 메서드로 변경,calculatorGenerate -> inputExpre…

c8e84f3

…ssionAndPrintResult로변경

feat: FourRule(Enum Class) 추가

9992fcf

refactor: calculator 리뷰 내용 전부 수정완료

6b7e006

refactor: RacingGame 리뷰 내용 전부 수정완료

6a793c3

CarTest 에 @ParameterizedTest 구현완료 & Input,Output 에 static method 로 변경, 명확하지 않은 이름의 변수 클래스 새로 명명,특정 변수값 상수화 완료

banjjoknim requested changes May 3, 2021

View reviewed changes

Miot2J added 3 commits May 3, 2021 23:29

refactor: 클래스 이름 재정의 및 삭제, MathSymbol.Enum 생성

ed81517

Exception.java -> ExceptionMessage.java 변경, CalculatorFunction.java 삭제, MathSymbol 생성 및 enumTest() 메소드 구현.

refactor: 단일 책임 원칙에 따라 메소드 분리

f0dc71c

inputExpressionAndPrintResult()-> printResult(), makeSliceMathExpression() 으로 분리

Miot2J changed the title ~~Calculator & Step1 리뷰요청합니다.~~ Calculator & Step1 리뷰요청합니다. (calculator만 최종 수정했습니다.) May 3, 2021

banjjoknim requested changes May 4, 2021

View reviewed changes

Miot2J added 4 commits May 4, 2021 23:04

refactor: 메소드분리, 모호한 이름 새로 네이밍

1dd7b00

refactor: 기능 중복 으로 makeCar() 삭제, 동작하지 않는 테스트케이스 수정

0c6996e

기존 makeCar() 메서드에서 carList를 반환 했는데 makeCarList() 와 대부분 중복되었으며 Car 객체 생성 책임을 #RacingGame#makeCar()에서 가질 필요가 없었다.

refactor: 테스트케이스 기능 변경, ConvertString.java 삭제, 명확한 뜻을 가지게 네이밍

f1345d5

refactor: Car.java 의 생성자에 있던 길이 검증을 메서드화

fc0794a

validateLength() 메서드 추가

Miot2J changed the title ~~Calculator & Step1 리뷰요청합니다. (calculator만 최종 수정했습니다.)~~ Calculator & Step1 리뷰요청합니다. (주신 피드백 전부 반영했습니다.) May 5, 2021

Miot2J added 2 commits May 5, 2021 18:33

refactor: reformat Code

1748c47

refactor: 중복되는 메서드 제거

9f769af

Calculator 의 연산자를 찾아서 계산 메서드를 호출하는 부분을 MathSymbol 의 findValidatedSymbol & operate 로 변경

banjjoknim approved these changes May 6, 2021

View reviewed changes

banjjoknim merged commit fb8017d into next-step:miot2j May 6, 2021

Miot2J changed the title ~~Calculator & Step1 리뷰요청합니다. (주신 피드백 전부 반영했습니다.)~~ Step1(문자열계산기,자동차경주) 리뷰요청합니다. May 6, 2021

Step1(문자열계산기,자동차경주) 리뷰요청합니다. #2179

Step1(문자열계산기,자동차경주) 리뷰요청합니다. #2179

Conversation

Miot2J commented Apr 29, 2021

banjjoknim 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miot2J May 2, 2021 • edited

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

Miot2J commented May 2, 2021

banjjoknim left a comment • edited

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

Miot2J May 3, 2021 • edited

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

banjjoknim May 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miot2J commented May 3, 2021 • edited

banjjoknim 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

banjjoknim May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miot2J commented May 5, 2021

banjjoknim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miot2J May 10, 2021 • edited

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

Miot2J May 2, 2021 •

edited

banjjoknim left a comment •

edited

Miot2J May 3, 2021 •

edited

banjjoknim May 2, 2021 •

edited

Miot2J commented May 3, 2021 •

edited

banjjoknim May 4, 2021 •

edited

Miot2J May 10, 2021 •

edited