[1단계 - 자동차 경주 구현] 오찌(오지훈) 미션 제출합니다. #281

Ohzzi · 2022-02-11T06:29:35Z

안녕하세요! 오찌(오지훈)입니다.
처음으로 코드 리뷰를 받는 거라 기대되기도 하고 떨리기도 하네요!
리뷰 해주시는 부분 잘 습득할 수 있도록 해보겠습니다!

이번에 페어와 코드를 작성하면서 잘 모르겠거나 어떤 방식을 사용하면 좋을지 의문인 부분들이 있어서 그 부분은 따로 정리해 보았습니다! 아무래도 TDD 자체를 처음 배우는 만큼 테스트와 관련된 의문점들이 많았었던 것 같네요.

Random 값을 테스트하기

프로덕션 코드에서 0부터 9사이의 값을 난수로 넣어주고 4 이상이면 자동차를 전진시키는 부분이 있는데, 매 시행마다 랜덤하게 값이 정해지는 만큼 테스트 코드를 작성하기가 어렵더라고요. 그래서 랜덤 값을 생성하는 RandomGenerator 클래스를 생성해서 난수 값이 0부터 9 사이의 범위에 들어가는지만 테스트 하고, 실제로 값을 넣어준 뒤에 전진 혹은 정지를 결정하는 로직은 Car 클래스로 넘겨준 뒤 테스트했습니다.

그런데 이 부분은 RandomGenerator의 테스트 부분에서 매 테스트마다 결과가 달라질수도(물론 여러번 테스트를 돌려본 결과에서는 무사히 통과되기는 했습니다) 있다는 점에서 좋은 테스트 코드는 아니지 않나...? 라는 생각이 들었습니다.

랜덤 값을 테스트하는 좋은 방법이 있을까요?

Controller의 테스트는 작성하지 않아도 괜찮을까?

다른 도메인들은 테스트 코드 작성 후 프로덕션 코드를 구현했는데, 게임 전체의 진행을 담당하는 컨트롤러 클래스의 테스트는 어떻게 해야 할지 막막해서, 결국 컨트롤러에 대한 테스트는 작성하지 않았습니다. 그런데 코드를 다 짠 후 생각해 보니까, 컨트롤러에서는 이미 다른 클래스들에서 다 테스트가 완료된 로직을 순서에 맞게 호출만 해줄 뿐이고, 기껏해야 try ~ catch 로 IllegalArgumentException 처리를 해주는 역할만 하고 있는데 테스트 코드를 작성해야 할까 라는 생각도 들었습니다. 이 부분에 대해서 어떻게 생각하시는지 궁금합니다!

상수 관리

이 부분은 페어와 제 의견이 처음에 일치하지 않았던 부분이기도 한데요, 저는 필요한 상수는 각 클래스에서 관리하도록 썼었는데, 에러 메시지나 알림 메시지(이름을 쉼표로 구분해서 입력해주세요 와 같은) 같은 메시지 관련 상수들은 따로 클래스를 만들어서 관리하자는 페어의 의견이 있었습니다. 비슷한 성격의 상수다보니 저도 페어의 의견에 동의해서 따로 메시지 상수들을 관리하는 클래스를 만들어서 사용했는데요, 이 부분에 대해서 어떻게 생각하시는지 궁금합니다!

그렇게 큰 규모의 프로그램이 아니지만 그래도 코드 리뷰해주는 것이 많은 시간과 에너지를 소모하는 일일텐데 리뷰해주시는데 정말 감사합니다! 사소한 부분이라도 눈에 밟히신다면 마음껏 지적해주세요! 감사합니다:)

jjj0611

안녕하세요 오찌 리뷰어 제이(장재주)입니다.
코드리뷰로 만나게 되어서 반가워요!

먼저 제 코드리뷰는 절대값이 아니고, 참고할만한 코멘트 정도로 이해해주시면 될 것 같아요.
따라서 이해가 되지 않는 부분도 있을거고 동의하지 않는 부분도 있을 수 있는데,
같이 이야기 하며 좋은 방향을 찾아가보면 좋을 것 같아요 :)

질문 주신 것에 대한 이야기를 하자면,

Random 값을 테스트하기

어떤 방법들을 고민해보셨을까요?
Random을 테스트한다는게 가능할까요?
이 질문에 대한 답을 들어보고 논의해보는 것이 좋을 것 같아요 :)

Controller에 대한 테스트코드 작성

잘 설계된 애플리케이션일수록 작은 단위에서 테스트하기 좋습니다. 혹시 controller를 테스트하기 위해 사용해본 방법들이 있을까요?

상수 관리

오찌는 어떤 주장을 했고, 연로그는 어떤 주장을 했나요? 페어의 의견에 동의해서라는 말에 두 사람의 어떤 주장이 있었는지와 오찌는 어떤 부분에서 동의를 하게 되었는지가 드러나지 않아서 제 의견을 드리는 것은 조금 뒤로 미룰게요! 오찌의 생각(왜 각 클래스에서 관리하자고 했다)과 연로그의 생각(왜 한 곳으로 모아서 관리하자고 했다)과 왜 연로그의 생각으로 설득 당했는지까지 적어주시면 좋을 것 같아요 🙂

