[차재언] Step2(RacingGame) 피드백 반영. #2204

Miot2J · 2021-05-10T06:50:12Z

항상 좋은 피드백 주셔서 감사합니다!👍

Pattern.matches() 사용시 내부적으로 Pattern을 생성해 성능을 위해 외부에서 pattern 미리 선언 후 match만 호출.

객체의 책임을 다른 클래스에서 가지고있던 부분을 리팩토링함.

banjjoknim

안녕하세요 재언님! 자동차 경주 게임 리뷰를 맡고 있는 '배재홍' 이라고 해요 :)
생각할 것들이 많아지면서, 미션을 수행하기 쉽지 않았을텐데 잘 해주셨네요! 💯 👍

코멘트들을 남겨드렸으니, 확인해보시고 계속해서 미션 진행해주시면 될 것 같아요 :)
그 외에, 질문 주신 부분들에 대해서는 추가적으로 DM을 드리도록 할게요.
궁금한 점 있으시면 언제든지 편하게 DM 주세요 :)

banjjoknim · 2021-05-11T09:04:09Z

src/main/java/calculator/Calculator.java

+    Pattern notNumberPattern = Pattern.compile(NOT_NUMBER_PATTERN);
+    Pattern theEndIsNotNumberPattern = Pattern.compile(THE_END_IS_NOT_NUMBER);
+    Pattern isNotMathExpressionPattern = Pattern.compile(IS_NOT_MATH_EXPRESSION);


패턴을 미리 만들어주셨네요! 👍 👍 👍

banjjoknim · 2021-05-11T09:04:21Z

