[2단계 - 자동차 경주 리팩토링] 연로그(권시연) 미션 제출합니다. #419

yeon-06 · 2022-02-19T08:16:40Z

안녕하세요 카일! 2단계 미션 제출합니다~!

2단계 요구사항

핵심 비지니스 로직을 가지는 객체를 domain 패키지, UI 관련한 객체를 view 패키지에 구현한다.
MVC 패턴 기반으로 리팩토링해 view 패키지의 객체가 domain 패키지 객체에 의존할 수 있지만, domain 패키지의 객체는 view 패키지 객체에 의존하지 않도록 구현한다.
테스트 가능한 부분과 테스트하기 힘든 부분을 분리해 테스트 가능한 부분에 대해서만 단위 테스트를 진행한다.

개선사항

패키지, 클래스, 메소드명 더 적절하게 변경
세부 개선사항은 코멘트로 추가

고민중

Car - go()는 기준 값보다 큰 경우에 움직입니다! 다만 '기준 값보다 크다'라는 조건이 Car에게 책임이 있는가, 아니면 Car의 상태를 변하는 조건이 주어지는 Cars에게 책임이 있는가에 대해 헷갈립니다.
TryCount에서 Integer.parseInt()가 실패하면 IllegalArgumentException을 throw하도록 처리해두었습니다. 원래는 문자열이 입력될 경우를 생각하며 '숫자를 입력해주세요'라는 에러 문구를 써두었는데요. 2147483648가 입력되면 int 범위를 넘어서서 위 에러가 발생합니다. 이 경우 int 2147483648 미만의 숫자만 입력 가능합니다 식으로 에러 문구를 바꿔야 할까요? 이런 경우까지 생각하는건 오버 프로그래밍을 야기할까요?

yeon-06 · 2022-02-19T08:17:39Z

src/main/java/racingcar/util/StringUtils.java