먼저 요구사항에 따라 잘 정리를 해주셨고, 커밋 단위도 작게, 로그를 잘 남기려고 노력하셨네요 👍
처음하는 과정이라면 쉽지 않았을텐데 고생하셨습니다.
제가 github상으로 보기에는 indent가 tab으로 되어 있는 것 같은데,
google의 코딩컨벤션에서는 tab을 사용하지 않고, space를 사용한다고 나와 있고,
recommand된 문서 중 하나는 구현 자체는 스페이스든 탭이든 정해져 있지 않다고 나와 있지만,
제가 본 대부분의 개발자들은 스페이스를 사용하는 것 같아요!(물론 이 부분도 case by case로 모두 다를 수 있습니다)
가능하다면 그런 부분에서도 통일감을 가져가면 좋을 것 같아요!

메서드 라인이나 indent의 깊이 else문을 사용하지 않는 것 등 다른 부분은 매우 잘 지켜주셨네요 😄

코멘트 드린 부분 확인해보시고 이해가 가지 않거나 하시면 언제든 이야기 해주세요 :)
첫 미션 고생하셨습니다!

jjj0611 · 2022-02-11T16:21:01Z

src/test/java/stringcalculator/StringCalculatorTest.java

+	@Test
+	public void splitAndSum_null_또는_빈문자() throws Exception {
+		int result = StringCalculator.splitAndSum(null);
+		assertThat(result).isEqualTo(0);
+
+		result = StringCalculator.splitAndSum("");
+		assertThat(result).isEqualTo(0);
+	}


위와 같이 테스트 할 경우 13번 라인의 실패와 16번 라인의 실패 모두 해당 테스트를 실패하게 만듭니다.
지금은 간단한 테스트라서 한 눈에 보이지만, 테스트 상황이 복잡해지면 하나의 테스트에서 두 가지를 테스트하는 것이 복잡도를 증가시킬 수 있습니다.
테스트를 좀 더 작은 단위로 쪼개거나 혹은 위와 같이 동일 한 결과 값에 대해서 테스트할 수 있는 방법을 찾아보면 어떨까요?
자동차 경주게임은 아니지만 한 번 고민해보시면 좋을 것 같아요.

jjj0611 · 2022-02-11T16:30:36Z

src/main/java/racingcar/util/RandomGenerator.java

