[2단계 - 자동차 경주 리팩터링] 알린(장원영) 미션 제출합니다. #387

OzRagwort · 2022-02-17T12:40:11Z

안녕하세요 에단 알린입니다!

이번 단계는 MVC 패턴이 되려면 어떻게 Model, View, Controller를 작성해야할까? 어떤 동작을 해야할까? 등을 고민하면서 리팩터링해봤습니다.

이전 단계에서 Test의 잘못된 부분과 부족했던 부분을 수정해봤습니다.

수정한 내용을 README에 기록하면서 리팩터링을 했습니다.

2단계 미션 리뷰도 잘 부탁드립니다!

- 문제점 : 첫번째 입력인 이름은 입력을 잘 받지만 두번째 입력인 시도 횟수를 제대로 입력받지 못함(null 입력이 됨) - 원인 분석 : InputView에서 Scanner를 상수로 처리했는데 이렇게 되면 문제가 발생하는것으로 예상됨 - 해결 방법 : Controller에서 Scanner를 만들어 파라미터로 주거나 InputView에서 Scanner를 static으로 쓰지 않으면 문제가 해결됨 - static을 없애고 InputView의 인스턴스 변수로 Scanner를 만들어 사용하는 방식으로 문제를 해결함 - Scanner를 Controller에서 만들어서 주지 않은 이유는 InputView에서만 사용되는 Scanner를 꼭 Controller에서 생성하고 가지고 있어야하나? 라는 생각이 들었기 때문임

- MVC 패턴 관련 리팩터링 내용 - Test 관련 리팩터링/오류 수정 내용

Laterality

안녕하세요 알린, 리뷰어 에단입니다.

VO가 추가되면서 책임이 더 분명하게 나뉘었네요 👍

몇군데 코멘트를 남겨드렸는데요. 참고하시면 더 좋은 코드가 될 것 같아요!

아래는 코드에 남겨드린 것 외에 추가로 드리고 싶은 코멘트에요.

indent width가 2->4로 변경되면서 들여쓰기만 변경된 코드도 변경사항에 포함됐는데요. 하나의 PR에는 변경사항이 적을수록 코드를 확인하는 동료가 더 중요한 변경사항에 집중할 수 있어요. 불필요한 변경이 생기지 않도록 가급적 코드의 초기 포맷을 유지해주세요.
코멘트에 이미 남겨드리긴 했지만 domain 영역에 util 클래스/패키지가 꼭 필요한지 한번 고민해보시면 좋겠습니다. 만약 util 패키지 없이 개발해야 한다면 현재 util 패키지에 있는 코드는 어디로 이동하게 될까요?

Laterality · 2022-02-17T13:50:09Z

src/main/java/racingcar/domain/dto/RequestAttemptDto.java

@@ -0,0 +1,16 @@
+package racingcar.domain.dto;


도메인에서 DTO가 맡은 역할은 무엇일까요?

DTO는 계층 사이에서(domain과 view사이) 필요한 데이터를 안전하게 이동하는 역할을 합니다.

저는 세 가지 이유로 DTO를 사용했습니다.

domain과 view 사이에서 데이터의 불변을 보장하여 전달하기 위해

domain가 view에 의존하지 않고 동작하게 하기 위해

객체 전체가 아닌 꼭 필요한 데이터만을 view로 보내기 위해

getter, setter 외에 다른 로직을 가지지 않는 특징이 있습니다. setter를 쓰지 않는다면 객체를 생성할 때만 데이터가 설정되기 때문에 전달 과정(controller)에서 전달하는 값이 수정되지 않는 것을 보장할 수 있다고 이해했습니다.

이번 단계에서 핵심 리팩터링 요구사항으로 MVC 패턴 기반으로 리팩토링해 view 패키지의 객체가 domain 패키지 객체에 의존할 수 있지만, domain 패키지의 객체는 view 패키지 객체에 의존하지 않도록 구현한다. 라는 내용이 있었습니다.

A가 B를 의존한다는 것은 A가 B를 사용하고 있어서 B가 변경되면 A도 변경될 수도 있다는 의미로 이해했습니다. 이 부분을 알고 나니 왜 domain 부분에서 View의 세부 사항을 알 필요가 없는지 이해했습니다.

이 요구사항을 만족하기 위해 domain의 객체에서 toString()을 view로 보내지 않고 DTO를 이용하여 view로 필요한 데이터를 보내주게 되었습니다.

그리고 Domain에서 Car를 줄테니 View에서 getter를 쓰거나 다른 조작을 하는 등 다른 작업을 하지 않아야 한다고 생각합니다. 그래서 DTO로 값을 보낼때도 Car와같은 객체보다 View에서 꼭 필요한 name과 position만으로 DTO를 만들어 보내야 한다고 생각하고 구현했습니다.

추가로 DTO는 Domain과 View에서 모두 사용하는데 꼭 Domain에 있어야 한가? 라는 생각이 들어서 racingcar 패키지 하위로 이동시켰습니다.

Laterality · 2022-02-17T13:51:23Z

src/main/java/racingcar/domain/dto/RequestAttemptDto.java