@@ -11,8 +11,9 @@
    public static String[] stripStringArray(String[] array) {
        int length = array.length;
        String[] copyArr = new String[length];
-        IntStream.range(0, length).forEach(i -> copyArr[i] = array[i].strip());
+        for (int i = 0; i < length; i++) {


StringUtils - stripStringArray()에서 불필요한 forEach 삭제
👉 굳이 Stream 처리를 해가며 forEach() 사용할 필요 없이 for문으로 처리하는게 더 간단하다고 느낌

이 글을 보면 항상 Stream이 좋은 건 아니라는 것을 알 수 있죠🙂 개인적으로 현재는, 스트림을 통한 성능적인 이슈보단 현재 연로그가 고민하는 것처럼 가독성적인 측면을 고려하는 것이 바람직할 것 같아요.

다만 이런 부분은 항상 같이 고민하면 좋을 것 같아요. 스트림을 처음 쓰거나 낯선사람에게는 당연하게도 for문이 익숙하고 가독성이 높을거에요. 즉 새로운 기술은 언제나 처음은 낯설기에 일정 기간 적응하고 난 뒤에도 가독성이 떨어지는가? 를 기준으로 판단해나가면 좋을 것 같아요.

yeon-06 · 2022-02-19T08:21:08Z

src/main/java/racingcar/util/RandomUtils.java

@@ -14,6 +15,6 @@ public static int generateNumber(int min, int max) {
        if (min == max) {
            return min;
        }
-        return min + random.nextInt(max - min);
+        return min + ThreadLocalRandom.current().nextInt(max - min);


Random 대신 ThreadLocalRandom 사용
👉 thread-safe한 클래스를 사용하고자 시도

yeon-06 · 2022-02-19T08:22:14Z

src/main/java/racingcar/controller/RacingController.java

 import racingcar.view.InputView;
 import racingcar.view.OutputView;

 public class RacingController {
-    private Cars cars;
-    private TryCount tryCount;


RacingController에서 멤버 변수를 지역 변수로 변경
👉 Controller에서 상태를 가질 여지를 없앰

Controller에서 상태를 없애야겠다고 판단한 이유가 있을까요?

만약에 Controller를 여러 곳에서 접근하는 경우 cars와 tryCount가 원하지 않는 상태를 갖거나 수정될 수 있다고 판단하였습니다!

좋은 관점인 것 같아요🙂 다만 현재는 한 사용자가 요청할 때마다, RacingController를 생성해서 사용하는 구조인데 해당 값을 공유하는 케이스가 있을까요?

현재는 없습니다! 미리 습관 들여두면 좋을 것 같아서 시도해보았습니다 ㅎㅎ

yeon-06 · 2022-02-19T08:23:52Z

src/test/java/racingcar/domain/CarsTest.java

 import org.junit.jupiter.api.Test;

-@SuppressWarnings("NonAsciiCharacters")


@SuppressWarnings 대신 @DisplayName 사용
👉 한글 메소드명 외로도 해당 오류가 발생하는 경우가 있으면 어떡하지? 라는 생각이 들어서 강제로 오류를 무시하는게 꺼려짐. @DisplayName을 사용하는 방향으로 결정

좋은 방향이네요🙂 다만 위에서 한글인 경우에 대한 워닝을 무시한다고 명시하셨는데, 한글 메소드명 외에 해당 오류가 발생한다라는 케이스가 있을까요?

DisplayName을 사용하는 것과 한글 메소드를 사용하는 것에 대한 기준은 주관적이지만 DisplayName을 사용하기로 한 이유가 조금 이해가 덜 되어서요!

제가 겪지는 않았지만 추후에 발생 가능성이 있지 않을까 불안함이 있었습니다 😂
자주 보는 에러가 아니다보니 어디서 발생할지 예상이 안되더라고요💦

yeon-06 · 2022-02-19T08:26:53Z

src/main/java/racingcar/util/RandomUtils.java

-public class RandomGenerator {
-    private static final Random random = new Random();
+public class RandomUtils {
+    private RandomUtils() {


util 관련 클래스에서 private 생성자 생성
👉 모든 메소드가 static + 상태가 없고 앞으로도 가지면 안된다고 판단해서 명시적으로 생성자를 막음

사소한 부분이지만 좋은 습관인 것 같아요🙂 실제로 나중에 사용하실 Lombok에서도 유틸클래스에서 사용하는 어노테이션은 퍼블릭 생성자를 못 만들도록 되어 있습니다.👏

KIMSIYOUNG

안녕하세요 연로그!
기존에도 모델과 뷰가 잘 분리되어 있어서 큰 변경사항은 없네요🙂
조금 더 고민해보면 좋을 것들과, 추가 설명해주셨으면 하는 부분들에 대해서 리뷰 달아두었습니다. 확인 부탁드려요. (ps 깃 고생하셨습니다..ㅎㅎ)

Car - go()는 기준 값보다 큰 경우에 움직입니다! 다만 '기준 값보다 크다'라는 조건이 Car에게 책임이 있는가, 아니면 Car의 상태를 변하는 조건이 주어지는 Cars에게 책임이 있는가에 대해 헷갈립니다.

요 부분은 코멘트에 추가해두었습니다.

TryCount에서 Integer.parseInt()가 실패하면 IllegalArgumentException을 throw하도록 처리해두었습니다. 원래는 문자열이 입력될 경우를 생각하며 '숫자를 입력해주세요'라는 에러 문구를 써두었는데요. 2147483648가 입력되면 int 범위를 넘어서서 위 에러가 발생합니다. 이 경우 int 2147483648 미만의 숫자만 입력 가능합니다 식으로 에러 문구를 바꿔야 할까요? 이런 경우까지 생각하는건 오버 프로그래밍을 야기할까요?

이 부분도 연로그가 고민하신 부분의 결정이 답이 될 것 같아요. 방어적인 형태로 코드를 작성하는 것은 매우 좋은 습관이라고 생각합니다. 21억건 이상의 입력을 방지하기 위해 long을 사용할 수도 있겠지만, 저라면 21억건 이상의 시도횟수는 프로그램에 대한 부적절한 시도라고 생각해서 다른 예외를 반환할 것 같아요. 인트의 최대값을 초과하는 입력에 대해서 접근할 때도 단순히 수를 늘리고 줄이는 것 보단, 21억 이상의 값이 입력되는 것이 프로그램에 어떤 의미인지 고민해보면 좋을 것 같아요. 추가로 그 숫자가 들어와도 정상동작해야한다면 long타입을 쓰겠지만 그 수가 들어왔을 때 예외를 뱉는다면 연로그가 말씀주신 예외보단 조금 더 의미있는 예외가 있을 것 같아요🙂

KIMSIYOUNG · 2022-02-20T06:19:49Z

README.md

 - 주어진 횟수 동안 n대의 자동차는 전진 또는 멈출 수 있다.
 - 각 자동차에 이름을 부여할 수 있다. 전진하는 자동차를 출력할 때 자동차 이름을 같이 출력한다.
 - 자동차 이름은 쉼표(,)를 기준으로 구분하며 이름은 5자 이하만 가능하다.
 - 사용자는 몇 번의 이동을 할 것인지를 입력할 수 있어야 한다.
 - 전진하는 조건은 0에서 9 사이에서 random 값을 구한 후 random 값이 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다.
 - 자동차 경주 게임을 완료한 후 누가 우승했는지를 알려준다. 우승자는 한 명 이상일 수 있다.

+## step2 요구사항
+- MVC 패턴 기반으로 리팩토링


코드는 요구사항이 변경됨에 따라 항상 최신화되지만 문서는 그렇지 않은 경향이 많아요. (안 바꿔도 티가 안 나니까..) 그렇지만 연로그는 같이 최신화해주셨네요👏

KIMSIYOUNG · 2022-02-20T06:21:07Z

src/main/java/racingcar/controller/RacingController.java

 import racingcar.view.InputView;
 import racingcar.view.OutputView;

 public class RacingController {
-    private Cars cars;
-    private TryCount tryCount;


Controller에서 상태를 없애야겠다고 판단한 이유가 있을까요?

KIMSIYOUNG · 2022-02-20T06:22:08Z

src/main/java/racingcar/controller/RacingController.java

@@ -39,7 +37,7 @@ private void race() {
        }
    }

-    private void terminate() {
+    private void terminate(Cars cars) {


프로그램을 종료한다라는 의미로 사용한 이 메소드에서 차들의 상태를 표현하고, 우승자를 보여주고 있어요.

프로그램을 잘 알고 있는 연로그에게는 종료 == 위 두가지 역할이 매핑될 수 있지만, 코드를 처음 보는 저에게도 조금 더 친숙한 형태로 네이밍 / 역할을 변경해볼까요?

KIMSIYOUNG · 2022-02-20T06:26:34Z

src/main/java/racingcar/domain/Car.java

@@ -12,7 +12,7 @@ public Car(String nameString) {
        this.position = new Position();
    }

-    public void goOrStop(int random) {
+    public void go(int random) {


현재는 자동차가 간다 안간다의 기준을 알고, 그를 통해 움직이는 행위를 결정하고 있는데요. 이런 경우에는 go라는 네이밍이 적절하지 않은 것 같아요. 해당 메소드를 사용하는 사람은 차가 갈 것이라고 예상하지만 움직이지 않는 경우도 분명 존재하니까요.

연로그의 고민처럼 자동차가 움직이는 조건이 외부에 있다면 가는 경우에만 go를 호출하는 건 자연스럽지만, 현재는 판단 기준이 내부에 있어서 네이밍이 조금은 부적절한 것 같아요.

이어서 기준값보다 크다의 판단기준은 Car인가 Cars인가에 대한 고민을 조금 더 해보면 좋을 것 같아요. 각각에 있을 수 있다라고 판단한 연로그의 이유를 조금 더 공유해주실 수 있나요?

추가로 이 부분을 전략패턴을 사용해서 수정해보는건 어떨까요? 완벽하게 이해하고 넘어가야한다기보단, 한 번정도 사용해보며 이런 식으로 해결할 수도 있구나라고 경험해보시면 좋을 것 같아요.

전략 패턴을 사용한다면 4라는 기준값은 어디에 들어갈지도 같이 고민해볼 수 있을 것 같아요.

goOrStop 이라는 이름이 볼때마다 이거 두가지 기능을 하는건가? 하고 자꾸 다시 보게 되어서 변경했는데 좀 더 생각해보겠습니다ㅠ_ㅠ)!!

전략 패턴 형식으로 수정했습니다!
랜덤 값이 Car도 Cars도 아닌 MoveStrategy로 이동되니 테스트도 편하고 책임도 분리돼서 기능이 적절히 분리됐다는 생각이 드네요😆
다만 한가지 의문점이 테스트를 위해 FixMoveStrategy라는 클래스를 생성했는데 괜찮은걸까요?
해당 클래스 자체는 나중에 랜덤값이 아닌 고정 값을 전달하고 싶은 경우 사용할 수 있으니 재사용의 가능 여부는 충분하지만 이런 식으로 '테스트를 위한 클래스'이 늘어나도 괜찮은건지에 대한 고민이 생겼습니다

KIMSIYOUNG · 2022-02-20T06:27:59Z

src/main/java/racingcar/domain/TryCount.java

@@ -16,6 +16,20 @@ public boolean isNotSame(int tryCount) {
        return this.tryCount != tryCount;
    }

+    private int convertStringToInt(String string) {


메소드 위치를 수정해주셨네요🙂 어떤 이유로 프라이빗 메소드가 Equals/HashCode 보다 위에 있는 것이 낫다고 판단하셨는지 공유해주실 수 있으실까요?

equals, hashcode는 재정의하지 않아도 기본적으로 선언되어 있고 제가 재정의 하였으나 별도로 특별한 로직을 포함한 것이 아니기 때문입니다! 그래서 다른 메소드에 비해 중요도가 떨어진다고 생각하였습니다

KIMSIYOUNG · 2022-02-20T06:36:31Z

src/main/java/racingcar/util/StringUtils.java

@@ -11,8 +11,9 @@
    public static String[] stripStringArray(String[] array) {
        int length = array.length;
        String[] copyArr = new String[length];
-        IntStream.range(0, length).forEach(i -> copyArr[i] = array[i].strip());
+        for (int i = 0; i < length; i++) {


이 글을 보면 항상 Stream이 좋은 건 아니라는 것을 알 수 있죠🙂 개인적으로 현재는, 스트림을 통한 성능적인 이슈보단 현재 연로그가 고민하는 것처럼 가독성적인 측면을 고려하는 것이 바람직할 것 같아요.

다만 이런 부분은 항상 같이 고민하면 좋을 것 같아요. 스트림을 처음 쓰거나 낯선사람에게는 당연하게도 for문이 익숙하고 가독성이 높을거에요. 즉 새로운 기술은 언제나 처음은 낯설기에 일정 기간 적응하고 난 뒤에도 가독성이 떨어지는가? 를 기준으로 판단해나가면 좋을 것 같아요.

KIMSIYOUNG · 2022-02-20T06:37:40Z

src/main/java/racingcar/domain/Cars.java

@@ -45,4 +45,11 @@ private void validateDuplicatedName(String[] carNames) {
            throw new IllegalArgumentException(ErrorMessages.DUPLICATED_NAME);
        }
    }
+
+    @Override
+    public String toString() {


모든 도메인 객체에 toString을 오버라이드 한 이유가 있나요?

toString()에 대해서 공부를 했는데 디버깅할 때 편리하고 (아직 저희 과정에서는 불필요하지만) 로그를 찍을때 자주 사용되는 것 같았습니다!
미리 생성해두는 습관을 길러두면 좋을 것 같아 선언했습니다!

KIMSIYOUNG · 2022-02-20T06:39:19Z

src/main/java/racingcar/util/RandomUtils.java

-public class RandomGenerator {
-    private static final Random random = new Random();
+public class RandomUtils {
+    private RandomUtils() {


사소한 부분이지만 좋은 습관인 것 같아요🙂 실제로 나중에 사용하실 Lombok에서도 유틸클래스에서 사용하는 어노테이션은 퍼블릭 생성자를 못 만들도록 되어 있습니다.👏

KIMSIYOUNG · 2022-02-20T06:40:14Z

src/main/java/racingcar/domain/Car.java

@@ -1,4 +1,4 @@
-package racingcar.model;
+package racingcar.domain;

 public class Car {


기존에 패키지명을 Model 이라고 사용하셨는데 Domain이라고 수정해주신 이유가 있을까요?

요구사항에 '핵심 비지니스 로직을 가지는 객체를 domain 패키지에 구현한다'라고 되어있어서 이름을 변경했습니다!

KIMSIYOUNG · 2022-02-20T06:41:44Z

src/test/java/racingcar/domain/CarsTest.java

 import org.junit.jupiter.api.Test;

-@SuppressWarnings("NonAsciiCharacters")


좋은 방향이네요🙂 다만 위에서 한글인 경우에 대한 워닝을 무시한다고 명시하셨는데, 한글 메소드명 외에 해당 오류가 발생한다라는 케이스가 있을까요?

DisplayName을 사용하는 것과 한글 메소드를 사용하는 것에 대한 기준은 주관적이지만 DisplayName을 사용하기로 한 이유가 조금 이해가 덜 되어서요!

yeon-06 · 2022-02-21T07:06:57Z

src/main/java/racingcar/view/InputView.java

    }

    public static String inputTryCount() {
        System.out.println(NoticeMessages.INPUT_TRY_CNT);
-        return scanner.next();
+        return scanner.nextLine();


이 부분을 수정을 했었는데 PR 충돌 해결하면서 코드가 다시 바뀌었던 것 같아요

사용자가 직접 입력하는 값도 테스트를 할 수 있나요?
할 수 있다면 어떤 방법을 사용할까요?
처음엔 전략 패턴을 적용시키면 가능할까해서 만들어보다가 그냥 문자열에 대한 테스트가 될뿐 사용자가 의도한대로 입력이 되었는지에 대한 테스트가 아닌 것 같아서 롤백했습니다.😭

연로그가 이끌어낸 결론이 제가 생각한 질문이었어요🙂

사용자가 직접 입력하는 것은 우리가 제어할 수 있는 대상이 아니니 테스트 대상이 아니라고 판단하는게 맞을 것 같아요. 그렇다면 우리가 제어할 수 없는 사용자 입력을 어떻게 제어할 수 있는 범위로 넣을 것인가?를 고민했을 때 전략패턴으로 이를 해결할 수 있구요.

다만 연로그가 말한 것 처럼 사용자가입력한 값 == 문자열에 대한 테스트가 될 확률이 높겠죠? 그렇기에 불필요하다면 추가하지 않는 것도 방법이에요.

조금 더 나아가서 프로그램 전체가 올바르게 동작한다 즉 레이싱컨트롤러.start()를 했을 때 전체 프로그램이 동작한다를 증명하려면 입력에 대한 전략패턴이 분명 필요해질 수 있겠죠? 어떤 것을 테스트할 것인가에 따라 전략패턴도, 다른 기법들도 사용될지 아닐지가 달라질 수 있다는 점까지 염두해두면 더 좋을 것 같습니다🙂

yeon-06 · 2022-02-21T08:18:50Z

src/main/java/racingcar/domain/TryCount.java

    private final int tryCount;

    public TryCount(String countString) {
+        validateStringIsNumber(countString);
+        isValidNumber(countString);


2147483648와 관련해서는 아예 최대 입력 가능 숫자에 대해 제한을 두었습니다!
숫자인지 판단 -> 유효한 범위인지 판단 -> 필드 값 초기화는 순서를 두었습니다

KIMSIYOUNG

안녕하세요 연로그!
미션을 진행하면서 연로그가 고민하고 생각했던 것들이 코드로 드러나고 계속해서 발전하는 모습이 잘 드러났던 것 같아요. 연로그도 동일하게 느끼셨나요?
이번 미션에서 꼭 고민해보고 갔으면 하는 것들에 대해서는 말씀드렸고 잘 반영해주신 것 같아요.
추가로 조금 더 나아가서 고민해보면 좋을 것들도 몇가지 리뷰 남겼는데 이 부분은 디엠이나 게더로 더 얘기해봐도 좋을 것 같아요.
첫 미션 하느라 고생 많으셨고 앞으로의 미션도 화이팅입니다🙂

KIMSIYOUNG · 2022-02-22T01:53:06Z

src/main/java/racingcar/controller/RacingController.java

-    private Cars cars;
-    private TryCount tryCount;
+    public void start(MoveStrategy moveStrategy) {
+        Cars cars = inputCars();


프로그램의 흐름도가 너무 깔끔하네요🙂

KIMSIYOUNG · 2022-02-22T01:54:06Z

src/main/java/racingcar/controller/RacingController.java

        int nowTryCnt = 0;
        OutputView.printStartMessage();
        while (tryCount.isNotSame(nowTryCnt++)) {
-            cars.moveAll();
+            cars.moveAll(moveStrategy);
            OutputView.printCarsStatus(cars.getCars());


도메인과 뷰의 분리에서 조금 더 나아가서, 뷰는 도메인을 알지 못한다는 규칙도 존재하는데요. 현재는 Car라는 도메인을 직접 알고 있어요. 요 부분의 의존성을 약화시킬 수 있는 혹은 의존하지 않고도 출력할 수 있는 다른 방법도 고민해보면 좋을 것 같아요.

뷰를 위한 dto를 생성한다에 대해서도 생각해보았는데요
자동차 경주에서는 오히려 오버 프로그래밍이 되지 않을까하여 도입하지 않았습니다!
혹시 다른 방법도 있을까요?

KIMSIYOUNG · 2022-02-22T02:07:51Z

src/main/java/racingcar/domain/strategy/RandomMoveStrategy.java

+
+    @Override
+    public int move() {
+        int random = RandomUtils.generateNumber(MOVE_MIN, MOVE_MAX + 1);


전략패턴을 잘 사용해주셨네요👏

조금 더 나아가서 현재 해당 전략들은 관점에 따라 아래와 같은 2가지 일을 하고 있는데요. 이 두가지 모두가 해당 전략의 역할로 간주했을 때와, 움직이는 범위는 자동차가 결정하는 것 중 현재 방법을 택한 이유가 있을까요?

움직일지 아닐지를 판단한다.

실제로 움직이는 양(현재 1 또는 0)을 결정하여 반환한다

Car에서 move할 거리를 받고 바로 움직이고 싶어서 Strategy에 책임을 위임했습니다!
움직일지 아닐지만 판단하는 기능만 하면 기존 로직이랑 크게 다른게 없지 않나? 라는 생각이 들었습니다

KIMSIYOUNG · 2022-02-22T02:14:37Z

src/main/java/racingcar/domain/strategy/FixMoveStrategy.java

@@ -0,0 +1,20 @@
+package racingcar.domain.strategy;
+
+public class FixMoveStrategy implements MoveStrategy {


요 질문에 대한 제 생각은 요기 적으면 좋을 것 같아요.

해당 클래스 자체는 나중에 랜덤값이 아닌 고정 값을 전달하고 싶은 경우 사용할 수 있으니 재사용의 가능 여부는 충분하지만 이런 식으로 '테스트를 위한 클래스'이 늘어나도 괜찮은건지에 대한 고민

테스트를 위한 클래스라고 간주된다면 테스트코드 패키지에 추가하는 것이 맞을 것 같아요🙂 재사용의 가능 여부가 충분하더라도, 그게 확실하지 않다면 프로덕션코드에 구현하는 것은 오히려 혼란을 가중하는 역효과가 있다고 생각합니다.

추가로 람다를 이용해서 구현체를 만들지 않고 테스트에서 사용할 수도 있습니다! (람다와 FunctionalInterface 라는 키워드로 검색해보시면 좋을 것 같아요.)

KIMSIYOUNG · 2022-02-22T02:28:01Z

src/main/java/racingcar/domain/Winners.java

-    public int getMaxPosition() {
-        return maxPosition;
-    }
-
    public List<String> getNames() {


Winners 라는 객체는 자동차들의 목록을 받아서 우승자를 선별하는 역할을 담당하고 있는데요. 이름을 모아서 출력하는 역할은 해당 객체가 자동차를 가지고 있기에 할 수 있는 역할인건 맞지만 도메인 로직에 가까운지 뷰로직에 가까운지 고민해보면 좋을 것 같아요.

제가 생각했을 땐 이 로직은 뷰에 가까운 로직이고, 그렇다면 상태를 가지고 있는 객체라 하더라도 해당 객체의 역할인가라고 했을 때는 아니라고 생각이 들어요.

그럼 이 객체정보 (우승자의 이름들을) 뷰에 전달하기 위해선 어떤 형태로 리팩토링할 수 있을까요? DTO라는 개념을 도입해서 리팩토링해봐도 좋을 것 같아요.

KIMSIYOUNG · 2022-02-22T02:30:30Z

src/main/java/racingcar/domain/Winners.java


    public Winners(List<Car> cars) {
-        maxPosition = getMaxPosition(cars);
+        int maxPosition = getMaxPosition(cars);


매번 리스트에서 계산하는게 아닌, 담아두고 값을 재활용한다 말씀해주신 부분도 충분히 고민 포인트라고 생각해요🙂

분명 데이터가 많고 계산연산이 복잡하다면 그 값을 저장해두는 것이 성능상의 이점을 가져갈 수 있을거에요. 다만 현재시점에서는 그 부분보다는 객체지향적으로 옳은 코드가 무엇인가에 대해서 더 고민하고 그 방향으로 개발을 이어가면 좋을 것 같고, 성능상의 이슈가 발생할 여지가 보일 때 혹은 미묘하게 발생할 때 리팩토링으로 변경하는 것도 좋을 것 같습니다.

KIMSIYOUNG · 2022-02-22T02:31:14Z

src/main/java/racingcar/view/OutputView.java

-    public static void printString(String string) {
-        System.out.println(string);
-    }
-
    public static void printWinners(Winners winners) {
        System.out.println(String.join(DELIMITER, winners.getNames()) + WINNER_SENTENCE);


뷰와 도메인간의 결합을 막기 위해 DTO라는 개념을 도입해보면 좋을 것 같아요🙂

KIMSIYOUNG · 2022-02-22T02:31:21Z

src/main/java/racingcar/view/OutputView.java

        }
        System.out.println();
    }

-    private static String getCarStatus(Car car) {
-        String carPositionString = getGaugeBar(car.getPosition());
+    private static String generateCarStatus(Car car) {


KIMSIYOUNG · 2022-02-22T02:33:45Z

src/main/java/racingcar/domain/Cars.java


 public class Cars {
    private static final String DELIMITER = ",";
-    private static final int MIN = 0;
-    private static final int MAX = 9;
+

    private final List<Car> cars = new ArrayList<>();


미션의 범위에는 벗어나지만 조금 더 공부를 원하시면 불변객체와 방어적 복사라는 키워드로 조금 더 공부해보셔도 좋을 것 같습니다🙂

KIMSIYOUNG · 2022-02-22T02:37:07Z

src/main/java/racingcar/view/InputView.java

    }

    public static String inputTryCount() {
        System.out.println(NoticeMessages.INPUT_TRY_CNT);
-        return scanner.next();
+        return scanner.nextLine();


연로그가 이끌어낸 결론이 제가 생각한 질문이었어요🙂

사용자가 직접 입력하는 것은 우리가 제어할 수 있는 대상이 아니니 테스트 대상이 아니라고 판단하는게 맞을 것 같아요. 그렇다면 우리가 제어할 수 없는 사용자 입력을 어떻게 제어할 수 있는 범위로 넣을 것인가?를 고민했을 때 전략패턴으로 이를 해결할 수 있구요.

다만 연로그가 말한 것 처럼 사용자가입력한 값 == 문자열에 대한 테스트가 될 확률이 높겠죠? 그렇기에 불필요하다면 추가하지 않는 것도 방법이에요.

조금 더 나아가서 프로그램 전체가 올바르게 동작한다 즉 레이싱컨트롤러.start()를 했을 때 전체 프로그램이 동작한다를 증명하려면 입력에 대한 전략패턴이 분명 필요해질 수 있겠죠? 어떤 것을 테스트할 것인가에 따라 전략패턴도, 다른 기법들도 사용될지 아닐지가 달라질 수 있다는 점까지 염두해두면 더 좋을 것 같습니다🙂

yeon-06 added 14 commits February 19, 2022 16:29

docs: 리드미 갱신 - 2단계 요구사항 추가

aba2e34

style: 유틸 클래스명 XXXUtils로 통일

ea04b53

refactor: 멤버 변수를 지역 변수로 변경

51da0e9

test: 전진 또는 정지 테스트 코드 중복 제거

36efe36

test: @SuppressWarnings 대신 @DisplayName 사용

0fb6314

refactor: Random 대신 ThreadLocalRandom 사용

b21e34d

refactor: 불필요한 forEach 사용 제거

1c977cb

style: goOrStop -> go 메소드 이름 변경

97cb8c8

style: 패키지명 model -> domain 변경

44f25fc

refactor: utils 에서는 생성자 제한

154e848

fix: git conflicts 해결

eea0010

refactor: 기본 toString() 생성

57efde6

style: 메소드 순서 변경

f898133

refactor: 메소드명 변경, 미사용 메소드 삭제

9090de1

yeon-06 commented Feb 19, 2022

View reviewed changes

KIMSIYOUNG requested changes Feb 20, 2022

View reviewed changes

yeon-06 added 6 commits February 21, 2022 01:15

refactor: 미사용 메소드 제거

d192408

style: 메소드 이름 변경

de49f39

refactor: parseInt전에 input값 검증

2528710

refactor: 랜덤값에 대해 전략 패턴 사용

fb37928

refactor: Exception이 발생 가능한 inputCarNames에 대한 처리 분리

70e667e

fix: 사용자 입력 시 공백을 포함해 받을 수 있도록 수정

6dbe286

yeon-06 commented Feb 21, 2022

View reviewed changes

KIMSIYOUNG approved these changes Feb 22, 2022

View reviewed changes

KIMSIYOUNG merged commit c6c8d14 into woowacourse:yeon-06 Feb 22, 2022

		import org.junit.jupiter.api.Test;

		@SuppressWarnings("NonAsciiCharacters")

		@@ -0,0 +1,20 @@
		package racingcar.domain.strategy;

		public class FixMoveStrategy implements MoveStrategy {

[2단계 - 자동차 경주 리팩토링] 연로그(권시연) 미션 제출합니다. #419

[2단계 - 자동차 경주 리팩토링] 연로그(권시연) 미션 제출합니다. #419

Conversation

yeon-06 commented Feb 19, 2022 • edited Loading

2단계 요구사항

개선사항

고민중

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

yeon-06 Feb 20, 2022 • edited Loading

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

KIMSIYOUNG 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

yeon-06 Feb 20, 2022 • edited Loading

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

KIMSIYOUNG 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

yeon-06 commented Feb 19, 2022 •

edited

Loading

yeon-06 Feb 20, 2022 •

edited

Loading

yeon-06 Feb 20, 2022 •

edited

Loading