+public class RandomGenerator {
+	private static final Random random = new Random();
+
+	public static int generateNumber(int min, int max) {


min = 10, max = 0이 들어오면 어떻게 될까요?

해당 메소드를 사용하는 프로덕션 코드가 한 줄인데 거기서는 멀쩡히 들어갔어서 미처 고려하지 않고 넘어갔네요😭
만약에 min값이 max 값보다 크게 들어오면 예외를 뱉어내겠네요... min > max 일 때 처리하는 로직을 추가하도록 하겠습니다!

jjj0611 · 2022-02-11T16:32:41Z

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

+import racingcar.message.NoticeMessages;
+
+public class InputView {
+	private static final BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(System.in));


BufferedReader에 InputStreamReader를 생성하여 할당하고,
InputStreamReader를 생성하는 시점에 System.in이라는 객체를 주입했는데,
해당 객체들이 어떤 상호작용을 할까요?

System.in(InputStream)이 콘솔의 키보드 입력을 읽어오고, InputStreamReader가 입력을 읽어오는 형식을 System.in의 Byte 단위에서 Character 단위로 바꿔줍니다. 그런데 InputStreamReader는 문자 하나 하나씩 읽어오기 때문에 배열의 크기를 지정해줘야 해서, 입력된 문자를 버퍼에 쌓아뒀다가 개행 문자를 기준으로 읽어오도록 BufferedReader에 인자로 주입해서 버퍼를 통해 읽어오는 것으로 알고 있습니다!

jjj0611 · 2022-02-11T16:42:12Z

src/main/java/racingcar/message/ErrorMessages.java

+	public static final String LONG_NAME = "[ERROR] 너무 긴 이름입니다.";
+	public static final String DUPLICATED_NAME = "[ERROR] 중복된 이름이 있습니다";
+	public static final String TRY_CNT_NOT_NUMBER = "[ERROR] 시도할 횟수는 숫자여야 합니다.";
+	public static final String TRY_CNT_NOT_NATURAL_NUMBER = "[ERROR] 시도할 횟수는 자연수여야 합니다.";


이 부분은 개인적은 의견과 스타일이라고 생각해주시면 좋을 것 같아요.
시도할 횟수는 자연수여야 한다는 안내가 다소 생소한 부분이 있는 것 같아요.
좀 더 유저 친화적인 메시지를 고민해보면 어떨까요?

아무래도 자연수여야 합니다 보다는 0보다 큰 수여야 합니다 같은 메시지가 조금 더 사용자가 알아보기에 직관적일까요...?

그건 오찌가 생각하는 더 좋은 방향을 고려해주면 좋을 것 같아요. 정답은 없습니다!
다만 개발자는 단순히 코드만 짜는게 아니라 프로덕트에 대한 고민을 같이 하면 좋겠다는 생각에 드린 코멘트였어요 :)

jjj0611 · 2022-02-11T16:52:29Z

README.md

+## 구현할 기능 리스트
+- 입력 관련
+  - 자동차 이름을 입력받아 자동차 생성
+    - 쉼표로 구분
+    - 이름이 5자 이하인지 체크
+    - 중복 제거
+    - 빈 값이 있는지 체크
+  - 레이싱을 시도할 회수 입력
+    - 숫자인지 체크
+    - 양수인지 체크
+- 게임 진행
+  - 0에서 9 사이의 랜덤한 값을 생성
+  - 랜덤값이 4 이상이면 자동차를 전진
+  - 매 시도마다 진행 상태 출력
+- 게임 결과
+  - 우승자 계산
+  - 우승자 출력
+    - 결과를 String으로 만들어서 출력
+    - 공동 우승 처리


프로그래밍을 할 때 기능을 잘 정의하는 것이 무엇보다 중요한 것 같아요.
그런 점에서 기능 요구사항을 잘 정리해주셨네요 👍
조금 더 욕심을 부려보자면, 해당 기능 명세서는 프로그래머가 요구사항을 이해하는데도 도움이 되지만,
추후에 이 프로그램을 담당할 개발자가 참고할 수 있는 문서이기도 합니다.
조금 더 다른 사람이 참고하기 좋은 문서를 만들어보면 어떨까요??

아직 감은 잘 안잡히지만(😭) 좀 더 보기 편하면서도 참고할만하게 문서를 수정해보도록 하겠습니다!

jjj0611 · 2022-02-12T03:51:12Z

src/main/java/racingcar/model/TryCount.java

+
+	public TryCount(String countString) {
+		this.tryCount = convertStringToInt(countString);
+		validatePositive(tryCount);


다른 코드와 통일감이 조금 떨어지는 부분 같아요.
Name.java에서 validate하는 부분과 코드 스타일이 조금 달라보이는데,
다른 이유가 있을까요?

특별한 이유가 없다면, 코드의 일관성을 지키는 것이 가독성을 높이는 좋은 방법 중 하나입니다. :)

저는 TryCount가 숫자인지 판단하는 방법으로 convertStringToInt()에서 parseInt()를 할 때 숫자가 아니면 발생하는 NumberFormatException을 catch 해서 다시 IllegalArgumentException을 던지는 방법을 생각했습니다! 이 과정을 무사히 통과해서 tryCount 필드에 값이 주입되면 양수인지 체크할 필요가 있다고 생각해서 validatePositive()를 추가적으로 구현해주었구요.
Name에서 validate하던 방식처럼 쓴다면, Integer.parseInt()에서 NumberFormatException을 잡아내는 로직을 한번 더 중복해서 써야 한다고 생각해서, 중복 코드를 줄이기 위해 위 코드와 같이 썼습니다.

이 경우에 코드 일관성을 지키기 위해 숫자인지를 다른 방법으로 검증하거나 하는 과정이 필요할까요..?

아 저는 이 부분에서 다른 코드는 validate()를 호출해서 내부에 있는 필드를 사용하고 있고,
이 코드에서는 validate의 인자값을 지정하여 넘겨주고 있는 부분을 말씀드린거였습니다!

jjj0611 · 2022-02-12T03:52:06Z

src/main/java/racingcar/model/TryCount.java

+	private int convertStringToInt(String string) {
+		try {
+			return Integer.parseInt(string);
+		} catch (NumberFormatException e) {


구체적인 예외를 잡는 것은 매우 좋은 습관입니다 👍

jjj0611 · 2022-02-12T03:55:57Z

src/main/java/racingcar/model/Winners.java

+	public Winners(List<Car> cars) {
+		int maxPosition = getMaxPosition(cars);
+		winners = cars.stream()
+			.filter(car -> car.getPosition() == maxPosition)


자동차의 위치에 대한 판단의 주체를
해당 정보를 가지고 있는 자동차가 하도록 변경해보면 어떨까요?

이 부분이 어떤 의도로 말씀하시는지가 살짝 이해가 안돼서요!
자동차 내에 위치를 비교하는 메소드를 추가해서 boolean 값을 반환하거나 하는 식으로 변경해보라는 말씀이실까요?

읽어보고 곰곰히 생각해보다 보니 제가 맞게 이해한 것 같아서 해당 방향으로 수정하도록 하겠습니다! 프리코스 당시 전체 피드백 내용에도 비슷한 내용이 있었는데 제가 적용하지 않고 넘어간거네요!

넵 오찌가 이해한 부분이 맞습니다!
이런 부분이 getter를 사용하지 않아도 되는 예라고 생각해요!
물론 getter만 있으면 그 데이터를 사용하는 쪽에서 다 결정하면 되잖아!라고 생각할 수도 있겠지만,
아마 해당 getter가 어디서 어떤 목적으로 사용되는지를 모두 파악하기가 어려운 코드가 될 수도 있습니다.

그런데 (다른 분들이 만들어주신 것을 참고하여) isSamePositionWith(int position)과 같이 만들면
아 이건 자동차의 위치가 인자로 받은 위치와 같은지를 판단하는 메서드이구나!라고 알 수가 있겠죠? :)

jjj0611 · 2022-02-12T03:57:22Z

src/main/java/stringcalculator/StringCalculator.java

+	}
+
+	private static int convertStringToNaturalNumber(String string) {
+		int converted = Integer.parseInt(string);


String에 숫자가 아닌 값이 들어오면 어떻게 되나요??

NumberFormatException이 발생하겠네요! 확실히 계산기는 후다닥 넘어가서 그런지 짚고 넘어가지 않은 부분이 많네요😭
지적 감사합니다!

넵 오찌!
계산기도 오찌의 프로덕트 중 하나입니다!
애정을 주세요!!😄

jjj0611 · 2022-02-12T03:57:26Z

src/main/java/stringcalculator/StringCalculator.java

+	}
+
+	private static String getCustomDelimiter() {
+		return LEFT_BRACKET + matcher.group(GET_CUSTOM_DELIMITER) + DEFAULT_DELIMITER + RIGHT_BRACKET;


기존에 Cars 클래스에서 관리하던 랜덤 값의 범위 설정을 RacingController로 옮김. RandomGenerator -> RandomNumberGenerator로 이름을 변경하고 생성할 랜덤 값의 범위를 인자로 받아서 인스턴스를 생성하도록 변경. 랜덤 값 생성 메소드를 non-static으로 변경.

…리하도록 수정

…록 수정

nowTryCount 라는 단어가 말이 안되는 단어여서 currentTryCount로 변수명을 변경

Ohzzi · 2022-02-14T06:04:37Z

피드백 주신 내용들을 반영하고 개인적으로도 몇 가지 찾아보면서 코드를 수정해보았습니다 :)
우선은 제가 질문드린 내용에 대해서 말씀드리자면,

Random 값을 테스트 하기

저는 랜덤 값이 의도대로 지정한 범위에서 나오는지 테스트하고, 그 랜덤 값을 가지고 하는 메소드는 따로 테스트를 하는게 맞다고 생각했습니다! 그런데 랜덤 값이 범위에서 나오는지 하는 테스트가 사실 "랜덤"을 테스트하는거니까 같은 테스트를 10000번을 돌려서 성공하더라도 10001번째 테스트는 성공하지 않을수도 있다는 생각 때문에 이게 올바른 테스트인지 의문이 들었습니다!
계속 생각해봐도 이런 방식으로밖에 테스트를 할 수가 없을 것 같아서요, 그래도 랜덤 값이 어떤 값이 나오든 그 랜덤 값을 이용하는 메소드가 확실히 테스트 성공을 하면 문제가 없을거라고는 생각했습니다. 결국에 랜덤 자체를 테스트하는 것은 불가능한것이 아닐까 하는 생각이 드는데 제이의 생각도 같을까요?

Controller에 대한 테스트 코드 작성

컨트롤러가 전체적인 게임 진행을 담당하기 때문에, 제가 생각한 방법으로는 컨트롤러를 실행하면서 의도적인 예외 상황을 만들어서 의도한 예외 처리를 테스트하는 방법과 성공하는 상황을 만들어서 게임 종료가 이상없이 이루어지는 테스트를 생각했습니다
그런데 이 두 케이스 모두 이미 더 작은 단위에서 비슷한 테스트를 진행하고 컨트롤러에서는 이를 조합하는 거라고 생각해서 테스트 코드 작성은 따로 하지 않았습니다.

상수 관리

저는 모든 상수는 해당 상수가 쓰이는 클래스 내에서 관리하는 것이 맞다고 생각했습니다. 예전에 Constant라는 클래스를 만들어서 상수들을 몰아넣고 필요한 클래스에서 Constant를 import 해가면서 써봤는데, 코드에 갑자기 다른 클래스의 상수가 나오면 알아보기에 불편하다는 생각 때문에 각각의 클래스에 넣어주는 방법을 생각했습니다!
연로그도 대부분의 상수 처리에 대해서는 같은 의견이었지만, 메시지 상수에 대해서만 의견이 갈렸습니다! 연로그는 다른 클래스에서도 같은메시지를 재사용 할 수 있기 때문에 따로 관리하는게 좋을 것 같다는 의견을 냈습니다. 그리고 본인의 일화를 이야기해주었는데요, 실무에서 일할 때 담당자 번호가 에러 메시지가 하드코딩 되어 있었는데, 이 담당자 번호를 바꿔달라는 문의에 해당 메시지가 도메인에 있는지, 컨트롤러에 있는지, 컨트롤러에 있다면 어느 컨트롤러에 있는지 찾는 작업이 엄청 불편했다고 합니다. 만약 메시지가 분리되어 있다면 로직을 타고 가지 않아도 ErrorMessage와 같은 클래스에서 변경하면 되지 않을까 라는 생각이 들었다고 합니다. 그래서 이 부분에 대해서는 실제로 불편했던 경험이 있는 연로그의 의견을 따랐습니다!
추가로 그래서 연로그는 개인 프로젝트 할 때는 properties를 활용했다고 해요 :)

여기서부터는 제가 피드백을 받고 수정하면서 새롭게 적용해 본 부분에서 드리고 싶은 질문입니다!

최대한 getter를 지양하고자 한게 comment에서도 말씀드렸지만 getter로 뽑아낸 값이라도 final로 선언된 원시 값이 아닌 경우에는 수정이 가능하다는 점에서(예를 들어 getter로 꺼낸 List에 add를 하는 경우가 가능) 출력에 필요한 데이터도 필요한 전부 string으로 만들어서 전달했습니다! 그런데 제이가 말씀해주신대로 콘솔 게임이 아닌 경우에는 부적절한? toString인 것 같더라고요

그래서 수정을 하고자 했는데, 생각해보니 getter로 꺼낸 데이터를 임의로 조작할 수 있다는 점 때문에 지양한거라면, JPA에서 데이터를 계층간 이동시킬 때 Entity값을 넘겨주는 대신 그 Entity로 DTO를 만들어서 전달해주는 방법이 생각났습니다. 그래서 여기서도 원래 데이터(예를 들면 Car)를 변화시킬 수 없는 값을 꺼내주면 되지 않을까라는 생각이 들었습니다!
제가 그래서 최종적으로 고친 방법은 단순히 Car 객체의 이름, 위치 정보만 담는 DTO인 CarStatus를 만들어서 view로 전달해 주었습니다!
이 경우에 이렇게 DTO로 전달하는 방법은 좋은 방법일까요? 제이의 생각이 궁금합니다 :)