src/main/java/calculator/Calculator.java

    }

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

    private double makeResult(List<String> mathExpressions) {
-        double result = Double.parseDouble(mathExpressions.get(0));
+        double result = Double.parseDouble(mathExpressions.get(FIRST_NUMBER_INDEX));
        int mathExpressionSize = mathExpressions.size();

        for (int i = 1; i < mathExpressionSize; i += 2) {


반복문에서 2라는 숫자에 어떤 의미가 있을까요? 의미를 드러낼 수 있도록 해봐요 :)

banjjoknim · 2021-05-11T09:04:37Z

src/main/java/calculator/Calculator.java

@@ -54,13 +59,15 @@ private void validate(String mathExpression) {
    }

    private void isTheEndNotNumber(String mathExpression) {


boolean을 반환하는 기능이 아닌, 검증을 하고 예외를 발생시키는 기능을 하고 있어요. 조금 더 검증에 알맞는 이름을 지어주면 어떨까요? 다른 부분에도 같이 적용해보면 좋겠죠? 😁

banjjoknim · 2021-05-11T09:05:17Z

src/main/java/calculator/MathSymbol.java

                .findAny()
-                .orElseThrow(() -> new IllegalArgumentException(String.format("잘못된 연산자 입니다.")));
+                .orElseThrow(() -> new IllegalArgumentException("잘못된 연산자 입니다."));
    }

    public double operate(double firstNumber, double secondNumber) {


operate() 내부에서 비교를 수행할 때, getXXX() 를 통해 상태를 다시 꺼내어(?) 사용하고 있어요. 이 부분은 바로 상태에 접근해서 사용해보면 어떨까요? 그렇게 하면 어떤 효과가 있을까요?

음.. 메소드 호출을 한 번 줄일 수 있어서
자원? 의 낭비가 줄어들겠네요

banjjoknim · 2021-05-11T09:05:37Z

src/main/java/racing/RacingController.java

+import static racing.domain.RacingGame.SEPARATOR;
+import static racing.view.Input.makeCarNames;
+import static racing.view.Input.makeGameRepeatCount;
+import static racing.view.Output.*;
+import static racing.view.Output.printWinMessage;


import static을 사용하면 코드 작성은 편해질 수 있으나, 메서드가 어디에서 작성되어 있는지 파악하기가 어려울 수도 있어요. 적절히 사용하면 좋을 것 같네요! :)

banjjoknim · 2021-05-11T10:22:03Z

src/test/java/RandomNumberTest.java

@@ -11,6 +13,6 @@ public void makeNumber() {
        //then
        int number1 = randomNumber.makeOneRandomNumber();


RandomNumber 객체를 만들어서 static 메서드인 makeOneRandomNumber()를 호출할 필요가 있을까요?
number1 보다 좀 더 적절한 이름을 지어주면 어떨까요? :)

banjjoknim · 2021-05-11T10:22:22Z

src/test/java/calculator/MathSymbolTest.java

+import static org.assertj.core.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
 public class MathSymbolTest {


현재 MathSymbolTest에는 + 연산자에 대한 테스트만 있어요. 다른 연산자에 대한 테스트도 추가해보면 어떨까요?

banjjoknim · 2021-05-11T10:22:35Z

src/test/java/calculator/MathSymbolTest.java

+    void findValidatedSymbolTest() {
+        assertAll(
+                () -> assertThat(MathSymbol.findValidatedSymbol("+")).isEqualTo(MathSymbol.PLUS),
+                () -> assertThrows(IllegalArgumentException.class,() -> MathSymbol.findValidatedSymbol("&"))


예외에 대한 테스트도 작성해주셨네요! 👍 👍 👍
다만, 예외에 대한 테스트와 잘 동작할 때의 테스트를 나눠서 작성해보면 어떨까요? 그러면 어떤 효과가 있을까요?

나중에 유지보수가 편해질거같습니다
(메인 코드를 추가했을때 잘 동작하는 테스트 코드에 추가만 하면됨)

banjjoknim · 2021-05-11T10:28:03Z

src/main/java/racing/RacingController.java

+        printInputCountMessage();
+        int count = makeGameRepeatCount();


입력시 메시지 출력과 입력은 모두 View에서 이루어지고 있어요. 그렇다면 이러한 것들을 View 에 모아놓으면 어떨까요? 그러면 어떤 효과가 있을까요?

컨트롤러에서 Output, Input에 총두번 접근하는 대신
한번만 접근해도 된다??

대신 Output과 Input의 메서드가 많아지면 한번에 관리하기는 어려워진다?

banjjoknim · 2021-05-11T10:39:00Z

src/main/java/racing/domain/RacingGame.java

+
+import static racing.utils.RandomNumber.makeOneRandomNumber;
+
+public class RacingGame {


RacingGame의 기능들을 살펴보면, List<Car>를 관리하고 있다는 것을 알 수 있어요. 그렇다면 List<Car>를 상태값으로 가져보면 어떨까요?

List를 상태로 가졌더니 메소드 내부에서 List를 중복해서 선언하던 코드들이사라지고
리턴값도 필요없게되었으며 파라미터도 필요가 없게 되었네요
코드들이 훨씬 간결해 진거 같습니다.

또한 RacingGame 객체를 선언하고 관리하는 컨트롤러에서도 리스트를 갱신하는 일이 사라졌네요

List<Car> 를 선언하여, RacingGame의 메서드들의 파라미터와 리턴값을 제거하고 list를 매번 새로 생성하는 코드를 제거 하였음

banjjoknim

안녕하세요 재언님!
미션 진행을 잘 해주셔서, 처음보다 많이 좋아진 것 같아요 :)
피드백을 반영하기 위해 고민하고 노력한 흔적들이 보였어요 👍
몇 가지 코멘트를 남겨드렸어요!
미션 진행하시면서 궁금한 점이 생기시면 언제든지 DM 주세요! 😁

banjjoknim · 2021-05-13T10:52:18Z

src/main/java/calculator/Calculator.java

        outputStartMessage();
-        resultOutput(calculator.makeResult(makeSlicedMathExpression()));
+        resultOutput(makeResult(makeSlicedMathExpression()));
    }

    public double calculate(double firstNumber, String operator, double secondNumber) {


calculate()는 Calculator에서만 사용되고 있어요. public으로 설정하신 이유가 있을까요? :)

banjjoknim · 2021-05-13T10:54:13Z

src/main/java/calculator/Calculator.java

        isInputNull(mathExpression);
        isNotMathExpression(mathExpression);
-        isTheEndNotNumber(mathExpression);
+        validateTheEndNotNumber(mathExpression);


같은 범주의 기능을 수행한다면, 이름 또한 일관성있게 지어보면 어떨까요?

banjjoknim · 2021-05-13T10:57:13Z

src/main/java/calculator/Calculator.java

-    private void isTheEndNotNumber(String mathExpression) {
-        if (Pattern.matches(THE_END_IS_NOT_NUMBER, mathExpression)) {
+    private void validateTheEndNotNumber(String mathExpression) {
+        Matcher matcher = theEndIsNotNumberPattern.matcher(mathExpression);


matcher 라는 이름은 의도가 다소 약하게 드러나는 것 같아요. matcher.matches()를 boolean 변수로 만들어서 의미를 부여하고 사용해보면 어떨까요? :)

banjjoknim · 2021-05-13T11:40:42Z

src/main/java/calculator/MathSymbol.java

@@ -22,22 +22,22 @@ public String getMathSymbol() {

    public static MathSymbol findValidatedSymbol(String operator) {
        return Arrays.stream(MathSymbol.values())
-                .filter(v -> v.getMathSymbol().equals(operator))
+                .filter(mathSymbol -> mathSymbol.getMathSymbol().equals(operator))


mathSymbol.getMathSymbol() 대신 바로 mathSymbol 필드에 접근해보면 어떨까요?

ㅎㅎ 이런 실수를

banjjoknim · 2021-05-13T12:15:28Z

src/main/java/calculator/MathSymbol.java

    }

    public double operate(double firstNumber, double secondNumber) {
-        if (PLUS.getMathSymbol() == this.getMathSymbol()) {
+        if (PLUS.mathSymbol == this.getMathSymbol()) {


아래와 같이 변경해보면 어떨까요?

Suggested change

if (PLUS.mathSymbol == this.getMathSymbol()) {

if (PLUS.mathSymbol == this.mathSymbol) {

추가적으로, 문자열의 비교는 == 대신 equals()를 통해 해요 :)
참고자료

banjjoknim · 2021-05-13T13:11:55Z

src/main/java/racing/view/Output.java

-            System.out.print(carList.get(i).getName() + DISTANCE_SEPARATOR);
+            distanceStringBuilder.append(carList.get(i).getName());
+            distanceStringBuilder.append(DISTANCE_SEPARATOR);
+            System.out.print(distanceStringBuilder);


출력 형식에 맞게 문자열 데이터의 가공을 완료한 뒤, 한번에 출력해보면 어떨까요? :)

banjjoknim · 2021-05-13T13:15:47Z

src/test/java/CarTest.java

+        car2.move(4);
+
+        //then
+        assertThat(car.isWinner(3)).isTrue();


car2에 대해서도 테스트를 작성해보면 어떨까요? :)

banjjoknim · 2021-05-13T13:19:21Z

src/test/java/RacingGameTest.java

-                () -> Assertions.assertDoesNotThrow(() -> new Car(carNameArray[0]).move(4)),
-                () -> Assertions.assertDoesNotThrow(() -> new Car(carNameArray[0]).move(5))
-        );
+        assertThat(winnerList.size()).isEqualTo(1);


winnerList의 size와 함께, winner 또한 잘 맞는지 테스트해보면 어떨까요?

banjjoknim · 2021-05-13T13:23:02Z

src/test/java/calculator/MathSymbolTest.java

    @Test
-    void enumTest() {
-        Assertions.assertThat(MathSymbol.PLUS.getMathSymbol()).isEqualTo("+");
+    void getMathSymbolTest() {
+        assertAll(
+                () -> assertThat(MathSymbol.PLUS.getMathSymbol()).isEqualTo("+"),
+                () -> assertThat(MathSymbol.MINUS.getMathSymbol()).isEqualTo("-"),
+                () -> assertThat(MathSymbol.MULTIPLE.getMathSymbol()).isEqualTo("*"),
+                () -> assertThat(MathSymbol.DIVIDE.getMathSymbol()).isEqualTo("/")
+        );
+    }


@ParameterizedTest의 @MethodSource를 사용해봐도 좋을 것 같아요 :)
참고자료가 도움이 될 것 같네요! 😁

banjjoknim · 2021-05-13T13:24:39Z

src/test/java/calculator/MathSymbolTest.java

+    @Test
+    void operateTest() {
+        assertThat(MathSymbol.PLUS.operate(1, 3)).isEqualTo(4);


+ 말고 다른 연산기호에 대한 테스트도 같이해보면 좋을 것 같아요!

racing 패키지의 정적 출력부분을 Input에서 동작하도록 변경

banjjoknim

안녕하세요 재언님! :)
계산기 기능 구현이 인상적이었어요. 😁
코멘트를 남겨드렸으니 확인해보시고 계속해서 진행해주시면 될 것 같습니다!😄
미션 진행하는게 쉽지 않으실텐데, 화이팅입니다 ^^!

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

banjjoknim · 2021-05-18T00:20:23Z

src/main/java/calculator/MathSymbol.java

@@ -22,24 +40,26 @@ public String getMathSymbol() {

    public static MathSymbol findValidatedSymbol(String operator) {
        return Arrays.stream(MathSymbol.values())
-                .filter(v -> v.getMathSymbol().equals(operator))
+                .filter(mathSymbol -> mathSymbol.mathSymbol.equals(operator))


mathSymbol.mathSymbol이 다소 어색한 느낌이 들긴 하지만, 적절한게 지금은 떠오르지 않네요.
조금 더 좋은 네이밍이 떠오른다면 바꿔봐도 좋을 것 같아요! :)

banjjoknim · 2021-05-18T00:23:32Z

src/main/java/calculator/MathSymbol.java

-
-    public double operate(double firstNumber, double secondNumber) {
-        if (PLUS.getMathSymbol() == this.getMathSymbol()) {
+    // 블로그 작성용으로 잠시만 남겨 두겠습니다.


가능하면 이런 주석은 반드시 지워주세요 😥

banjjoknim · 2021-05-18T00:23:49Z

src/main/java/calculator/MathSymbol.java

-    }
+    }*/
+
+    abstract double operate(double firstNumber , double secondNumber);


추상 메서드를 이용해 각각의 연산자에게 연산 행위를 정의해주셨네요! 👍

banjjoknim · 2021-05-18T00:25:10Z

src/main/java/racing/RacingController.java

+        printInputCountMessage();
+        int count = makeGameRepeatCount();
+        racingGame.repeatMoveCars(count);
+        printNowDistance(racingGame.getCars());


아래와 같이 마지막 결과 한번만 나오고 있어요. 중간 과정이 안나오고 있다는 뜻이었어요. :)

banjjoknim · 2021-05-18T00:59:03Z

src/main/java/racing/domain/Car.java

    public void move(int randomNumber) {
-        if (randomNumber >= 4) {
+        if (randomNumber >= MOVE_CHANGE_CONDITION) {


Car가 다양한 방법으로 움직일 수 있도록 해보면 어떨까요? :)

banjjoknim · 2021-05-18T01:14:56Z

src/test/java/RandomNumberTest.java

@@ -2,6 +2,8 @@
 import org.junit.jupiter.api.Test;
 import racing.utils.RandomNumber;

+import static org.assertj.core.api.Assertions.*;
+
 public class RandomNumberTest {


RandomNumberTest 의 패키지도 프로덕션 코드의 RandomNumber 클래스와 동일하게 해주면 어떨까요? 그러면 어떤 효과가 있을까요?

banjjoknim · 2021-05-18T01:15:54Z

src/test/java/calculator/CalculatorTest.java

@@ -4,6 +4,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;

+import static org.assertj.core.api.Assertions.*;


사용되지 않는 import 문 이네요!

banjjoknim · 2021-05-18T01:17:15Z

src/test/java/calculator/CalculatorTest.java

@@ -4,6 +4,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;

+import static org.assertj.core.api.Assertions.*;
+
 public class CalculatorTest {


현재 실패하는 테스트가 포함되어 있어요. 잘 점검하도록 해봐요. :)

으.. 항상 수정하고 테스트 돌린 후 제출한다고 생각하는데
자꾸 실수하네요 ㅠㅠ 습관화 하겠습니다😞

banjjoknim · 2021-05-18T01:20:16Z

src/test/java/calculator/MathSymbolTest.java

+    @Test
+    void operateTest() {
+        assertAll(
+                () -> assertThat(MathSymbol.PLUS.operate(1, 31)).isEqualTo(32),
+                () -> assertThat(MathSymbol.MINUS.operate(40, 31)).isEqualTo(9),
+                () -> assertThat(MathSymbol.MULTIPLE.operate(2, 10)).isEqualTo(20),
+                () -> assertThat(MathSymbol.DIVIDE.operate(10, 2)).isEqualTo(5)
+        );
+
+    }


이 테스트는 실패하고 있어요. 잘 점검하도록 해봐요. :)
추가적으로, @DisplayName을 이용하여 좀 더 테스트의 내용을 명시적으로 표현해주면 어떨까요?

banjjoknim · 2021-05-18T01:21:22Z

src/test/java/calculator/MathSymbolTest.java

+    @ParameterizedTest
+    @MethodSource("provideMathSymbols")
+    @DisplayName("MathSymbol의 모든 상수 검증 테스트")
+    void wholeMathSymbolTest(String symbol, MathSymbol Mathsymbol) {
+        assertThat(MathSymbol.findValidatedSymbol(symbol)).isEqualTo(Mathsymbol);
    }
+
+    private static Stream<Arguments> provideMathSymbols() {
+        return Stream.of(
+                Arguments.of("+", MathSymbol.PLUS),
+                Arguments.of("-", MathSymbol.MINUS),
+                Arguments.of("*", MathSymbol.MULTIPLE),
+                Arguments.of("/", MathSymbol.DIVIDE)
+        );
+    }


모든 경우의 수(MathSymbol의 모든 상수)에 대한 테스트를 작성해주셨네요! 👍

…턴 타입 변경 RacingGame이 List<String> 타입을 반환하는 대신 List<Car>를 반환하도록 변경, Car객체의 move()의 조건을 다양화 하기위해 MoveConditionStrategy인터페이스 생성 후 구현체 OverFourStrategy 생성

banjjoknim

재언님 안녕하세요! 😊
미션도 열심히 해주시고 질문도 활발하게 해주고 계신데요~!
정말 잘해주고 계신 것 같아서 뿌듯함을 많이 느끼고 있답니다. ❤
코멘트를 남겨드렸는데, 고민해보시고 반영해보시면 좋을 것 같아요!
분명 막히는 부분이나 생각이 잘 풀리지 않는 부분이 있을 수도 있을거란 생각이 들어요.
그럴때면 편하게 DM으로 질문주시면 같이 고민해볼 수 있을 것 같아요!
우리는 함께 자라고 있는거겠죠? ^^!
화이팅이에요~!! 👏👍🤞

banjjoknim · 2021-05-20T02:54:50Z

src/test/java/calculator/CalculatorTest.java

    }

    @Test
    void divisionTest() {
        double first = 12;
        double second = 4;
-        Assertions.assertThat(calculator.calculate(first, "/", second)).isEqualTo(3);
+        assertThat(calculator.calculate(first, "/", second)).isEqualTo(3);


이제보니, 0으로 나누는 것에 대한 예외 테스트도 작성해보면 좋을 것 같네요 :)

banjjoknim · 2021-05-20T05:37:30Z

src/main/java/calculator/Calculator.java

+    Pattern notNumberPattern = Pattern.compile(NOT_NUMBER_PATTERN);
+    Pattern theEndIsNotNumberPattern = Pattern.compile(THE_END_IS_NOT_NUMBER);
+    Pattern isNotMathExpressionPattern = Pattern.compile(IS_NOT_MATH_EXPRESSION);


미리 만들어주신 세 패턴은 쉽게 변하지 않을거란 생각이 들어요. 그렇다면 상수로 변경해보면 어떨까요? :)
추가적으로, Calculator 내에서만 사용되고 있네요! :)

상수 선언을 할때, 원시형만 이용해서 선언하는 고정관념이 있었네요 😀

banjjoknim · 2021-05-20T07:01:40Z

src/main/java/calculator/MathSymbol.java

+    DIVIDE("/") {
+        @Override
+        double operate(double firstNumber, double secondNumber) {
+            return firstNumber / secondNumber;


0으로 나누는 경우에는 예외를 발생시켜보면 어떨까요? :)

banjjoknim · 2021-05-20T07:02:13Z

src/main/java/racing/RacingController.java

+        printWinMessage(racingGame.findWinner());
+    }
+
+    private static List<Car> makeCars(String[] carNameArray) {


carNameArray는 자료구조가 이름에 포함되어 있어요. 이럴 경우 자료구조가 변경된다면 어떤 일이 발생할까요? :)

아래 cars 와 달리 자료형이 변경 될리 없다는 전제로 일부러 Array라 이름 붙였는데,
굳이 제약을 걸 필요는 없을 것 같네요😀

banjjoknim · 2021-05-20T07:02:40Z

src/main/java/racing/RacingController.java

+
+    private static List<Car> makeCars(String[] carNameArray) {
+        List<Car> cars = new ArrayList<>();
+        MoveConditionStrategy moveConditionStrategy = new OverFourStrategy();


makeCars를 호출하면서 파라미터로 전략을 넣어주면 어떨까요? 그러면 어떤 효과가 있을까요? :)

조금 더 변경에 자유로워 질 것 같습니다!
전략을 선택하고 리턴하는 메서드를 만들어 전략을 쉽게 변경 할 수도 있겠네요😀

banjjoknim · 2021-05-20T07:07:46Z

src/main/java/racing/view/Output.java

+        for (int i = 0; i <= lastMemberIndexOfRequiringSeparator; i++) {
+            winMessageStringBuilder.append(winnerList.get(i).getName());
+            winMessageStringBuilder.append(WINNER_SEPARATOR);
+            System.out.print(winMessageStringBuilder);
+            winMessageStringBuilder.setLength(BUILDER_INIT_NUMBER);
        }
-        System.out.println(winnerList.get(memberIndexOfNotRequiringSeparator));
+        System.out.println(winnerList.get(lastMemberIndexOfNotRequiringSeparator).getName());


printWinMessage() 내에서 출력할 우승자의 이름을 만들어주고 있어요. 현재, for 문의 루프를 돌면서 복잡한 과정을 거치고 있는데, 이제 이 부분을 개선해보면 좋을 것 같아요. String.join() 이라는 자바 API를 사용해보면 크게 개선할 수 있을 것 같네요! :) 참고자료가 도움이 될 것 같아요! 이 외에도 다양한 자바 API를 사용하도록 노력해봐요!

굉장히 좋은 api가 있었네요 ㅎㅎㅎ
감사합니다! 😀

banjjoknim · 2021-05-20T07:08:09Z

src/main/java/racing/view/Output.java

+        for (Car car : carList) {
+            distanceStringBuilder.append(car.getName());
+            distanceStringBuilder.append(DISTANCE_SEPARATOR);
+            StringBuilder loadBuilder = repeatAppendLoad(car.getMoveCount());
+            distanceStringBuilder.append(loadBuilder);
+            distanceStringBuilder.append(System.lineSeparator());
        }


StringBuilder#append()는 메서드 체이닝으로 연결해서 사용할 수도 있어요. StringBuilder#append()가 StringBuilder를 반환하고 있기 때문인데요~ 내부 구현을 살펴보는 것도 좋을 것 같아요! :)

문맥을 연결할때 사용하면 좋을 것 같네요
저도 같은 문맥 끼리 묶어 봤습니다.

banjjoknim · 2021-05-20T07:08:55Z

src/test/java/racing/domain/CarTest.java

+    @DisplayName("난수가 4이상일때 거리 증가")
+    public void moveTest() {
+        //given
+        OverFourStrategy moveConditionStrategy = new OverFourStrategy();


Car 객체를 생성할 때 반복적으로 OverFourStrategy 객체를 생성하고 있어요. @BeforeEach 등으로 미리 초기화하도록 만들어둔 뒤 사용해보면 어떨까요? :) 테스트 픽스처에 대해서 알아보면 좋을 것 같네요.
추가적으로, OverFourStrategy 대신 MoveConditionStrategy 타입으로 만들어보면 어떨까요? 그러면 어떤 차이가 있을까요?

변경에 자유로워 질 것 같습니다

banjjoknim · 2021-05-20T07:11:03Z

src/test/java/racing/domain/CarTest.java

+    @DisplayName("가장 많이 움직인 자동차의 이동 횟수가 승자 조건이다")
+    @ParameterizedTest
+    @CsvSource(value = {"456"})
+    void isWinner(String input) {
+        //given
+        OverFourStrategy moveConditionStrategy = new OverFourStrategy();
+        Car car = new Car("aaa", moveConditionStrategy);
+        List<String> numbers = Arrays.asList(input.split(""));
+
+        //when
+        for (String number : numbers) {
+            car.move(Integer.parseInt(number));
+        }
+
+        //then
+        assertThat(car.isWinner(3)).isTrue();
+    }


가장 많이 움직인 자동차의 이동 횟수가 승자 조건이다. 라는 테스트의 설명이 다소 부적절한 느낌이 드는 것 같아요! 여기서는 Car 객체가 승자인지 아닌지를 판단하는 기능을 테스트하고 있어요. :)
추가적으로, 특별히 @CsvSource를 사용했지만, 현재 테스트에서는 그럴만한 요소는 보이지 않는 것 같아요.
반드시 @CsvSource를 사용해야하는 이유가 있을까요? :)

저 당시에는 move를 3회 호출하는 코드를 줄이려 사용했었습니다.
테스트 코드가 불 필요하다고 생각되어 삭제 했습니다

banjjoknim · 2021-05-20T07:11:45Z

src/test/java/racing/domain/RacingGameTest.java

+        String carString = "aaa,bbb,ccc,ddd,eee,fff";
+        String[] carNameArray = carString.split(SEPARATOR);
+        List<Car> cars = new ArrayList<>();
+        MoveConditionStrategy moveConditionStrategy = new OverFourStrategy();
+        for (String carName : carNameArray) {
+            cars.add(new Car(carName,moveConditionStrategy));
+        }
+        RacingGame racingGame = new RacingGame(cars);
+        racingGame.getCars().get(0).move(5);
+
+        //when
+        List<Car> winnerList = racingGame.findWinner();


우승자를 찾는 기능을 테스트하기위한 준비 과정이 복잡해보여요. 과연 RacingGame#findWinner()를 테스트하는데에 List<Car> cars가 생성되는 과정이 모두 필요할까요? 이미 cars의 생성은 CarTest에서 검증이 되어야하고, 이미 검증된 상태라고 생각되요. 그렇다면, RacingGame에서는 바로 List<Car> cars를 생성해서 테스트를 해도되지 않을까요? :) (ex. Arrays.asList(~~~))

@FunctionalInterface

테스트 메소드를 위해 boolean 반환이 필요했지만 @FunctionalInterface 사용하여 테스트 코드에서 람다식으로 boolean형 리턴값을 고정시켜 사용

banjjoknim

안녕하세요 재언님!
새로운 시각에서 접근하고 익숙하지 않은 것들이 많으시겠죠?
그러다보니 미션 진행하기가 쉽지 않으셨을텐데 열심히 잘 해주고 계시네요! 👏👍
코멘트를 남겨드렸으니, 확인해보시고 계속해서 미션 진행해주시면 될 것 같아요.
미션 진행하시면서 궁금한 점 있으시면 늘 그렇듯, 편하게 DM 주세요! 😉
화이팅입니다~! ^^! 😁😊❤🤞

banjjoknim · 2021-05-24T07:37:52Z

src/main/java/calculator/Calculator.java

+    private static final String NOT_NUMBER_REGEX = "[^0-9]";
+    private static final String THE_END_IS_NOT_NUMBER_REGEX = "[0-9]*$";
+    private static final String IS_NOT_MATH_EXPRESSION_REGEX = "[^0-9+\\-*/]";


기본적으로 boolean 변수명을 지을 때, 긍정의 뜻을 가진 이름을 짓도록 해요. 부정의 뜻을 가진 이름을 지어주면 false를 반환해야할 때, 한번 더 부정으로 생각해야하기 때문이에요. 현재 이 상수들은 부정의 뜻을 가지고 있어요. boolean을 반환할 때 사용되는 Match 객체를 생성할 때 사용되고 있기 때문에 긍정의 뜻을 가지도록 이름을 변경해주면 좋을 것 같네요 :)
예를 들어, NOT_NUMBER_REGEX의 부정형은 숫자 정규식에 해당하지 않는 것이 아니다 라는 느낌으로.. 다소 해석이 불편해질 수 있겠죠?

banjjoknim · 2021-05-24T07:42:20Z

src/main/java/calculator/MathSymbol.java

+            if (firstNumber == 0 || secondNumber == 0) {
+                throw new IllegalArgumentException("0이 포함된 값으로 나눗셈을 할 수 없습니다.");
+            }


분자에 0이 있는 경우는 나눗셈을 할 수 있는걸로 알고 있어요 :)

banjjoknim · 2021-05-24T07:50:49Z

src/main/java/racing/RacingController.java

+        int repeatCount = Input.makeGameRepeatCount();
+        racingGame.repeatMoveCars(repeatCount);
+
+        printResultMessage();


static import를 하면 편리하지만, 현재 클래스의 멤버인걸로 오해할 수 있으니 가능하면 어떤 클래스에 속한 메서드인지 명시해주는게 좋을 것 같다는 생각이 드네요 :)