+
+import racingcar.domain.vo.Attempt;
+
+public class RequestAttemptDto {


DTO들이 필드를 하나씩만 가지고 있는데, 굳이 DTO로 묶을 필요가 있을까요?

이전 코드를 작성할 당시에는 DTO에 대해 제대로 이해하지 않고 사용하면서 조금은 무의식적으로 사용했던 것 같습니다. 공부를 하고 난 뒤 이 부분을 보면서 DTO가 필요한 이유를 고민해봤습니다.

DTO가 필드를 하나만 가지고 있더라도 DTO를 이용하여 값을 전달하면 domain에서 데이터를 가져오기 전까지 변조가 되지 않는 장점이 있을 것 같습니다. Contoller에서 View와 Domain사이에 오가는 데이터를 수정할 이유도 없고, 수정해서도 안되기 때문에 DTO로 데이터 불변을 보장하는 것이 좋다고 생각됩니다.

Laterality · 2022-02-17T13:53:04Z

src/main/java/racingcar/domain/enums/ErrorMessage.java

+    TOO_LONG_CAR_NAME_ERROR_MESSAGE("입력한 자동차 이름이 너무 깁니다."),
+    NUMBER_FORMAT_ERROR_MESSAGE("시도 회수는 숫자로 입력해야합니다."),
+    NUMBER_NEGATIVE_ERROR_MESSAGE("시도 회수는 0이상이어야 합니다."),
+    EMPTY_INPUT_ERROR_MESSAGE("입력값이 없습니다.");


이 에러 메시지는 View에서만 사용되는데 도메인 패키지에 위치한 이유는 무엇일까요?

제가 아직 view와 domain의 경계를 구분하지 못하고 있어서 포함한 것 같습니다.

View에서 발생하는 예외 처리에 사용할 ViewErrorMessage와 domain에서 발생하는 예외 처리에 사용할 DomainErrorMessage로 분리하였습니다.

이 부분에서 질문이 있습니다.

처음에는 패키지 이름이 달라서 같은 ErrorMessage라는 이름을 사용하려 했는데 ApplicationTest에서 같은 이름을 가지면서 에러가 발생했습니다.

racingcar.domain.enums.ErrorMessage 와 racingcar.view.enums.ErrorMessage같이 전체 경로를 써서 구분할 수 있긴 하지만 매번 길게 쓰기도 힘들고 구분하기 어려울 것 같았습니다.

이런 문제를 해결하기 위해 앞에 View와 Domain을 붙였는데, 패키지 이름을 포함하면 view.enum.ViewErrorMessage, domain.enum.DomainErrorMessage가 되어 View와 Domain이 중복되었습니다.

이런 상황이라면 패키지와 Enum 명에 중복이 생기더라도 구분을 하는 게 더 좋을까요?

서로 다른 클래스에 이름이 같은 패키지가 위치하면 몇가지 단점들이 있는데요.

말씀하신 것처럼 한 코드에서 이들을 함께 사용하는 경우 패키지까지 명시해서 구분해야 한다는 점.

이름만 나타내면 실수로 이름이 같은 다른 클래스를 import해서 사용하더라도 실수를 눈치채기가 어렵다는 점.

때문에 가능하면 패키지가 다르더라도 이름이 중복되지 않도록 짓는 것이 좋습니다.

관점을 바꿔서, 이 enum이 과연 필요한 것인가를 생각해볼 수도 있는데요.

왜 오류 메시지는 이 메시지를 사용하는 곳(메시지를 담은 예외 클래스나 예외를 던지는 코드)이 아니라 별도의 enum으로 분리됐을까요?

enum으로 분리한 이유와 문제점

오류 메시지를 가진 enum을 왜 사용했는지를 생각해보면 오류가 발생하는 곳에서 사용하기도 하지만 Test코드에서 사용하기 위해 생성한 것 같았습니다. 오류 메시지를 바꾸게 되면 테스트도 바꿔야 하고 구현 코드도 바꿔야 하네? 왜 두 곳이나 바꿔야 하지? 라는 생각이었습니다. 더 정확한 테스트를 위해 hasMessageContaining을 이용해서 테스트했는데, enum만들고 테스트 코드에서도 get()으로 불러오면 되겠다고 짧게 생각했던 것 같습니다. 지금 다시 생각해보면 몇 가지 문제가 있을 것 같습니다.

오류 메시지를 수정해야 한다면 test코드의 수정 없이 enum파일만 수정하면 되기 때문에 test코드가 사실상 의미가 없는 것이 아닌가?

TDD는 test코드를 수정하고 빨간불을 본 뒤 구현 코드를 수정하여 초록불이 되도록 해야 하는데 enum 파일만 수정하면 되기 때문에 TDD의 목적과 정반대가 아닌가?

지금 enum에서 상수의 이름만 보면 어느 클래스, 어느 위치에서 사용되는지 알기 어렵다.

오류 메시지를 사용하는 여러 클래스가 하나의 enum에 의존하게 될 것 같은데 코드의 수정을 조심해야 한다거나 다른 클래스의 오류 메시지까지 알게되는 등 문제점이 발생할 수 있지 않을까?

지금 다시 생각해보면 오류 메시지가 바뀌게 된다면 test코드를 고치고 구현 코드를 고치면서 두 곳이 모두 수정되는 것이 맞다고 생각됩니다. enum이 생기면서 TDD의 목적과도 반대되고 불필요한 의존성까지 생기기 때문에 이 상황에서는 enum이 꼭 필요한 것은 아니라고 생각됩니다.

질문

하지만 구현 코드에서는 이런식으로 오류 메시지를 모아두는 enum이 필요없을 수 있지만 test source set에서는 써도 되지 않을까? 라는 생각을 하게 되었습니다.

만약 CarName에서 발생하는 오류 메시지를 테스트하는 곳이 CarNameTest와 ApplicationTest라고 한다면, 이 오류 메시지를 수정할 때 과정은 아래처럼 될 것 같습니다.

CarName(수정x), CarNameTest(수정o, 실패), ApplicationTest(수정x, 성공)

CarName(수정o), CarNameTest(수정o, 성공), ApplicationTest(수정x, 실패)

CarName(수정o), CarNameTest(수정o, 성공), ApplicationTest(수정o, 성공)

CarName에 대한 테스트가 두 개만 있으면 괜찮겠지만 많아진다면 그만큼 수정할 테스트코드가 많아지고 복잡해질 것 같습니다. 이럴때 test source set에서 오류 메시지 enum을 만들고 사용한다면 더 빠르게 테스트 코드를 수정할 수 있을 것 같았습니다.

하지만 위에 3,4번의 문제도 있고, 내가 편하자고 enum을 넣는게 맞는건가 해서 적용하지는 않았습니다. test source set에서 오류 메시지 enum을 만들어 사용하는 것에 대해서는 어떻게 생각하시나요?

테스트 내에서 중복된 문자열 사이의 중복을 줄이기 위해 테스트 내에 상수를 별도로 두는 것은 문제가 없다고 생각해요. 오히려 테스트에는 값을 하드코딩하는 것이 테스트를 더 명확하게 나타내기 때문에 더 낫다는 의견도 있기 때문에 이 부분은 주관적인 영역으로 남겨둘게요.

다만 상수를 enum으로 모으겠다면 이를 일관성있게 유지하는 게 중요한데요. 아래 질문들을 고민해보시면 좋겠어요.

Q1. 만약, 예외 메시지에 (사용자 이름 xxx는 존재하지 않는 사용자입니다와 같이)오류가 발생한 파라미터가 포함된다면 예외 메시지를 어떻게 만들어야 할까요? 혹은 비슷하게 예외 메시지에 특정 요소가 있을 수도 있고 없을 수도 있는 것처럼 메시지가 동적으로 생성되는 예외는 어떻게 테스트해야 할까요?

Q2. enum에 정규식이나 format string으로 패턴을 매칭해야 할까요?

Q3. 만약 패턴이 복잡해진다면 잘못된 패턴으로 인해 통과해선 안되는 코드가 테스트에 통과할 수도 있을텐데, 이 패턴에 대한 테스트는 필요 없을까요?

Laterality · 2022-02-17T13:55:16Z

src/main/java/racingcar/domain/util/MovementUtil.java

@@ -0,0 +1,14 @@
+package racingcar.domain.util;
+
+public class MovementUtil {


비단 이 클래스 뿐만 아니라 XXUtil과 같은 Util 클래스는 어떤 책임을 가져야 할까요?

보통 Util 클래스는 문자열 관련, 랜덤값 구하기, 날짜 및 시간 구하기 등 특정 비지니스 로직이나 독립적인 기능을 한다고 합니다.

책임은 어떤 요청에 대해 그 클래스가 꼭 해야 하는 일과 같은 개념으로 이해하고 있습니다. 제가 생각해본 Util 클래스의 책임은 여러 계층에서 사용될 수도 있고 특별한 의존 관계가 없는 일인 것 같습니다.

그 기준을 아직은 정확하게 잡기는 어렵지만 java.util , java.lang , java.time패키지들을 사용하는 간단한 동작들을 Util 클래스로 만들어 사용할 수 있을 것 같습니다. 예를 들어 java.util.Random을 사용하여 랜덤값을 만들어내는 경우 RandomUtil을 만들어 여러 상황에서의 랜덤값을 얻어올 수 있다고 생각합니다. 물론 지금은 Cars에서만 자동차 이동을 위해 0~9까지의 랜덤값을 받아오는 기능을 사용하고 있어서 Util 클래스를 만들지 않고 Cars에 직접 구현할 수도 있겠지만, 랜덤 값을 만드는 것이 자동차가 할 일인가? 라고 생각하면 아니라고 생각되어서 RandomUtil을 만드는 것이 더 좋을 것 같습니다. 또 InputView에서 자동차의 이름을 받아와서 split하는 메서드가 있는데 이런 것도 StringUtil 클래스를 만들어 사용해도 될 것 같습니다. 입력을 나누는 일이 View의 일인가? 라고 생각하면 아니라고 생각되었기 때문입니다.

RandomUtil은 시작 숫자와 끝 숫자만을 받아와 그사이에 랜덤값을 반환하는 기능을 하는 메서드를 가지고 시작 숫자와 끝 숫자는 Cars가 가지도록 수정하였습니다.

MovementUtil은 파라미터로 받은 값에 따라 이동할지 말지를 확인하고 있는데 이것은 Car에서 해도 될 것 같습니다.

관심사가 적절히 나뉘었는지 고민하셨군요 👍

Util이라는 이름은 어디서든 사용할 수 있다는 뉘앙스를 풍기기도 하는데요.

그러다보니 여러 클라이언트가 사용하는 코드가 될 수 있기 때문에 주의가 필요합니다. 변경이 발생할 경우 파급력이 클 수 있기 때문에 변경을 최소화해야 합니다.

"비지니스 로직"이라고 부를 만큼 중요한 로직이 Util 클래스에 위치하게 된다면 도메인에 그 로직이 위치하기에 더 적절한 곳이 있을 수도 있어요. Util 클래스/메서드를 작성할 때에는 항상 "이 기능이 정말 Utility 성격인지" 고민해보시는 게 좋습니다. Util에 어떤 기능을 두어야 하는지에 대한 명확한 기준을 세워보시는 것도 도움이 될 거에요.

Laterality · 2022-02-17T13:56:53Z

src/main/java/racingcar/domain/util/NonMovableNumberGenerator.java

@@ -0,0 +1,9 @@
+package racingcar.domain.util;
+
+public class NonMovableNumberGenerator implements NumberGenerator {


테스트에만 사용되는 클래스는 test source set에 선언해주세요.

제가 test source set를 src에 있는 main과 test에서 test라고 이해했는데 맞을까요?

모두 test에 racingcar.strategy로 이동하였습니다!

네 맞습니다 :)

Laterality · 2022-02-17T14:01:01Z

src/main/java/racingcar/domain/vo/Attempt.java

+    private int toInteger(String string) {
+        return Integer.parseInt(string);
+    }
+
+    private void validateNumberFormat(String attempt) {
+        try {
+            Integer.parseInt(attempt);
+        } catch (NumberFormatException numberFormatException) {
+            throw new RuntimeException(ErrorMessage.NUMBER_FORMAT_ERROR_MESSAGE.get());
+        }
+    }


Integer.parseInt()가 중복으로 호출되는데요. 어떻게 하면 호출을 줄일 수 있을까요?

Integer.parseInt()는 내부에서 입력이 숫자가 아니면 알아서 NumberFormatException을 던지는 것 같습니다. 그래서 validateNumberFormat 메서드는 필요가 없을 것 같습니다. 하지만 toInteger에서 NumberFormatException이 발생했을 때 원하는 에러 메시지로 예외를 던지고 싶어서 toInteger() 안에서 try catch문을 이용했습니다.

Laterality · 2022-02-17T14:02:16Z

src/main/java/racingcar/domain/vo/Attempt.java

+
+    private void validateNegative(int attempt) {
+        if (attempt < ZERO) {
+            throw new RuntimeException(ErrorMessage.NUMBER_NEGATIVE_ERROR_MESSAGE.get());


적절한 시점에 예외를 던지는 것도 중요하지만, 예외의 종류도 중요합니다.

어떤 예외를 던지느냐에 따라 사용자에게 전달되는 의미가 다른데요. 이 상황에 더 적절한 예외는 없을까요?

전체 코드를 보면서 대부분 RuntimeException으로 예외를 던지는것을 확인했습니다. 이렇게 예외를 처리하면 정확하게 어떤 상황인지 알 수 없다는 문제가 발생할 것 같습니다.

어떤 상황에서 어떤 예외를 사용했는지는 다음과 같습니다.

사용자가 입력한 데이터가 잘못된 경우 → IllegalArgumentException

찾으려는 데이터가 없는 경우 → NoSuchElementException

Integer.parseInt()에 문자열을 파라미터로 준 경우 → NumberFormatException

잘못된 상태가 된 경우 → IllegalStateException

여기서 조금 헷갈리는 부분이 IllegalStateException입니다.

IllegalStateException는 Cars의 findWinners()에서 자동차가 없는경우 발생하는데 Cars를 생성할 때 자동차가 없으면 에러가 발생하기 때문에 자동차가 없는 상태를 가진 Cars가 findWinners를 호출할 일이없을 것 같아서 사용했습니다. 적절한 사용이 맞을까요?

이부분은 사례가 워낙 다양해서 언제 던지는 것이 적절하다고 단정하기는 어렵지만,

IllegalStateException의 Javadoc은 이 예외를 아래와 같이 설명하고 있습니다.

Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.

문서대로라면 "호출 시점이 적절하지 않은 경우" 이 예외를 던지는 게 적절해보입니다(스트림을 close한 다음에 읽거나 쓰는 경우가 있겠네요).

저는 비지니스 로직의 전제 조건이 만족되지 않는 경우에 이 예외를 던지기도 합니다. 논리적으로는 반드시 존재해야 하는 객체가 실제로는 저장소에 존재하지 않는다거나 하는 경우를 예로 들 수 있겠네요.

Laterality · 2022-02-17T14:06:09Z

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

+        return position.compareTo(car.position) == SAME_POSITION;
+    }
+
+    public void move(int randomNumber) {


인자명이 randomNumber인데요. 굳이 random이어야 할까요?

move는 받는 값이 랜덤한 값인지 무엇인지는 알 필요가 없을 것 같습니다.

간단하게 int number로 수정하였습니다!

Laterality · 2022-02-17T14:08:59Z

src/main/java/racingcar/domain/vo/CarName.java

+        return name.hashCode();
+    }
+
+    private void validEmpty(String name) {


메서드명은 동사로 시작하면 어떨까요?

꼼꼼하게 확인하지 못했네요. 수정했습니다! 😂

Laterality · 2022-02-17T14:30:30Z

src/main/java/racingcar/domain/vo/RoundResult.java

+    private Map<String, Integer> roundResult = new LinkedHashMap<>();
+
+    public void add(Car car) {
+        String[] carInformation = car.toString().split(RESULT_DELIMITER);


toString에 관해 이전 PR에서 남겨드린 코멘트를 참고해주세요.

toString()은 “이펙티브 자바의 Item 12”와 여러 곳에서 toString()에 대해 보면서 toString()은 디버깅을 위해 사용하는 것 외에 사용은 하지 않는 것이 좋다고 생각하게 되었습니다. 그래서 이 부분과 테스트에서 toString을 이용하여 테스트하는 모든 부분을 수정하였습니다.

이전 코드는 내가 정한 규칙으로 반환한 toString()을 이용하여 나만 알 수 있는 방법으로 데이터를 가져왔던 것 같습니다. 다른 개발자가 이 코드를 본다면 쉽게 이해하기 어려울 수 있을 것 같습니다. toString()을 활용하는 방법을 잘못 이해했기 때문에 발생한 문제인 것 같습니다.

처음에 RoundResult에서 toString()으로 데이터를 가져온 이유는 강박적으로 getter를 쓰지 않기 위해서 였습니다. 그런데 조금 더 생각해보니 Car 객체의 이름과 이동 거리를 view에서 사용하게 되었기 때문에 getter를 써도 될 것 같다고 생각했습니다.

getter를 쓰지 말라는 이유는 getter로 반환된 값을 다른 메서드가 수정하거나 사용하는 로직을 처리하지 말라는 식으로 이해했습니다. 따라서 view에서 사용하기 위해서 getter를 하는 것은 허용이 되지않을까? 라고 생각했습니다.

- 도메인에서 DTO가 맡은 역할이 무엇일까요? - DTO가 필드를 하나씩만 가지는데 DTO를 쓸 필요가 있나요?

- 2번 중복으로 호출되는 Integer.parseInt()의 호출을 1번 호출되도록 수정

- NumberGenerator를 test source로 이동 - MovementUtil가 하던 일을 Car안에서 하도록 이동 - RandomUtil를 만들어 랜덤값에 대한 일을 하도록 함

OzRagwort · 2022-02-19T08:16:06Z

안녕하세요 에단! 꼼꼼하고 소중한 피드백 감사합니다!

말씀해주신 피드백 중 추가로 해주신 피드백에 대해 고민해봤습니다.

indent width가 2->4로 변경되면서 들여쓰기만 변경된 코드도 변경사항에 포함됐는데요. 하나의 PR에는 변경사항이 적을수록 코드를 확인하는 동료가 더 중요한 변경사항에 집중할 수 있어요. 불필요한 변경이 생기지 않도록 가급적 코드의 초기 포맷을 유지해주세요.

그 부분을 생각하지 못했네요 😢 다음부터는 더 신경쓰겠습니다. 감사합니다!
코멘트에 이미 남겨드리긴 했지만 domain 영역에 util 클래스/패키지가 꼭 필요한지 한번 고민해보시면 좋겠습니다. 만약 util 패키지 없이 개발해야 한다면 현재 util 패키지에 있는 코드는 어디로 이동하게 될까요?

util 패키지가 없이 개발되어야 한다면 기존의 RandomNumberGenerator와 MovementUtil 모두 Cars가 가져야 할 것 같습니다. 랜덤값을 생성하는 기능, 이동이 가능한지 확인하는 기능 모두 Cars에서만 사용하기 때문입니다. 다른 클래스를 만들게 되면 의존관계에 따라 한 클래스가 수정되면 다른 클래스에도 영향을 줄 것 같았기 때문입니다. 추가로 util은 domain이 아닌 패키지에서도 사용될 수 있을 것 같습니다. 그래서 util 패키지를 만들게 된다면 domain 영역이 아닌 더 상위에 util 패키지를 만들어야 할 것 같습니다.

Laterality

안녕하세요 알린, 에단입니다 :)

각 기능에 대한 책임이 더 잘 분리됐네요 👍

추가로 몇가지 코멘트를 남겨드렸는데요. 이전 코멘트 중에 확인되지 않은 부분과 함께 확인 부탁드릴게요.

추가로, 도메인 객체가 모두 vo 패키지 내에 있는데, 정말 모두 VO가 맞는지도 고민해보시면 좋겠습니다.

Laterality · 2022-02-19T09:10:12Z

src/main/java/racingcar/domain/enums/ErrorMessage.java

+    TOO_LONG_CAR_NAME_ERROR_MESSAGE("입력한 자동차 이름이 너무 깁니다."),
+    NUMBER_FORMAT_ERROR_MESSAGE("시도 회수는 숫자로 입력해야합니다."),
+    NUMBER_NEGATIVE_ERROR_MESSAGE("시도 회수는 0이상이어야 합니다."),
+    EMPTY_INPUT_ERROR_MESSAGE("입력값이 없습니다.");


서로 다른 클래스에 이름이 같은 패키지가 위치하면 몇가지 단점들이 있는데요.

말씀하신 것처럼 한 코드에서 이들을 함께 사용하는 경우 패키지까지 명시해서 구분해야 한다는 점.

이름만 나타내면 실수로 이름이 같은 다른 클래스를 import해서 사용하더라도 실수를 눈치채기가 어렵다는 점.

때문에 가능하면 패키지가 다르더라도 이름이 중복되지 않도록 짓는 것이 좋습니다.

관점을 바꿔서, 이 enum이 과연 필요한 것인가를 생각해볼 수도 있는데요.

왜 오류 메시지는 이 메시지를 사용하는 곳(메시지를 담은 예외 클래스나 예외를 던지는 코드)이 아니라 별도의 enum으로 분리됐을까요?

Laterality · 2022-02-19T09:23:38Z

src/main/java/racingcar/domain/util/MovementUtil.java

@@ -0,0 +1,14 @@
+package racingcar.domain.util;
+
+public class MovementUtil {


관심사가 적절히 나뉘었는지 고민하셨군요 👍

Util이라는 이름은 어디서든 사용할 수 있다는 뉘앙스를 풍기기도 하는데요.

그러다보니 여러 클라이언트가 사용하는 코드가 될 수 있기 때문에 주의가 필요합니다. 변경이 발생할 경우 파급력이 클 수 있기 때문에 변경을 최소화해야 합니다.

"비지니스 로직"이라고 부를 만큼 중요한 로직이 Util 클래스에 위치하게 된다면 도메인에 그 로직이 위치하기에 더 적절한 곳이 있을 수도 있어요. Util 클래스/메서드를 작성할 때에는 항상 "이 기능이 정말 Utility 성격인지" 고민해보시는 게 좋습니다. Util에 어떤 기능을 두어야 하는지에 대한 명확한 기준을 세워보시는 것도 도움이 될 거에요.

Laterality · 2022-02-19T09:24:41Z

src/main/java/racingcar/domain/util/NonMovableNumberGenerator.java

@@ -0,0 +1,9 @@
+package racingcar.domain.util;
+
+public class NonMovableNumberGenerator implements NumberGenerator {


네 맞습니다 :)

Laterality · 2022-02-19T09:35:41Z

src/main/java/racingcar/domain/vo/Attempt.java

+
+    private void validateNegative(int attempt) {
+        if (attempt < ZERO) {
+            throw new RuntimeException(ErrorMessage.NUMBER_NEGATIVE_ERROR_MESSAGE.get());


이부분은 사례가 워낙 다양해서 언제 던지는 것이 적절하다고 단정하기는 어렵지만,

IllegalStateException의 Javadoc은 이 예외를 아래와 같이 설명하고 있습니다.

Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.

문서대로라면 "호출 시점이 적절하지 않은 경우" 이 예외를 던지는 게 적절해보입니다(스트림을 close한 다음에 읽거나 쓰는 경우가 있겠네요).

저는 비지니스 로직의 전제 조건이 만족되지 않는 경우에 이 예외를 던지기도 합니다. 논리적으로는 반드시 존재해야 하는 객체가 실제로는 저장소에 존재하지 않는다거나 하는 경우를 예로 들 수 있겠네요.

Laterality · 2022-02-19T09:56:44Z

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

+        assertThat(results).allSatisfy(result -> {
+            assertThat(result.getNames()).hasSize(nameInput.length);
+            assertThat(result.getNames()).contains(nameInput);
+        });


회차에 대한 결과를 검증하는 것으로 보여요.

입력한 car가 모두 결과에 포함돼 있는지는 검증하고 있지만, 실제로 각 car를 움직였는지는 검증하고 있지 않네요.

이 부분도 테스트하려면 어떻게 해야 할까요?

힌트는 여기

수정한 커밋

힌트를 보고도 가장 오래 고민했지만 많이 어려운 문제였습니다. 우선 각 car들이 이동을 했는지를 확인하려면 두 가지 방법이 있을 것 같습니다.

DTO에 이동에 성공을 했는지 실패했는지 결과를 추가 → 성공, 실패의 결과를 또 어떻게 테스트할지에 대한 문제가 있음

랜덤값이 아닌 예상가능한 수로 이동을 통제하고 테스트하는 방법

1번보다는 2번이 더 적절한 방법이라고 생각했습니다.

프리코스 미션에서는 Mockito를 사용하여 Randoms클래스에서 랜덤값을 고정적으로 반환하도록 했습니다. Mockito를 적용해보려 했으나 기존의 코드를 잘 설계하면 Mockito없이도 테스트할 수 있다는 다른 크루의 피드백을 보고 사용하지 않기로 했습니다.

Mockito를 사용하지않고 비슷하게 동작하려면 결국 어쩔 수 없이 테스트를 위한 메서드를 만들어야한다고 생각했습니다. Cars에서 모든 차가 경주를 하는 메서드인 repeatRaceBy 메서드에 미리 정한 숫자들을 전달하고 랜덤값 대신 car.move(int number)에 파라미터로 주면 결과를 예상할 수 있고 자동차가 제대로 이동하는지 확인할 수 있습니다. RacingGameService도 테스트용 메서드를 만들어 RacingGameService에서도 테스트가 가능합니다.

미리 숫자와 결과를 만들어두고 사용하여 테스트하기 때문에 힌트에 나온 테스트 픽스처를 활용한 것 같은데 제대로 활용한 것이 맞을까요?

제가 생각했던 것과 조금 다른 방향으로 고민을 하셨었군요 😅

각 car가 움직였는지 검증하려면 position을 확인해야 합니다.

그러기 위해서는 position을 움직이는 요인(=숫자 생성)을 테스트 환경에서 제어할 수 있어야 할 테고요.

현재는 이 숫자 생성 방식을 테스트 대상 클래스가 무작위로 하고 있기 때문에 숫자를 생성하는 로직을 분리해보면 어떨까 하는 의도였어요.

잘 생각해보시면 Mockito를 이용해 하는 작업이 위에 설명한 것과 크게 다르지 않다는 점을 이해하실 수 있을 거에요.

Mockito가 없이도 테스트가 가능하다는 것은 그런 의미였을 거고요.

- validate 메서드는 의미없이 여러 일을 하기 때문에 코드를 이동하고 메서드는 삭제함

OzRagwort · 2022-02-20T19:17:27Z

안녕하세요 에단! 알린입니다 😃

주신 코멘트에 추가로 말씀해주신 vo에 대해 고민해봤습니다.

도메인 객체가 모두 `vo` 패키지 내에 있는데, 정말 모두 VO가 맞는가?

VO는 크게 세 가지 특징이 있다고 합니다.

setter가 없고 값이 불변해야함. → 재할당하면 안되고 불변성을 가져야함
equals(), hashCode()를 재정의 해야한다 → 동등성 검사 필요
validate()가 있어야 한다. → 자가 유효성 검증이 필요

VO는 왜 불변이어야 하는가?

VO가 왜 불변이어야 하는지를 이해하기가 조금 어려웠고 지금도 잘 알고있건지 모르겠습니다. 제가 이해한 VO가 불변이어야 하는 이유는 의도하지 않은 수정을 막기 위함입니다. VO가 가변이라면 참조 형식으로 객체를 공유하고 값이 수정되면 의도하지 않게 공유한 다른 곳에서도 값이 수정되기 때문입니다. 이 부분이 처음에는 이해하기 어려웠는데 얕은 복사, 깊은 복사에서 얕은 복사를 한 객체를 다른곳에서 사용하면 의도치않게 원본도 함께 복사가 되는 상황과 비슷하다고 느끼면서 조금은 이해가 되었습니다.

RoundResult

VO에 있는 (Cars, Car, CarName, Position, Attempt, RoundResult ) 중 이런 특징을 가지지 않는 객체는 RoundResult입니다. RoundResult는 특성으로 따지면 View로 데이터를 전달하기 위해 만들었기 때문에 DTO쪽에 더 가까운 것 같습니다. RoundResult에서 setter 역할을 하는 add()를 지우고 생성자에서 결과를 저장하도록 수정하고 DTO 패키지로 이동시켰습니다. 수정 커밋

Car, CarName, Position, Attempt

나머지는 Cars를 제외하고 모두 VO가 맞아야 한다고 생각합니다. 하지만 제 구현 코드에서 Attempt와 Position는 내부의 값이 변합니다. Position이 가변이면 Car도 가변입니다. 따라서 제가 작성한 코드에서 객체들은 아직은 VO가 아닙니다. 이 객체들을 모두 불변 객체로 만든다면 VO가 된다고 생각합니다. 수정 커밋

Cars

Cars는 일급 컬렉션 입니다. 처음에는 제 구현 코드에서 Cars에 있는 코드들은 원래 Service에 있어야 하는 코드이기 때문에 VO가 아닐 수도 있다고 생각했습니다. 그러나 결국 일급 컬렉션도 어느 값을 Wrapping한 것이고 값이 불변해야한다는 글(일급 컬렉션 (First Class Collection)의 소개와 써야할 이유)을 보고 VO인것 같기도 합니다.

질문(일급 컬렉션은 꼭 불변해야할까요?)

아무리 고민해봐도 Cars가 불변해야한다면 일급 컬렉션을 쓰지 않고 구현했을 때 Service에 있을 List도 불변해야한다는 것인데 정말 그럴 필요가 있는지 잘 모르겠습니다. 그래서 저는 꼭 완벽한 불변일 필요는 없고 재할당만 방지하면 될 것이라고 생각하여 Cars에서 List<Car>를 final로 선언하는 정도로 수정하였습니다. 그리고 Cars에서 무언가를 반환하는 경우 Collections.umodifiableList를 사용해 반환한 데이터를 수정할 수 없도록 했습니다. 반환된 List를 수정하려면 Collections.umodifiableList때문에 수정이 되지 않고 각각의 Car는 VO로 불변 객체이기 때문에 결국 Cars는 재할당만 방지하고 반환값만 잘 관리하면 될 것 같다고 생각했습니다. 이 부분에 대해서 에단의 생각이 궁금합니다.

참고한 곳

DTO vs VO vs Entity
일급 컬렉션 (First Class Collection)의 소개와 써야할 이유
VO(Value Object)란? by 검프
VO(Value Object)란? by livenow

Laterality

안녕하세요 알린,

숫자 생성을 전략으로 분리하는 것 까지는 좋았는데, 적용하는 부분에서 어려움이 있으셨던 것 같아요.

코멘트 참고하셔서 이 부분은 꼭 완성해보셨으면 좋겠어요.

다음 미션 진행을 위해 이번 미션은 여기서 마무리하도록 할텐데요. 궁금하신 점이 있으시면 언제든 DM 주세요.

고생 많으셨어요!

Q&A

질문주신 내용에 딱 맞는 좋은 글이 있어서 공유드릴게요.

Laterality · 2022-02-21T09:46:53Z

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

+        assertThat(results).allSatisfy(result -> {
+            assertThat(result.getNames()).hasSize(nameInput.length);
+            assertThat(result.getNames()).contains(nameInput);
+        });


제가 생각했던 것과 조금 다른 방향으로 고민을 하셨었군요 😅

각 car가 움직였는지 검증하려면 position을 확인해야 합니다.

그러기 위해서는 position을 움직이는 요인(=숫자 생성)을 테스트 환경에서 제어할 수 있어야 할 테고요.

현재는 이 숫자 생성 방식을 테스트 대상 클래스가 무작위로 하고 있기 때문에 숫자를 생성하는 로직을 분리해보면 어떨까 하는 의도였어요.

잘 생각해보시면 Mockito를 이용해 하는 작업이 위에 설명한 것과 크게 다르지 않다는 점을 이해하실 수 있을 거에요.

Mockito가 없이도 테스트가 가능하다는 것은 그런 의미였을 거고요.

Laterality · 2022-02-21T09:55:59Z

src/main/java/racingcar/domain/enums/ErrorMessage.java

+    TOO_LONG_CAR_NAME_ERROR_MESSAGE("입력한 자동차 이름이 너무 깁니다."),
+    NUMBER_FORMAT_ERROR_MESSAGE("시도 회수는 숫자로 입력해야합니다."),
+    NUMBER_NEGATIVE_ERROR_MESSAGE("시도 회수는 0이상이어야 합니다."),
+    EMPTY_INPUT_ERROR_MESSAGE("입력값이 없습니다.");


테스트 내에서 중복된 문자열 사이의 중복을 줄이기 위해 테스트 내에 상수를 별도로 두는 것은 문제가 없다고 생각해요. 오히려 테스트에는 값을 하드코딩하는 것이 테스트를 더 명확하게 나타내기 때문에 더 낫다는 의견도 있기 때문에 이 부분은 주관적인 영역으로 남겨둘게요.

다만 상수를 enum으로 모으겠다면 이를 일관성있게 유지하는 게 중요한데요. 아래 질문들을 고민해보시면 좋겠어요.

Q1. 만약, 예외 메시지에 (사용자 이름 xxx는 존재하지 않는 사용자입니다와 같이)오류가 발생한 파라미터가 포함된다면 예외 메시지를 어떻게 만들어야 할까요? 혹은 비슷하게 예외 메시지에 특정 요소가 있을 수도 있고 없을 수도 있는 것처럼 메시지가 동적으로 생성되는 예외는 어떻게 테스트해야 할까요?

Q2. enum에 정규식이나 format string으로 패턴을 매칭해야 할까요?

Q3. 만약 패턴이 복잡해진다면 잘못된 패턴으로 인해 통과해선 안되는 코드가 테스트에 통과할 수도 있을텐데, 이 패턴에 대한 테스트는 필요 없을까요?

Laterality · 2022-02-21T10:02:42Z

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

@@ -0,0 +1,102 @@
+package racingcar.domain.fcc;


패키지명의 fcc는 어떤 의미인가요?

Laterality · 2022-02-21T10:05:00Z

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

+        return new RoundResult(result);
+    }
+
+    public List<RoundResult> repeatRaceBy(Attempt attempt, List<List<Integer>> numberSet) {


이 부분은 코멘트를 참고하셔서 어떻게 제거할 수 있을지 꼭 연습해보시면 좋겠어요.

OzRagwort added 19 commits February 16, 2022 15:47

refactor(cars): 랜덤값을 생성하는 변수명을 의도가 나타내도록 수정

3f51525

refactor: @ParameterizedTest로 중복된 테스트 통합

7bb7e27

refactor: 우아한테크코스 자바 스타일 가이드 적용

e40afcf

test(generator): 전략 패턴으로 만들어진 generator 인터페이스 테스트

8754444

refactor(winners): 출력 세부사항을 view에서 하도록 수정

b895289

refactor: DTO를 이용하여 데이터를 전달하도록 수정

d674d83

docs(README): 요구사항 정리

ab75bfa

refactor: 로직을 가지는 객체를 domain 패키지로 이동

bb60eee

refactor(main): main이 Controller가 되도록 수정

ca5516f

fix(test): 마지막 이름으로 공백이 입력될 때 예외 처리하도록 수정

5b73995

test(main): 기능 통합 테스트와 예외 상황 테스트 추가

7c195eb

refactor: controller와 domain 사이에 service를 추가

924bb31

refactor: 사용하지 않는 코드 삭제

356d87a

test: List 변수를 allSatisfy를 이용하여 테스트

4d5a3e1

docs(README): 통합 테스트에서 발생한 문제,원인,해결방법 정리

4c43a02

refactor(ErrorMessage): 에러 메시지를 가진 enum을 생성하도록 수정

4f905b2

test: hasMessageContaining()로 예외 에러 메시지를 확인하여 테스트

e9f6be3

docs(README): 리팩터링 내용을 카테고리 별로 정리

4489fa1

- MVC 패턴 관련 리팩터링 내용 - Test 관련 리팩터링/오류 수정 내용

Laterality requested changes Feb 17, 2022

View reviewed changes

OzRagwort added 10 commits February 19, 2022 02:45

docs(README): 피드백과 수정할 점 정리

08d9d7b

docs(README): DTO의 역할 정리

8a13586

- 도메인에서 DTO가 맡은 역할이 무엇일까요? - DTO가 필드를 하나씩만 가지는데 DTO를 쓸 필요가 있나요?

refactor(ErrorMessage): View와 Domain의 에러 메시지 enum을 분리

8f6cc69

refactor(Attempt): 메서드가 중복 호출되지 않도록 수정

33bced1

- 2번 중복으로 호출되는 Integer.parseInt()의 호출을 1번 호출되도록 수정

refactor: 상황에 맞는 예외를 던지도록 수정

060c4b7

refactor: 메소드, 파라미터 명을 의도와 목적에 맞도록 수정

4b0ad04

refactor: toString()을 이용한 구현 코드 수정

415256a

feat(cars): Cars 생성 시 자동차가 0개인 경우 예외 발생 검증 추가

6ec3b31

refactor(util): 책임에 따라 Util 클래스를 이동 및 수정

71811d5

- NumberGenerator를 test source로 이동 - MovementUtil가 하던 일을 Car안에서 하도록 이동 - RandomUtil를 만들어 랜덤값에 대한 일을 하도록 함

refactor(dto): dto 패키지의 위치를 racingcar 하위로 이동

e06e3de

refactor(dto): util 패키지의 위치를 가장 상위로 이동

388f0ca

Laterality requested changes Feb 19, 2022

View reviewed changes

OzRagwort added 9 commits February 20, 2022 02:56

docs(README): 이전 정리 내용 수정, 추가 수정 내용 정리

94e5d66

docs(README): static 사용에 대한 생각 정리

cf5e8e5

refactor(Attempt): 불필요한 메서드 삭제

02f04b7

- validate 메서드는 의미없이 여러 일을 하기 때문에 코드를 이동하고 메서드는 삭제함

refactor: 오류 메시지를 가진 enum 삭제

bcb354a

refactor: RoundResult를 vo 패키지에서 dto 패키지로 이동

db9ad21

refactor: VO를 불변 객체로 수정

238a59a

refactor(Cars): 경주 결과에서 이동 거리에 대한 테스트 추가, 테스트용 메서드 추가

dd85e53

test(RacingGameService): unmodifiableList 관련 테스트 추가

184c27a

refactor: 오타 수정

bd50bde

Laterality approved these changes Feb 21, 2022

View reviewed changes

Laterality merged commit 0d569ad into woowacourse:ozragwort Feb 21, 2022

OzRagwort mentioned this pull request Feb 26, 2022

[1단계 - 로또(자동)] 알린(장원영) 미션 제출합니다. woowacourse/java-lotto#374

Merged


		import racingcar.domain.vo.Attempt;

		public class RequestAttemptDto {

		@@ -0,0 +1,14 @@
		package racingcar.domain.util;

		public class MovementUtil {

		@@ -0,0 +1,9 @@
		package racingcar.domain.util;

		public class NonMovableNumberGenerator implements NumberGenerator {

[2단계 - 자동차 경주 리팩터링] 알린(장원영) 미션 제출합니다. #387

[2단계 - 자동차 경주 리팩터링] 알린(장원영) 미션 제출합니다. #387

Conversation

OzRagwort commented Feb 17, 2022

Laterality 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

enum으로 분리한 이유와 문제점

질문

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

Choose a reason for hiding this comment

OzRagwort commented Feb 19, 2022

Laterality 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

OzRagwort commented Feb 20, 2022

도메인 객체가 모두 vo 패키지 내에 있는데, 정말 모두 VO가 맞는가?

VO는 왜 불변이어야 하는가?

RoundResult

Car, CarName, Position, Attempt

Cars

질문(일급 컬렉션은 꼭 불변해야할까요?)

참고한 곳

Laterality left a comment • edited

Choose a reason for hiding this comment

Q&A

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

도메인 객체가 모두 `vo` 패키지 내에 있는데, 정말 모두 VO가 맞는가?

Laterality left a comment •

edited