getter와 불변성에 대한 비슷한 질문인데, 다른 계층으로 데이터를 꺼내서 넘겨줄 때, 위에서 얘기한 CarStatus 객체를 주더라도 문제는 여러 객체를 넘겨줘서 List가 되었을 때 그 List 자체가 불변하지 않다는 점이 있더라고요. 그런데 view로 List를 넘겨주거나 하는 부분을 생각해보면 이 List 자체도 어떤 로직을 담은 것이 아니라 불변한 값만을 담고 있는 DTO라고 볼 수 있을 것 같아서, 전부 Collections.unmodifableList 자료구조로 바꿔주었는데, 이런 방법이 혹시 불필요한 로직일까요??
커밋시에 "feat: Car" 객체 추가 와 같이 title만 적은 간략한 커밋 방법이 좋을까요 아니면 body와 footer까지 모두 적은 긴 커밋이 좋을까요?

jjj0611

안녕하세요 오찌! 리뷰어 제이에요.
수정해주신 부분 확인했습니다. 고생하셨어요.

Random 값을 테스트 하기

결국에 랜덤 자체를 테스트하는 것은 불가능한것이 아닐까 하는 생각이 드는데

답변: 좋은 고민을 해주셨네요! 그 생각을 기반으로 테스트하기 불가능한 것과 테스트해야하는 것을 구분할 수 있습니다. 가령 랜덤 넘버가 4이상이면 전진한다.를 테스트할 때 랜덤넘버가 4이상인지를 테스트하기가 어렵고, 그래서 4이상 혹은 3이하인 상황이라고 가정하고 테스트하면 테스트를 쉽게 짤 수 있을 겁니다.