output을 생략하는 대신 print라는 동사를 공통적으로 붙였는데
코드가 아주 많아지면 output을 명시하는게 좀 더 가독성이 좋을 것 같기도 하네요👍

banjjoknim · 2021-05-24T07:53:39Z

src/main/java/racing/domain/Car.java

+    public Car(String name, MoveConditionStrategy moveConditionStrategy, int initDistance) {
+        validateLength(name);
+        this.name = name;
+        this.moveConditionStrategy = moveConditionStrategy;
+        this.movedLog = new MovedLog(initDistance);
+    }


생성자 오버로딩을 하기로 했다면, this()를 사용해보면 어떨까요? :)

맞다... 좋은 중복제거 방법이 있었죠 ㅎㅎ😀😀
예전에 한번 배운것 같은데 써먹은적이 없어서 까먹었었네요 😢

banjjoknim · 2021-05-24T08:03:24Z

src/main/java/racing/domain/Car.java

+    }
+
+    public boolean isWinner(int winnerCount) {
+        return movedLog.getFinalDistance() == winnerCount;


MovedLog 객체가 자동차의 위치를 관리하기로 했다면, MovedLog 객체에게 물어보면 어떨까요? :)

banjjoknim · 2021-05-24T08:45:49Z

src/test/java/calculator/CalculatorTest.java