테스트하기 불가능한 대상과 테스트할 수 있는 대상을 구분하여 테스팅 하는 것이 매우 중요해요!

Controller에 대한 테스트 코드 작성

그런데 이 두 케이스 모두 이미 더 작은 단위에서 비슷한 테스트를 진행하고 컨트롤러에서는 이를 조합하는 거라고 생각해서 테스트 코드 작성은 따로 하지 않았습니다.

답변: 생각의 방향이 좋네요 :) 단위 테스트, 통합 테스트, 인수 테스트 등 테스팅에도 여러 종류가 있습니다. 지금 당장 고민하기에는 조금 벅찰 수 있으니 대충 이런 것이 있구나 하고 넘어가 주셔도 돼요 :) 일단 단위 테스트를 잘 작성하는 것부터 집중하여 훈련하는 것이 중요한 단계입니다 :)

상수 관리
Constant를 import 해가면서 써봤는데, 코드에 갑자기 다른 클래스의 상수가 나오면 알아보기에 불편하다는 생각

다른 클래스에서도 같은메시지를 재사용 할 수 있기 때문에 따로 관리하는게 좋을 것 같다는 의견을 냈습니다. 그리고 본인의 일화를 이야기해주었는데요, 실무에서 일할 때 담당자 번호가 에러 메시지가 하드코딩 되어 있었는데, 이 담당자 번호를 바꿔달라는 문의에 해당 메시지가 도메인에 있는지, 컨트롤러에 있는지, 컨트롤러에 있다면 어느 컨트롤러에 있는지 찾는 작업이 엄청 불편했다고 합니다. 만약 메시지가 분리되어 있다면 로직을 타고 가지 않아도 ErrorMessage와 같은 클래스에서 변경하면 되지 않을까

답변: 누구의 이야기도 틀리지 않았네요! 트레이드 오프의 영역입니다. 물론 에러 메시지가 재활용 되는 부분에 대해서는 추후에 다른 해결방법들도 많이 있겠지만, 저는 전역적인 상수를 그렇게 좋아하는 편은 아닙니다!
이유는 저도 어떤 클래스를 다양한 곳에서 마구잡이로 호출하는 것을 좋아하지 않고, 에러메시지의 중복은 다른 방법으로도 충분히 해결할 수 있다고 생각하기 때문입니다 :)

여기서부터는 제가 피드백을 받고 수정하면서 새롭게 적용해 본 부분에서 드리고 싶은 질문입니다!

최대한 getter를 지양하고자 한게 comment에서도 말씀드렸지만 getter로 뽑아낸 값이라도 final로 선언된 원시 값이 아닌 경우에는 수정이 가능하다는 점에서(예를 들어 getter로 꺼낸 List에 add를 하는 경우가 가능) 출력에 필요한 데이터도 필요한 전부 string으로 만들어서 전달했습니다! 그런데 제이가 말씀해주신대로 콘솔 게임이 아닌 경우에는 부적절한? toString인 것 같더라고요
그래서 수정을 하고자 했는데, 생각해보니 getter로 꺼낸 데이터를 임의로 조작할 수 있다는 점 때문에 지양한거라면, JPA에서 데이터를 계층간 이동시킬 때 Entity값을 넘겨주는 대신 그 Entity로 DTO를 만들어서 전달해주는 방법이 생각났습니다. 그래서 여기서도 원래 데이터(예를 들면 Car)를 변화시킬 수 없는 값을 꺼내주면 되지 않을까라는 생각이 들었습니다!
제가 그래서 최종적으로 고친 방법은 단순히 Car 객체의 이름, 위치 정보만 담는 DTO인 CarStatus를 만들어서 view로 전달해 주었습니다!
이 경우에 이렇게 DTO로 전달하는 방법은 좋은 방법일까요? 제이의 생각이 궁금합니다 :)