+        double zero = 0;
+
+        assertThat(calculator.calculate(first, "/", second)).isEqualTo(3);
+        assertThrows(IllegalArgumentException.class,()->{


만약 여러 테스트를 한번에 진행할 때는 assertAll()을 사용하도록 해요. 이 테스트는 바로 위의 테스트가 실패하면 진행되지 않을거에요. :)
추가적으로, 예외 상황과 그렇지 않은 상황을 나눠서 테스트를 진행해보면 어떨까요? 그렇게 하면 어떤 효과가 있을까요?

예외처리가 필요한 코드를 직관적으로 알 수있어, 다른 개발자가 예외 처리를 염두에 두고 개발 할 수 있을것 같아요😊

banjjoknim · 2021-05-24T08:48:09Z

src/test/java/racing/domain/CarTest.java

+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+public class CarTest {
+    MoveConditionStrategy moveConditionStrategy;


테스트 역시 필드를 선언했다면, 그에 대한 접근제한자를 적절히 설정해주도록 해요 :)

굳이 public으로 열 필요가 없겠네요, 다른 테스트에서 사용하지 않으니까요 ㅎㅎ

banjjoknim · 2021-05-24T08:49:16Z

src/test/java/racing/domain/CarTest.java

+    @Test
+    @DisplayName("자동차가 이동")
+    public void move() {
+        Car car = new Car("aaa", () -> true);


람다식을 이용해서 전략을 지정해주셨네요! 👍 💯
메서드 시그니처에 대해서 찾아보면 도움이 될 수도 있을 것 같아요 :)

아하 사실 코드가 잘 이해 안갔는데 이제 이해가 된 것 같아요😀
메서드 시그니쳐가 동일하면 컴파일러가 동일한 메소드라고 판단을 할테고
new Car(name, Strategy) 형태 대신 new Car(name, boolean)을 사용했는데 코드가 동작하는건
컴파일러가 Strategy 와 boolean이 동일 하다고 인식 한다는 것 이고
Strategy가 @FuntionalInterface를 사용해 리턴값이 bool타입인 isMovable() 자체를 값으로 재사용 할 수 있기에
Strategy가 bool타입으로 재사용 된거군요👍

여기서 제가 잘 이해했는지 의문점이 생기는데
만약 리턴값이 bool타입인 함수 대신 String타입인 함수가 하나 선언 되어있다면
new Car(name,"내가 강제로 입력한 문자")로 선언하여 "내가 강제로 입력한 문자" 를 활용 할 수 있다!
라고 제대로 이해 한게 맞을까요 🤔😀

banjjoknim · 2021-05-24T08:53:22Z

src/test/java/racing/domain/CarTest.java

+    @DisplayName("자동차 이름이 5자 초과면 에러")
+    @ParameterizedTest
+    @CsvSource(value = {"123456", "1234567", "12345678"})
+    public void invalidMakeCarTest(String name) {
+        assertThatExceptionOfType(IllegalArgumentException.class)
+                .isThrownBy(() -> {
+                    new Car(name, moveConditionStrategy);
+                });
+    }
+}