-> 저는 개인적으로 좋아하고, 실제로 자동차 경주게임을 만들 때 dto를 사용하여서 출력했어요. 그 당시에 그 부분이 오버엔지니어링이 아니냐는 피드백도 받았었는데, 저는 충분히 오버엔지니어링이라고 생각하면서도 그게 더 확장성 있는 코드라고 생각했어요. 본인이 컨트롤할 수 있는 수준이라면 최대한 확장성 있는 코드를 지향하는게 좋다고 생각합니다 :)

getter와 불변성에 대한 비슷한 질문인데, 다른 계층으로 데이터를 꺼내서 넘겨줄 때, 위에서 얘기한 CarStatus 객체를 주더라도 문제는 여러 객체를 넘겨줘서 List가 되었을 때 그 List 자체가 불변하지 않다는 점이 있더라고요. 그런데 view로 List를 넘겨주거나 하는 부분을 생각해보면 이 List 자체도 어떤 로직을 담은 것이 아니라 불변한 값만을 담고 있는 DTO라고 볼 수 있을 것 같아서, 전부 Collections.unmodifableList 자료구조로 바꿔주었는데, 이런 방법이 혹시 불필요한 로직일까요??

-> 전혀 불필요한 로직이라고 생각하지 않아요. 다만 현업에서 중간에 add해서 리스트를 변형한다든지 하시는 분이 거의 없기 때문에 레거시에서는 해당하는 형태로 바꿔둔 부분은 거의 찾기 힘들었는데, 저는 그런 조심스러움을 선호하는 편입니다 :)

커밋시에 "feat: Car" 객체 추가 와 같이 title만 적은 간략한 커밋 방법이 좋을까요 아니면 body와 footer까지 모두 적은 긴 커밋이 좋을까요?

-> 저는 긴 커밋을 선호해요 :) 물론 간결하게 의도를 다 전달할 수 있으면 좋긴하겠지만, 생각보다 한 줄에 모든 의도를 담기가 힘들 때도 많고, 코드로만 모든 것을 말하기 어려울 때도 많은 것 같아요. 커밋 내용을 상세하게 적어주면, 커뮤니케이션 비용이 줄어드는 부분은 확실히 있는 것 같아요!

답변은 여기까지이고, 추가로 코드 보면서 코멘트 남겼으니 확인 부탁드려요 :)
해당하는 내용들은 2단계를 진행하면서 고민해보시고 반영해주세요!
1단계 수고 많으셨고, 이만 머지하겠습니다 :)

jjj0611 · 2022-02-15T09:33:29Z

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

+    private Cars cars;
+    private TryCount tryCount;


이렇게 멤버 변수로 갖고 있는 경우 발생할 수 있는 문제에는 어떤게 있을까요??
해당 컨트롤러 메서드 전역에서 사용하려고 위와 같이 선언 한 것으로 보이는데,
어떤 문제가 있을 수 있는지 고민해보면 좋을 것 같아요 :)

이 피드백을 받고 데일리 미팅 같은 조 크루들과 논의했어요! 두번째 PR할 때 정리하겠습니다!

사실 이 부분은 레벨1 1단계에서 고민하기엔 조금 어려울 수 있는 내용이지만, 잘 고민해주신 것 같아요! :)

jjj0611 · 2022-02-15T09:34:31Z

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

+    private void inputTryCount() {
+        try {
+            tryCount = new TryCount(InputView.inputTryCount());
+        } catch (IllegalArgumentException | IOException e) {
+            OutputView.printException(e);
+            inputTryCount();
+        }
+    }


자동차 생성 로직과 다르게 initialize되고 있는 것 같은데 이유가 있을까요?

제가 의도하고자 한 바는 TryCount 생성 로직이었는데, Cars를 생성하는 과정이 통일되지 않았네요.
코드 통일성을 위해 Cars도 생성하는 메소드를 분리하도록 할게요!

jjj0611 · 2022-02-15T09:37:26Z

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

+        int currentTryCount = 0;
+        OutputView.printStartMessage();
+        while (tryCount.isNotSame(currentTryCount++)) {
+            cars.moveAll(RandomNumberGenerator.fromBounds(RANDOM_LOWER_BOUND, RANDOM_UPPER_BOUND));


0부터 9까지 랜덤한 숫자를 생성하여 4이상이 나오면 전진한다.

저는 0과 9라는 숫자가 랜덤한 숫자를 생성하는 책임이 있는 곳에 위치하는게 조금 더 좋다는 생각을 갖고 있습니다. 확률을 좀 높여서 1부터 10까지 중에 생성한다고 하면, 랜덤한 숫자를 생성하는 곳에 가서 변경하면 되니까요!

Controller에 있으면 어떤 장점이 있을까요??

제가 Controller에 둔 이유는 아무래도 게임 자체 규칙이기 때문에 가장 상위 클래스에서 관리하는 것이 맞지 않나 라는 생각을 했습니다!
그리고 만약 랜덤 숫자를 생성하는 위치에 해당 상수를 두게 되면 그 객체는 매 번 같은 범위의 난수만 생성할 수 있기 때문에, 규칙의 확장성을 위해서 RandomNumberGenerator가 boundary를 받아서 생성되도록 하느라 Controller에 두게 되었어요.

엇 게임 자체 규칙의 역할을 하는 친구가 RancomNumberGenerator가 될 수도 있지 않을까요?!
이것 또한 정답은 없습니다! :) 다만, Controller의 의미와 역할에 대해서 잘 고민해보시면 좋을 것 같아요!
Controller는 규칙을 관리해야 하는가?
규칙은 view일까요? model일까요?

jjj0611 · 2022-02-15T09:43:26Z

src/main/java/racingcar/model/car/Car.java

+    public boolean isMaxPosition(int maxPosition) {
+        return position.toInt() == maxPosition;
+    }


피드백 잘 반영해주셨네요!
그런데 만약 해당 메서드의 인자로 maxPosition이 아닌 minPosition이 들어오면 어떻게 되나요?
꼴등을 구하는 로직이 추가되어서 minPosition이 들어왔을 때 해당 메서드를 사용하면 헷갈리게 되지 않을까요?

좀 더 범용적이게 변경해볼까요? :)

이 부분은 메소드명을 변경해 보겠습니다 :D

jjj0611 · 2022-02-15T09:45:57Z

src/main/java/racingcar/model/car/Car.java

+    @Override
+    public String toString() {
+        return name + " : " + position;
+    }


toString이 여전히 존재하는데 어디서 사용하는 걸까요?

사용처가 없는데 그냥 넘어갔네요... 원래 사용하지 않는 메소드명은 회색 처리가 되는데 toString()은 그러지 않아서 무심코 넘어갔나봐요

jjj0611 · 2022-02-15T09:53:51Z

src/main/java/racingcar/model/value/Position.java

+    public int toInt() {
+        return this.position;
+    }


값 객체도 객체입니다! 이 메서드를 사용하여 위치를 비교하는 로직이 있는것 같은데,
객체에서 꺼내는게 아니라 메시지를 던져보면 어떨까요?

이 글을 참고해봐도 좋을 것 같아요 :)

정독해보겠습니다 :)

jjj0611 · 2022-02-15T09:54:08Z

src/main/java/racingcar/model/value/TryCount.java

+    public boolean isNotSame(int tryCount) {
+        return this.tryCount != tryCount;
+    }


메시지를 잘 던져주셨네요 :)

jjj0611 · 2022-02-15T09:55:15Z

src/main/java/racingcar/util/RandomNumberGenerator.java

+    public static RandomNumberGenerator fromBounds(int lowerBound, int upperBound) {
+        if (lowerBound > upperBound) {
+            return new RandomNumberGenerator(upperBound, lowerBound);
+        }
+        return new RandomNumberGenerator(lowerBound, upperBound);
+    }
+
+    public int generate() {
+        if (lowerBound == upperBound) {
+            return lowerBound;
+        }
+        return lowerBound + random.nextInt(upperBound - lowerBound);
+    }


static method로 bound를 받아서 랜덤 넘버를 생성하기도 하고,
인스턴스 생성시에 bound를 받아서 인스턴스화하여 랜덤 넘버를 생성하기도 하네요.

두 가지 모두 구현하신 이유가 있을까요?

이 부분에 정적 팩토리 메소드를 사용했습니다!
지금은 0~9 사이 같은 특정 범위 내에서 생성하지만, RandomNumberGenerator가 특정 boundary를 입력받아서 랜덤 값을 생성하는 것외에도 다른 규칙으로 랜덤 값을 생성하게 될 수도 있을 것 같아서, "boundary 값을 받아서 그 안에서 랜덤값을 생성한다" 라는 의미를 잘 드러낼 수 있도록 생성자에 이름을 붙이기 위해 사용했어요.

추가로 private한 생성자에서는 그냥 필드 할당만 해주고, fromBounds에서 피드백 내용이었던 범위 하한이 상한보다 크게 들어올 경우의 처리 로직을 넣어주었습니다!

jjj0611 · 2022-02-15T09:58:04Z

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

+    private static String createStatusString(CarStatus carStatus) {
+        return String.format("%s : ", carStatus.getName()) + DISTANCE_MARK.repeat(carStatus.getPosition());
+    }


String.format을 사용해주셨는데 뒤에는 그냥 더해서 출력하고 있네요.
진행 상태는 포멧팅 처리를 안하신 이유가 있나요?
포멧 문자열도 매직넘버 아닐까요??

이 피드백을 보고 해봤더니 "%s : %s"라고 하고 인자 두 개를 넣어주는걸로 format 처리가 가능했었네요
format을 거의 안써보다 보니 몰랐습니다 ㅠ

jjj0611 · 2022-02-15T09:58:31Z

src/test/java/racingcar/model/car/CarTest.java