테스트를 할 때, 지정된 요구사항 또는 조건의 경계에 대해서도 테스트를 진행해보면 좋을 것 같네요!

5자 초과인 예외처리 부분은 경계 테스트를 해서
위쪽의 5자 이내인 테스트 부분에서 5자, 4자의 경계값 테스트를 추가 했습니다.

banjjoknim · 2021-05-24T09:00:28Z

src/test/java/racing/domain/RacingGameTest.java

+    @DisplayName("첫번째 자동차인 aaa가 움직이면 우승차는 aaa 1대")
+    @Test
+    public void findSingleWinnerTest() {
+        //given
+        List<Car> cars = Arrays.asList(
+                new Car("aaa", moveConditionStrategy, 1),
+                new Car("bbb", moveConditionStrategy, 0),
+                new Car("ccc", moveConditionStrategy, 0),
+                new Car("ddd", moveConditionStrategy, 0),
+                new Car("eee", moveConditionStrategy, 0)
+        );
+        RacingGame racingGame = new RacingGame(cars);
+
+        //when
+        List<Car> winnerList = racingGame.findWinner();
+
+        //then
+        assertAll(
+                () -> assertThat(winnerList.size()).isEqualTo(1),
+                () -> assertThat(winnerList.get(0).getName()).isEqualTo("aaa")
+        );
+    }
+
+    @DisplayName("모든 자동차가 움직이면 전체 공동 우승")
+    @Test
+    public void findWholeWinnerTest() {
+        //given
+        List<Car> cars = Arrays.asList(
+                new Car("aaa", moveConditionStrategy, 2),
+                new Car("bbb", moveConditionStrategy, 2),
+                new Car("ccc", moveConditionStrategy, 2),
+                new Car("ddd", moveConditionStrategy, 2),
+                new Car("eee", moveConditionStrategy, 2)
+        );
+        RacingGame racingGame = new RacingGame(cars);
+
+        //when
+        List<Car> winnerList = racingGame.findWinner();
+
+        //then
+        assertThat(winnerList.size()).isEqualTo(5);
+    }


두 테스트 모두 자동차가 움직일 경우 우승자를 찾는 테스트이지만, 자동차는 움직이지 않고 위치의 초기화만 이루어지고 있네요!
개인적인 기호이긴 하지만, 특정 자동차를 우승자로 만들기 위해서 의도적으로 움직인 후, 해당 자동차의 위치가 우승자의 위치와 동일한지 비교해보는 건 어떨까요? RacingGame#findWinner()의 시나리오와 가능한 동일하게 테스트를 진행해보면 어떨까? 하는 생각이에요 :)
물론, 자동차를 움직이지 않았다고 해서 정확하지 않은 테스트라는 것은 절대 아니에요! 현재로서도 충분히 테스트를 잘 해주고 있다고 생각하며, 실제 시나리오와 비슷하게 move() 메서드를 호출하는 테스트 코드를 작성해보는 것도 나쁘지 않을 것 같다는 의견을 드려봐요 :) (개인적인 생각으로는 조금 더 안정감을 얻을 수 있을 것 같아요)

그럼 한대만 움직이는 경우는 한대만 움직여 시나리오대로 검증을하고
전부 움직인 경우는 코드가 길어져 가독성이 떨어 질 수도있으니
모두 움직였다고 가정하여 움직인 위치로 초기화 시켜 테스트를 하도록 해 보겠습니다.

banjjoknim · 2021-05-24T12:02:53Z

src/main/java/racing/domain/Car.java

+    public void move() {
+        boolean canMove = moveConditionStrategy.isMovable();
+        if (canMove) {
+            movedLog.addMovedLog(MOVED_DISTANCE);
+        }
+        if (!canMove) {
+            movedLog.addMovedLog(NOT_MOVED_DISTANCE);
+        }
+    }


이 부분은 아래와 같이 변경해볼 수 있을 것 같아요. :)

Suggested change

public void move() {

boolean canMove = moveConditionStrategy.isMovable();

if (canMove) {

movedLog.addMovedLog(MOVED_DISTANCE);

}

if (!canMove) {

movedLog.addMovedLog(NOT_MOVED_DISTANCE);

}

}

if (canMove) {

movedLog.addMovedLog(MOVED_DISTANCE);

return;

}

movedLog.addMovedLog(NOT_MOVED_DISTANCE);

추가적으로, 이동여부를 판별해서 다르게 기록을 하는 것도 괜찮지만 이동여부 자체를 기록하는 것도 괜찮을 것 같다는 생각이 드네요! 구현은 자유롭게 할 수 있으니, 이 부분은 다양한 방향으로 고민해볼 수 있을 것 같네요! :)

이 부분은 두가지의 장점을 판단하여 구현 하면 좋을 것 같아요 😀
예시가 정확할지 모르겠지만 대강 생각해보면

이동여부를 판별해서 다르게 기록하는 경우 -> 전체 라운드에서 이동여부에 따른 거리가 달라 질 수있는경우(1칸대신 2칸,3칸)? 이동여부보다 거리가 중요하여 거리에 관련된 로직이 많을때

이동여부 자체를 기록하는 경우 -> 이동 여부가 더 중요한 경우? 각 라운드별로 자동차가 이동했는지 안했는지 판단하여 bool 타입을 사용할 로직이 많을때(최종라운드에서 이동하면 추가거리보너스(3칸)를 준다?)
정도로 나눌 수 있지 않을까 싶어요

banjjoknim · 2021-05-24T12:27:41Z

src/main/java/racing/domain/MovedLog.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class MovedLog {


MovedLog에서 Log라는 단어가 갖는 의미가 다소 강한 것 같아요. Log라고 한다면, 모든 기록을 다 기록한다는 의미로 받아들여질 수도 있을 것 같아요. 현재로서는 각각의 Car 객체가 기록을 가지는 형태이므로, 각각의 Car 객체에 대한 기록을 관리하는 역할을 한다는 의미를 드러낼 수 있도록 네이밍을 해보면 어떨까요? :)

확실히 자동차랑 관련이 없어 보이는 네이밍인거 같네요 😢

실패,성공 테스트 분리 및 올바른 접근 제한자 설정

banjjoknim

안녕하세요 재언님!
이번에도 미션을 잘 진행해주셨는데요~ 😁
질문도 많이 해주시고, 열심히 하는 모습이 보기 좋아요! 👍
코멘트를 남겨드렸는데, 확인하시고 미션 진행해주시면 될 것 같아요~~
질문 주신 부분들에 대해서는 DM으로 남겨드렸습니다!
화이팅입니다 ^^!

banjjoknim · 2021-05-25T10:21:03Z

src/main/java/racing/RacingController.java

+        cars = racingGame.getCars();
+        Output.printResultByMovedLog(cars, repeatCount);


Output#printResultByMovedLog()에 RacingGame 객체를 그대로 넘겨주는 건 어떨까요? Model과 데이터가 아주 많아진다고 가정했을때, 이처럼 객체 내부의 값들을 Controller단에서 getter를 통해 모두 꺼내어 View에 넘겨주면 어떤 현상이 나타나게 될지 고민해보면 좋을 것 같아요 :)

컨트롤러에 객체 내부의 값들을 꺼내는 코드가 많아져 컨트롤러가 아주 길어 질 것 같습니다😀

banjjoknim · 2021-05-25T11:35:54Z

src/main/java/racing/view/Input.java

@@ -2,14 +2,18 @@

 import java.util.Scanner;

+import static racing.view.Output.*;


사용되지 않는 import 문이네요! :)

음..? 인텔리제이에서 안쓰는 import 자동 제거 옵션을 넣어 놨는데 안사라졌나 봅니다 ㅠ😅

banjjoknim · 2021-05-25T12:30:13Z

src/main/java/racing/view/Output.java

+
+    public static void printWinMessage(List<Car> winners) {
+        System.out.print(WIN_MESSAGE);
+        System.out.println(winners.stream().map(Car::getName).collect(Collectors.joining(", ")));


stream()을 사용하셨네요! 👍 stream()을 사용하신다면, 아래와 같이 한줄에 하나의 점(.)만 찍도록 해봐요.
가독성이 좋아지고, 참고자료와 같이 타입 힌트에 대한 IDE의 도움도 받을 수 있답니다 :)

Suggested change

System.out.println(winners.stream().map(Car::getName).collect(Collectors.joining(", ")));

String winnerNames = winners.stream()

.map(Car::getName)

.collect(joining(", "));

System.out.println(winnerNames);

한 줄에 있는게 좋아 보였는데 한 줄에 한 점이 더 좋을 듯 하네요

힌트에 관해서는
힌트 설정법도 달라진듯 하고, 새로운 방식으로 설정을 했지만 힌트는 잘 나오진 않네요 ㅠㅠ😅

banjjoknim · 2021-05-25T15:01:50Z

src/test/java/racing/domain/CarTest.java

+    @CsvSource(value = {"aaaaa", "bbbb", "ccc"})
+    @DisplayName("자동차 이름이 전부 5자 이내인 자동차 생성")
+    public void makeCarTest(String name) {
+        assertThat(new Car(name, moveConditionStrategy).getName()).isEqualTo(name);


아래와 같이 예외가 발생하지 않는(자동차 객체가 잘 생성된) 상황에 대한 테스트도 고려할 수 있다는 점도 기억하면 좋을 것 같아요 :)

Suggested change

assertThat(new Car(name, moveConditionStrategy).getName()).isEqualTo(name);

assertDoesNotThrow(() -> new Car(name, moveConditionStrategy));

훨씬 더 직관적이네요 😀👍

banjjoknim · 2021-05-26T04:23:00Z

src/main/java/racing/domain/Car.java

+    public Car(String name, MoveConditionStrategy moveConditionStrategy) {
+        validateLength(name);
+        this.name = name;
+        this.moveConditionStrategy = moveConditionStrategy;
+        this.distanceLog = new CarDistanceLog();
+    }
+
+    public Car(String name, MoveConditionStrategy moveConditionStrategy, int initDistance) {
+        this(name, moveConditionStrategy);
+        this.distanceLog = new CarDistanceLog(initDistance);
+    }


주 생성자를 만들어서 해당 생성자에서만 필드를 초기화해보면 어떨까요? 주 생성자는 일반적으로 생성자 중에서 가장 하단에 배치된답니다.

banjjoknim · 2021-05-26T05:12:44Z

src/main/java/racing/domain/CarDistanceLog.java

+public class CarDistanceLog {
+    private static final int INITIAL_DISTANCE = 0;
+
+    private int finalDistance = INITIAL_DISTANCE;


아래의 CarDistanceLog(int) 생성자를 통해 CarDistanceLog를 초기화해줄 수 있을 것 같아요. 조금 더 적극적으로 생성자를 사용해보면 어떨까요? :)