+import org.junit.jupiter.params.provider.ValueSource;
+
+@SuppressWarnings("NonAsciiCharacters")
+class CarTest {


parameterized test의 활용 👍

Ohzzi added 30 commits February 9, 2022 17:12

test: 테스트 코드 작성

55d1d22

feat: 빈 문자열 또는 null 값이 입력될 경우 0 반환

290d21a

feat: 쉼표 구분자로 분리하여 숫자의 합 반환

3144381

refactor: 빈 문자열 체크와 합 구하는 메소드 분리

733cb66

feat: 구분자에 콜론 추가

31095bf

refactor: 문자열을 분리하는 메소드 분리

44d68a9

feat: 커스텀 구분자로 분리하는 메소드 추가

584357d

refactor: 분리된 문자를 정수로 바꾸는 메소드 분리

1c48fde

feat: 음수를 입력하면 RuntimeException 반환

ff7a2bd

refactor: 메소드 이름 변경 및 불필요한 메소드 제거

e7126bd

refactor: 하드코딩 제거 및 2개 이상의 역할을 하는 메소드 분리

38290f7

docs: 구현할 기능 리스트 정리

f88471f

feat: 이름을 검증하고 자동차 생성

830c456

feat: 자동차가 입력받은 값이 4 이상이면 전진

e302194

feat: 자동차의 이름과 위치를 문자로 나타내도록 toString() 작성

c4a9383

feat: 자동차들을 저장하는 Cars 클래스를 만들고 이름의 중복을 검사

8886041

feat: Cars의 실행 결과를 생성하는 toString() 메소드 재정의

e2ded86

refactor: Cars 생성할 때 배열 대신 String을 받은 뒤 split 하도록 수정

7973427

feat: 게임을 생성하고 자동차 이름을 입력

01241ca

feat: 레이싱 게임을 시도할 횟수 입력

f89d7ef

feat: 시도 횟수 값을 가지고 검증하는 TryCount 객체 생성

2c75526

feat: 랜덤 값을 생성하는 클래스 작성

43a0d91

feat: 시도 횟수 만큼 게임을 실행하고 매 차례마다 진행 상태를 출력

dde808c

feat: 우승자를 출력 후 게임 종료

f425b91

fix: 이름 입력 시 띄어쓰기 공백 제거

6e506ae

refactor: Car의 이름을 Name이라는 새로운 객체로 분리

18db939

refactor: Car의 위치값을 Position 객체로 분리

3d2adf2

refactor: 메소드 이름 변경(getName -> toString)

1e4a387

refactor: 에러 및 출력 메시지 상수 클래스 분리

9b8f467

refactor: 메소드명 직관적으로 변경 (validateBoundary -> validateLength)

81980ce

Ohzzi added 3 commits February 11, 2022 13:39

test: Non-ASCII 경고 무시하도록 추가

1735513

test: 중복되는 테스트 메소드 분리

ef099ee

style: 코드 컨벤션에 맞게 수정 (메소드 인자 명 o -> obj)

bf6c028

jjj0611 suggested changes Feb 12, 2022

View reviewed changes

Ohzzi added 22 commits February 12, 2022 15:35

style: 우테코 자바 컨벤션 적용

5c9e93a

docs: 요구 기능 재정리

4b638cf

feat: Car 객체의 이름, 위치 정보만 담는 DTO 객체인 CarStatus 추가

cb4d3bc

refactor: 게임 진행 상태를 나타낼 때 Cars에서 문자열을 생성하지 않고 DTO를 view로 넘겨서 view에서 처…

2ace942

…리하도록 수정

refactor: 우승자를 출력할 때 이름 정보를 view로 넘기고 view에서 출력할 문자열을 만들도록 수정

fd9d774

test: toString()을 사용하거나 toString() 자체를 테스트하는 로직 수정

c287e6d

test: 어노테이션 사용하여 테스트 통합

e521efd

chore: model 패키지를 자동차 객체들과 value 객체들로 패키지 분리

a3ea276

test: Winners의 테스트를 기존에 우승자가 나왔는지만 체크하던 테스트에서 단독 우승과 공동 우승을 체크하는 테스트로 변경

ac08d9f

refactor: 자동차가 최대 거리에 있는지 판단하는 로직을 Winners -> Car 로 이동

bf3e714

test: 자동차의 position이 입력받은 최대 position과 같은지 검사하는 테스트 추가

1727bf7

refactor: 자동차 상태나 우승자 정보와 같은 DTO의 리스트를 전달할 때 값을 조작할 수 없도록 불변 리스트로 만들도…

dd124ef

…록 수정

test: 불필요한 import 제거

4f3f7e3

style: public 메소드가 private 메소드보다 위에 오도록 수정

1173d96

feat: Position 객체에 equals(), hashCode() 오버라이드

339e253

refactor: 변수명 직관적으로 변경

3796744

nowTryCount 라는 단어가 말이 안되는 단어여서 currentTryCount로 변수명을 변경

refactor: TryCount가 음수일 시 출력하는 예외 메시지 변경

3773ae1

refactor(calculator): 문자열 format 사용

f4b7ccd

style(calculator): 메소드 순서 알아보기 쉽도록 조정

fef3d49

fix(calculator): 숫자가 아닌 문자열이 들어왔을 때 예외 처리

a8097f9

test(calculator): splitAndSum 테스트 시 null 값 테스트와 빈 문자 테스트 분리

243b82b

jjj0611 approved these changes Feb 15, 2022

View reviewed changes

jjj0611 merged commit 51eb1c2 into woowacourse:ohzzi Feb 15, 2022

Ohzzi mentioned this pull request Feb 16, 2022

[2단계 - 자동차 경주 리팩터링] 오찌(오지훈) 미션 제출합니다. #366

Merged

[1단계 - 자동차 경주 구현] 오찌(오지훈) 미션 제출합니다. #281

[1단계 - 자동차 경주 구현] 오찌(오지훈) 미션 제출합니다. #281

Conversation

Ohzzi commented Feb 11, 2022

Random 값을 테스트하기

Controller의 테스트는 작성하지 않아도 괜찮을까?

상수 관리

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

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

Ohzzi Feb 13, 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

Ohzzi commented Feb 14, 2022

jjj0611 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

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

jjj0611 left a comment •

edited

Loading

Ohzzi Feb 13, 2022 •

edited

Loading