추가적으로, 객체의 int(primitive) 타입 필드는 객체 생성시 0으로 초기화되기 때문에 지금으로서는 굳이 상수를 추가해서 0으로 초기화해줄 필요는 없을 것 같다는 생각이 들어요. 만약 특정 위치로 초기화하고 싶다면 생성자를 사용하면 되지 않을까요?
또, finalDistance를 INITIAL_DISTANCE 로 초기화하는게 다소 어색한 것 같다는 느낌도 들어요 :) 적절한 변수명을 지어주면 어떨까요? :)

banjjoknim · 2021-05-26T05:36:12Z

src/main/java/racing/domain/CarDistanceLog.java

+    public void addMovedLog(int distance) {
+        this.distanceLogs.add(finalDistance + distance);
+        this.finalDistance += distance;
+    }


클래스 명은 변경되었지만, 메서드 명은 변경되지 않았네요! :)
추가적으로, 초기 위치 + 지금까지 기록된 이동거리가 현재 위치가 되도록 구현해보면 어떨까요?
필드로 선언되어 있는 distanceLogs를 활용해보면 좋을 것 같아요 :)

초기에 만들었던 형태가 distanceLogs를 이용해 만드는 형태 였는데 코드가 더 복잡해 보여 현재 코드로 바꿨습니다.

banjjoknim · 2021-05-26T05:36:27Z

src/main/java/racing/domain/CarDistanceLog.java

+        this.finalDistance += distance;
+    }
+
+    public boolean isWinner(int winnerPosition) {


isWinner()는 CarDistanceLog가 갖고 있는 메서드에는 다소 어색한 이름인 것 같아요. 우승자는 CarDistanceLog가 아닌 Car일테니까요. 조금 더 어울리는 이름을 붙여보면 어떨까요? :)

banjjoknim · 2021-05-26T05:37:46Z

src/main/java/racing/domain/RacingGame.java

+public class RacingGame {
+    public static final String SEPARATOR = ",";
+
+    List<Car> cars;


cars 필드를 동일한 패키지라면 접근할 수 있게 되어있네요! :) private 접근제한자를 설정해보면 어떨까요?

banjjoknim · 2021-05-26T05:39:03Z

src/main/java/racing/domain/RacingGame.java

+        return winnerCars;
+    }
+
+    private int findWinnePosition() {


findWinnePosition() 오타발견!

헛... ide에서 왜 오타를 안잡아 줬을까요 😮

banjjoknim

안녕하세요 재언님 ^^!
그동안 자동차 경주 게임 미션 진행하느라 고생하셨어요. 👍 💯
간단하게 생각해볼 수 있는 것들을 조금 남겨뒀어요.
고민해보시고 어렵다거나, 잘 이해가 안된다거나 하는 것들이 있으면 편하게 DM 주시면 될 것 같아요.
이번 미션은 여기서 Merge하도록 할게요. 앞으로도 화이팅이에요! 😁

banjjoknim · 2021-05-29T05:19:24Z

src/main/java/racing/view/Output.java

+        String winnerNames = winners.stream()
+                .map(Car::getName)
+                .collect(Collectors.joining(", "));


Stream() 사용시 힌트가 나오는 경우는 코드 라인이 4줄 이상일 경우에요. 힌트가 나오는 모습을 보여주기 위해 간단하게 스크린샷을 찍어봤어요 :) (빨간줄은 무시해주세요 ㅎㅎ 힌트를 위해 작성해본거니까요!)

banjjoknim · 2021-05-29T05:42:55Z

src/main/java/racing/domain/Car.java

+    private MoveConditionStrategy moveConditionStrategy;
+
+    public Car(String name, MoveConditionStrategy moveConditionStrategy) {
+        this(name, moveConditionStrategy, 0);


0이 의미하는게 어떤건지 알 수 있도록 해보면 어떨까요? :)

banjjoknim · 2021-05-29T05:43:54Z

src/main/java/racing/domain/Car.java

+    private int createMovedDistance() {
+        boolean canMove = moveConditionStrategy.isMovable();


이동거리를 만들 때, 메서드 내부에서 자동차가 가진 전략을 통해 canMove를 판단하는 대신, 자동차의 외부에서 메서드에게 전략을 전달해보면 어떨까요? 그러면 어떤 효과가 있을까요? :)

banjjoknim · 2021-05-29T05:44:21Z

src/main/java/racing/domain/DistanceLog.java

+    public DistanceLog() {
+    }


Car에서 DistanceLog(int) 생성자를 사용하여 DistanceLog를 생성하고 있어요. 기본 생성자는 사용되지 않는 것 같네요! 테스트에서만 사용되는 코드가 프로덕션 코드에 존재하는 것 같다는 생각이 들어요 :)

banjjoknim · 2021-05-29T05:44:32Z

src/main/java/racing/domain/DistanceLog.java

+    public void addDistanceLog(int distance) {
+        this.distanceLogs.add(this.distance + distance);
+        this.distance += distance;
+    }


이 부분은 분리해볼 수 있을 것 같아요. Log를 추가하는 기능과 Log의 distance를 계산하는(1을 더하거나, 위치를 계산해서 반환하거나) 기능으로 분리해보면 어떨까요? 그러면 초기에 복잡해보였던 코드도 개선의 가능성이 있을 것 같다는 생각이 드네요! 물론 특정 시점의 위치를 구하기위해 매번 계산해야하는 번거로움은 존재하겠지만, 새로운 Log를 추가할 때마다 현재도 계산하고 있으니 이 부분은 비슷할 것 같다는 생각이 들어요 :)

banjjoknim · 2021-05-29T05:47:48Z

src/main/java/racing/domain/NumberIsBiggerStrategy.java

+
+import racing.utils.RandomNumber;
+
+public class NumberIsBiggerStrategy implements MoveConditionStrategy {


NumberIsBiggerStrategy에서 가장 핵심은 랜덤한 번호인 것 같아요. 그렇다면 RandomMoveConditionStrategy와 같은 이름을 고려해볼 수 있을 것 같아요 :)

banjjoknim · 2021-05-29T05:48:22Z

src/main/java/racing/domain/RacingGame.java

+        }
+    }
+
+    public List<Car> findWinner() {


조금 사소한 부분일 수도 있지만, List 타입을 반환하고 있으므로 findWinner()와 같은 단수형보다는 findWinners()와 같이 복수형을 사용하는게 좋아보여요 :)

banjjoknim · 2021-05-29T05:49:21Z

src/test/java/racing/domain/DistanceLogTest.java

+import static org.junit.jupiter.api.Assertions.assertAll;
+
+public class DistanceLogTest {
+    DistanceLog distanceLog = new DistanceLog();


여기서도 역시, 적절한 접근 제한자를 설정해주면 좋을 것 같아요 의식적으로 접근 제한자를 고려하도록 해봐요 :) (CarTest도 확인이 필요할 것 같아요!)

banjjoknim · 2021-05-29T05:50:36Z

src/main/java/racing/domain/MoveConditionStrategy.java

@@ -0,0 +1,5 @@
+package racing.domain;
+
+public interface MoveConditionStrategy {


이 부분에 관하여, 일급 시민 또는 일급 객체에 대해서 찾아보고 공부해보면 좋을 것 같네요 :)

Miot2J added 4 commits May 10, 2021 12:03

refactor: Pattern 내부에서 반복되는 코드 개선

2e22008

Pattern.matches() 사용시 내부적으로 Pattern을 생성해 성능을 위해 외부에서 pattern 미리 선언 후 match만 호출.

refactor: 문자열 끼리 + 연산을 StringBuilder 를 통해 속도 개선, 객체의 책임 명확화

63d1db7

객체의 책임을 다른 클래스에서 가지고있던 부분을 리팩토링함.

refactor: TestCode 수정 및 추가 완료

42f061e

refactor: MVC 패턴에 맞게 패키지 및 클래스 변경

70a20da

Miot2J changed the title ~~[차재언] Step2 구현 완료 했습니다.~~ [차재언] Step2를 MVC 적용하여 구현 완료 했습니다. May 10, 2021

banjjoknim requested changes May 11, 2021

View reviewed changes

refactor: RacingGame 에 List<Car> 객체 선언, Test 구조 개선

dd1a3f2

List<Car> 를 선언하여, RacingGame의 메서드들의 파라미터와 리턴값을 제거하고 list를 매번 새로 생성하는 코드를 제거 하였음

banjjoknim requested changes May 13, 2021

View reviewed changes

Miot2J added 3 commits May 15, 2021 21:52

refactor: RacingGame 생성자로 List<Car> 객체 할당, 반환타입에 방어적 복사기법 적용

9ae509e

refactor: 출력시 stringBuilder를 통해 한번에 문장을 완성후 한번만 sys 출력하도록 변경

6eb0e4b

refactor: @MethodSource 사용해 MathSymbol검증, racing 패키지의 출력 수정

3b2b556

racing 패키지의 정적 출력부분을 Input에서 동작하도록 변경

Miot2J changed the title ~~[차재언] Step2를 MVC 적용하여 구현 완료 했습니다.~~ [차재언] Step2 피드백 반영 했습니다. May 15, 2021

refactor: MathSymbol에 abstract 메소드를 통해 각각 다른 계산결과 오버라이딩

99d47f2

banjjoknim requested changes May 18, 2021

View reviewed changes

Miot2J changed the title ~~[차재언] Step2 피드백 반영 했습니다.~~ [차재언] Step2(RacingGame) 에 전략패턴을 반영 했습니다. May 18, 2021

refactor: CarTest#isWinner에 DisplayName 추가

0f31cd8

banjjoknim requested changes May 20, 2021

View reviewed changes

Miot2J added 6 commits May 23, 2021 22:52

refactor: Car 객체는 자동차가 움직일 조건을 알 필요가 없기에 조건을 가지고 있는 책임 제거

4e26208

refactor: Car 객체에 MovedLog를 추가해 이동 기록을 관리하도록 함

e958b65

refactor: RacingGame 메소드의 불필요한 파라미터 제거

65a85e8

refactor: String.join을 사용하여 불필요한 상수, builder 제거

42f7540

refactor: StringBuilder.append() 를 메서드 체이닝형식으로 재 구성

e458649

refactor: 불필요한 TestCode 제거 및 변경

aa26c82

Miot2J changed the title ~~[차재언] Step2(RacingGame) 에 전략패턴을 반영 했습니다.~~ [차재언] Step2(RacingGame) 에 MovedLog 객체 추가. May 23, 2021

Miot2J added 5 commits May 24, 2021 15:10

refactor: Output#printWinMessage() 메소드에 스트림 적용

4266ffc

refactor: Car#move() 에 반환타입 제거

bbcc308

테스트 메소드를 위해 boolean 반환이 필요했지만 @FunctionalInterface 사용하여 테스트 코드에서 람다식으로 boolean형 리턴값을 고정시켜 사용

refactor: 변경된 변수명 적용

522fdc5

refactor: MovedLog에 출발지점이 다른 생성자 추가

03a02f5

refactor: method 호출 순서에 따라 재배치

9087550

refactor: if의 조건이 boolean 이고, 조건의 결과를 그대로 반환하는 코드의 조건문 제거

d95aceb

banjjoknim requested changes May 24, 2021

View reviewed changes

banjjoknim reviewed May 24, 2021

View reviewed changes

Miot2J added 8 commits May 25, 2021 02:10

refactor: boolean 타입의 변수명은 긍정형으로 짓기!

df15861

refactor: 가독성을 위해 static import를 제거하고 Output을 명시

7bc194d

refactor: Car가 movedLog의 finalDistance를 알 필요가 없어 책임 분리

903c176

refactor: 종속함수의 함수 호출순서에 맞게 변경

041c4fb

refactor: 네이밍 변경

41892ff

refactor: 테스트 코드에 대한 피드백 반영

6dabc61

실패,성공 테스트 분리 및 올바른 접근 제한자 설정

refactor: MovedLog -> CarDistanceLog로 변경

c589fb5

refactor: ReformatCode 기능 사용

80a2215

Miot2J changed the title ~~[차재언] Step2(RacingGame) 에 MovedLog 객체 추가.~~ [차재언] Step2(RacingGame) 피드백 반영. May 24, 2021

banjjoknim requested changes May 26, 2021

View reviewed changes

Miot2J added 4 commits May 27, 2021 21:01

refactor: Car 생성자 변경,

20af7d5

refactor: get 사용해서 할당하는 코드 제거

4ed7d98

refactor: DistanceLog 피드백 반영

30901e1

refactor: RacingGame의 cars 접근 제어자 변경,메서드 오타 수정

a699d5a

banjjoknim approved these changes May 29, 2021

View reviewed changes

banjjoknim merged commit caf5e6d into next-step:miot2j May 29, 2021

		@@ -54,13 +59,15 @@ private void validate(String mathExpression) {
		}

		private void isTheEndNotNumber(String mathExpression) {

		@@ -11,6 +13,6 @@ public void makeNumber() {
		//then
		int number1 = randomNumber.makeOneRandomNumber();


		import static racing.utils.RandomNumber.makeOneRandomNumber;

		public class RacingGame {

	if (PLUS.mathSymbol == this.getMathSymbol()) {
	if (PLUS.mathSymbol == this.mathSymbol) {

		cars = racingGame.getCars();
		Output.printResultByMovedLog(cars, repeatCount);

		@@ -2,14 +2,18 @@

		import java.util.Scanner;

		import static racing.view.Output.*;

	assertThat(new Car(name, moveConditionStrategy).getName()).isEqualTo(name);
	assertDoesNotThrow(() -> new Car(name, moveConditionStrategy));

		private int createMovedDistance() {
		boolean canMove = moveConditionStrategy.isMovable();


		import racing.utils.RandomNumber;

		public class NumberIsBiggerStrategy implements MoveConditionStrategy {

		@@ -0,0 +1,5 @@
		package racing.domain;

		public interface MoveConditionStrategy {

[차재언] Step2(RacingGame) 피드백 반영. #2204

[차재언] Step2(RacingGame) 피드백 반영. #2204

Conversation

Miot2J commented May 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

banjjoknim May 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banjjoknim May 13, 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

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

banjjoknim May 18, 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

banjjoknim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banjjoknim May 20, 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

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 left a comment

Choose a reason for hiding this comment

banjjoknim May 24, 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

banjjoknim May 24, 2021 • edited

Choose a reason for hiding this comment

Miot2J commented May 10, 2021 •

edited

banjjoknim May 13, 2021 •

edited

banjjoknim May 13, 2021 •

edited

banjjoknim May 18, 2021 •

edited

banjjoknim May 20, 2021 •

edited

banjjoknim May 24, 2021 •

edited

banjjoknim May 24, 2021 •

edited