[2단계 - 자동차 경주 리팩터링] 페드로(류형욱) 미션 제출합니다. #757

hw0603 · 2024-02-18T07:45:19Z

안녕하세요 아서! 주말인데 빠르고 자세한 피드백 감사했어요.

말씀하셨던 부분들 반영해서 2단계 PR을 작성합니다.
2단계 미션에서는 'MVC 구조로의 리팩토링'이 주제였는데, 1단계 미션 진행 당시 MVC 구조를 채택하였어서 큰 구조 변경 없이 제출해요. 첫 리뷰에서 언급하셨던 '출력을 담당하는 클래스가 도메인을 알아야 할까?' 에 대한 부분은 1단계에서 수정 완료하여 이미 머지된 상태입니다!

변수/메서드명 변경이나 상수 포장 등을 제외하면, 주요 변경점은 다음 두 가지로 정리할 수 있을 것 같아요.

입력 재 시도 로직 구현의 변경

기존 구현에서 메서드명이 게터와 혼동되거나 메서드의 역할을 정확히 드러내지 못하는 문제가 있었고, 재 시도를 위한 플래그 변수의 이름 역시 모호했었는데요.

플래그 값이 왜 필요했는지 한번 고민해보시면 좋을 것 같아요.
혹시 메서드가 두가지 기능을 하고 있지는 않나요?
현재 단순히 이름을 읽어오는 역할만 하고 있나요?

이 말에서 힌트를 얻어 반복 구조를 바꾸고 플래그 변수를 제거해 보았습니다.
다만, 일반적으로 잘 사용하지 않았던 do~while 구문으로 반복을 구현했는데, 이것에 대한 아서의 생각은 어떤지 궁금해요. 지양해야 하는 부분일까요? 아니면 사용했을 때의 코드가 좀 더 읽기 편해진다면 걱정 없이 사용해도 되는 걸까요?

`RacingGame` 클래스의 추가

입력 재 시도 로직을 변경하면서 '입력받은 이름과 이동 횟수를 과연 컨트롤러의 필드로 가지고 있어야 하는가?' 에 대한 고민이 있었습니다. 필드에 저장해 두었던 이유를 생각해 보니 입력받는 메서드와 레이싱 라운드를 반복해 주는 라운드가 분리되어 있어, 값 참조를 위한 저장 그 이상도 이하도 아니더라구요.
이런 이유라면 차라리 자동차 그룹과 라운드 진행 횟수를 가지는 RacingGame 객체를 도입하고, 컨트롤러에서는 RacingGame에 .race() 명령만 내린 후 결과를 받아와 뷰에 전달하는 역할만 수행하는 것이 좀 더 깔끔하겠다는 생각이 들어 해당 구조로 변경했습니다.

결과적으로 CarGroup은 Car의 리스트를 래핑한 일급 컬렉션으로서, 승자를 찾고 한 라운드를 진행하는 책임을 갖게 됩니다. 반면 RacingGame은 자신의 moveCount를 참조하여 CarGroup에게 해당 횟수만큼 라운드를 반복하게 하는 역할을 하구요.
컨트롤러에서는 RacingGame을 사용하기에 RacingGame.findWinners()는 단순히 CarGroup.findWinners()를 반환하도록 되어 있습니다.

public List<String> findWinners() {
    return cars.findWinners();
}

단순히 점프를 띄워 주는(?) 이러한 구현에 문제가 없을지 궁금하고, 이 경우 RacingGame에서 테스트할 수 있는 것은 "반복문이 잘 돌아가는가?" 와 "carGroup.findWinners() 가 잘 호출되고 반환되는가?" 정도 밖에 없을 것 같아 해당 클래스에 대한 테스트 코드는 작성하지 않았는데, 아서의 생각은 어떤지 궁금합니다!

- 과도한 메서드 분리 제거 - 입력을 검증하는 부분과 입력을 받는 부분 메서드 분리 - read~() 메서드는 입력된 값을 반환하도록 시그니처 수정

- RacingGame 클래스가 추가됨에 따라 메서드 명을 보다 직관적으로 수정

- 모든 라운드가 종료된 후 일괄적으로 라운드 별 자동차의 위치를 출력하도록 수정

- 개행 문자를 System 모듈에서 참조하도록 변경

hw0603 · 2024-02-18T07:55:43Z

src/main/java/racingcar/controller/GameController.java

+        List<Map<String, Integer>> raceResponse = racingGame.race();
+        OutputView.printPosition(raceResponse);


#700 (comment)

2단계 리팩터링을 하면서 컨트롤러의 역할이 무엇인지 한번 고민해보시면 1,2번에 대한 고민이 해결될 수 있을 것 같아요.
어떤 부분이 깔끔해보이지 않았을까요?

제가 깔끔해 보이지 않는다고 생각했던 부분이 이 부분이였는데요, 제가 생각하는 컨트롤러의 역할은 '독립되어 있는 도메인과 뷰를 이어주는 것' 입니다. 때문에 비즈니스 로직 처리 후 응답받은 결과를 뷰에서 출력할 수 있는 형태로 가공하는 것은 컨트롤러의 책임이 맞다고 생각했어요.
다만, '뷰에서 출력할 수 있는 형태'가 List<Map<String, Integer>>와 같은 구조가 되는 것은 별로라는 생각이 들어요. 다른 크루들을 보니 DTO를 사용하여 하나의 객체로 관리하는 방식을 사용하는 경우도 있던데 처음에는 이 정도의 규모에 DTO까지 필요할까? 라는 생각이 들어 일부러 사용하지 않았거든요.
아서는 기본 자료구조로 정보를 전달할지, 별도의 DTO를 사용할지를 판단하는 본인만의 기준이 있으신가요?

저도 지금은 DTO까지 고려할 구조는 아니라고 생각합니다.
List<Map<String,Integer>> 같은 복잡한 Collection을 반환해주는 것 또한 부적절하다고 생각해요.
그렇기 때문에 메서드를 적절히 분리하는 과정이 필요합니다.
지금 왜 List<Map<String,Integer>> 구조를 반환하게 됐고, 나눠서 보낼수는 없는 상황이었는지 한번 고민해보시면 좋을 것 같아요.

DTO 이야기는 나중에 레벨2 때 실컷 하실 것이기 때문에 그때까지 아껴놓기로해요 ㅎㅎ

Hyunta

안녕하세요 페드로,
말씀해주신 것처럼 이미 구조가 잘 잡혀있었기 떄문에 리팩터링할 요소가 많지는 않았네요.
관련해서 몇가지 코멘트를 남겨놨으니 한번 확인 부탁드릴게요!

Hyunta · 2024-02-18T13:25:00Z

src/main/java/racingcar/controller/GameController.java

+        do {
+            names = readCarNames();
+        } while (!isNameValid(names));
+        do {
+            moveCount = readMoveCount();
+        } while (!isMoveCountValid(moveCount));


값을 불러오는 기능이 별개의 메서드로 왜 빠지지 못했을까요?

현재는 값을 선언해주는 부분과 값을 불러오는 부분이 같이 있어요 -> names = readCarNames();
선언 부분과 호출 부분을 분리하면 별개의 메서드로 분리할 수 있지 않을까요?

어.. 무슨 말씀이신지 잘 이해하지 못했습니다. 반복 구조를 별개의 메서드(foo())로 빼고, names = foo() 형태로 구현하라는 말씀이실까요?

제가 모호하게 설명을 드렸었네요.
GameContoller의 init() 메서드 안에 해당 구조에 대한 내용입니다.
do-while 문이 두번이나 반복되기 때문에 가독성이 떨어져, 메서드를 분리하고자 하는 목적에서 피드백을 드렸습니다.

List<String> names; do { names = readCarNames(); } while (!isNameValid(names));

이 상태에서 do~while 구문을 메서드로 뽑아내려고 하면 어렵겠죠.
그런데 만약 do~while 구문이 값을 선언하는 것이 아니라 return 하는 형태가 된다면 분리가 가능하겠죠.

List<String> carNames = getCarNames() private final List<String> getCarNames(){ List<String> names; do { names = readCarNames(); } while (!isNameValid(names)); return names; }

아하 이해했습니다. 1단계 초기 구현은 말씀하신 대로 구현했다가 단순히 반복해 주는 일만 하는 메서드인 것 같아서 다시 init() 안에서 구현했었는데, do while이 여러 번 반복되면 가독성이 떨어질 수 있겠네요.

Hyunta · 2024-02-18T13:26:16Z

src/main/java/racingcar/controller/GameController.java

    }

-    private boolean doReadCarNames() throws IOException {
+    private boolean isNameValid(List<String> names) {


이름을 검증하는 방식을 Controller가 알고 있어야 할까요?

재 입력 루프를 만들기 위한 메서드였긴 한데, 검증 로직을 컨트롤러가 가지지 않고 validator를 호출만 하고 있는데 이러한 구조도 Controller가 검증하는 방식을 알고 있다고 보는 건가요..?

Controller 입장에서는 검증한다는 사실 조차 알 필요가 없지 않을까요?
그냥 view 단에서 입력값을 받아와서, model 단으로 던져주면 될 것 같은데
현재 페드로의 controller에서는 어떤 객체로 검증하는지까지 알 수 있어요.
꼭 필요한 부분일까요?

확실히 컨트롤러의 역할을 생각해보면 부가적인 부분인 것 같아요. 별도의 validator를 두기보다는 아예 객체 생성 시 검증하는 형태가 더 나을 수도 있겠네요. 다음 미션 설계 시 고려해 보겠습니다!

Hyunta · 2024-02-18T13:29:01Z

src/main/java/racingcar/utils/InputValidator.java

 public class InputValidator {
+    private static final int NAME_LENGTH_LIMIT = 5;
+    private static final String NAME_REGEX_PATTERN = "^[a-zA-Z]*$";
+
+    private InputValidator() {
+    }
+


해당 클래스는 util성 클래스 인가요?
페드로가 생각할 때 util 패키지에 담길 수 있는 클래스는 어떤게 있나요?

util 패키지에는 특정 도메인에 종속적이지 않으면서, 프로젝트 전반적으로 사용하는 클래스들을 담아두었던 것 같아요. 특정 클래스를 검증하는 validator가 있다면 util에 있으면 안 되겠지만, InputValidator는 검증에 관한 로직들이 모두 포함되어 있으므로 util이 맞다고 생각하는데 아서의 생각은 어떤가요?

util 패키지에는 특정 도메인에 종속적이지 않으면서, 프로젝트 전반적으로 사용하는 클래스들을 담아두었던 것 같아요.
이 말에 동의합니다.
그런데 이름의 길이 제한, 자동차 이름의 중복 제한, 이동횟수의 자연수 검증은 도메인과 밀접한 관계에 있는 것 같아요.

저는 util 클래스의 사용을 최대한 지양합니다. 혹여나 사용을 하더라도 어느정도 규모가 커졌을 때 반복적으로 구현해야하는 기능들을 생겼을 때 만드는 편이에요. 그마저도 예를 들어 camelCase를 snakeCase로 변환해야한다던지 말 그대로 어디서나 사용해야하는 클래스들만 만들려고 합니다. 앞으로 미션을 진행하면서 util 클래스로 풀어내기전에 페드로가 말한 것 처럼 특정 도메인에 종속적이지 않은지잘 고민해보면 util 클래스가 필요하지 않은 상황일 수도 있습니다.

참고자료:
https://stackoverflow.com/questions/3340032/are-utility-classes-evil

'특정' 도메인에 종속된다 가 아니라 '어떤(any)' 도메인에 종속되는지 여부가 초점이였군요. 생각해 보면 프리코스 때와 마찬가지로 큰 고민 없이 util 패키지를 만들고 봤던 것 같아요. 생각하지 못했던 관점에서의 말씀과 참고 링크 감사합니다!

src/main/java/racingcar/utils/NameParser.java

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

Hyunta · 2024-02-18T13:35:41Z

src/main/java/racingcar/controller/GameController.java

+        List<Map<String, Integer>> raceResponse = racingGame.race();
+        OutputView.printPosition(raceResponse);


저도 지금은 DTO까지 고려할 구조는 아니라고 생각합니다.
List<Map<String,Integer>> 같은 복잡한 Collection을 반환해주는 것 또한 부적절하다고 생각해요.
그렇기 때문에 메서드를 적절히 분리하는 과정이 필요합니다.
지금 왜 List<Map<String,Integer>> 구조를 반환하게 됐고, 나눠서 보낼수는 없는 상황이었는지 한번 고민해보시면 좋을 것 같아요.

DTO 이야기는 나중에 레벨2 때 실컷 하실 것이기 때문에 그때까지 아껴놓기로해요 ㅎㅎ

Hyunta · 2024-02-18T13:50:09Z

일반적으로 잘 사용하지 않았던 do~while 구문으로 반복을 구현했는데, 이것에 대한 아서의 생각은 어떤지 궁금해요. 지양해야 하는 부분일까요? 아니면 사용했을 때의 코드가 좀 더 읽기 편해진다면 걱정 없이 사용해도 되는 걸까요?

do~while 혹은 try-catch 를 통한 재귀호출은 요구사항에 따라서 어쩔 수 없는 부분이라고 생각합니다.
그런데 재시도를 하기 위해서 플래그 값이 꼭 필요하다고 생각하지는 않아요. 지금은 값을 불러오는데 실패했을 때 항상 예외가 발생하고, 재시도 처리가 항상 일어나니까 catch를 통해서 충분히 해결가능한 상황이라고 보여집니다.

단순히 점프를 띄워 주는(?) 이러한 구현에 문제가 없을지 궁금하고, 이 경우 RacingGame에서 테스트할 수 있는 것은 "반복문이 잘 돌아가는가?" 와 "carGroup.findWinners() 가 잘 호출되고 반환되는가?" 정도 밖에 없을 것 같아 해당 클래스에 대한 테스트 코드는 작성하지 않았는데, 아서의 생각은 어떤지 궁금합니다!

저는 RaicingGame을 추가한건 좋은 선택이었다고 생각됩니다. 비즈니스 로직을 캡슐화할 수 있어서 Controller에서는 비즈니스 로직을 몰라도 추상화된 메서드만 호출해주면 되는 구조가 만들어졌으니까요.
그러다보면 페드로가 말한것처럼 흐름을 제어하는 메서드에 대해서 테스트을 해야하나? 라는 의문이 들 수 있는데, 저는 이런 테스트는 만들지 않으려고 합니다. 그런데 이건 정말 사람마다 다르기 때문에 절대로 제 생각에 동의하실 필요는 없어요.

hw0603 · 2024-02-19T05:02:40Z

저도 지금은 DTO까지 고려할 구조는 아니라고 생각합니다.
List<Map<String,Integer>> 같은 복잡한 Collection을 반환해주는 것 또한 부적절하다고 생각해요.
그렇기 때문에 메서드를 적절히 분리하는 과정이 필요합니다.
지금 왜 List<Map<String,Integer>> 구조를 반환하게 됐고, 나눠서 보낼수는 없는 상황이었는지 한번 고민해보시면 좋을 것 같아요.

라운드를 반복 진행하는 역할을 가지는 RacingGame이 생기면서, 컨트롤러에서는 RacingGame에 "전체 라운드를 진행해" 라는 메시지를 보내는 구조가 돼서 결과값도 전체 라운드의 결과를 한 번에 받아올 수 밖에 없었던 것 같아요.

좋은 선택이라고 말씀 주셨던 RacingGame을 도입하지 않거나 객체로 감싸서 List<RoundResponse>형태로 DTO...로 활용하는 방법밖에 생각이 나지 않는데 혹시 힌트를 좀 더 받을 수 있을까요..ㅎㅎ

Hyunta · 2024-02-19T11:01:30Z

src/main/java/racingcar/model/RacingGame.java

+    public List<Map<String, Integer>> race() {
+        return IntStream.range(0, moveCount)
+                .mapToObj(c -> cars.playRound())
+                .toList();
+    }


좋은 선택이라고 말씀 주셨던 RacingGame을 도입하지 않거나 객체로 감싸서 List<RoundResponse>형태로 DTO...로 활용하는 방법밖에 생각이 나지 않는데 혹시 힌트를 좀 더 받을 수 있을까요..ㅎㅎ

동시에 여러 코드를 보다보니 RacingGame에moveCount` 까지 있는걸 놓쳤네요.
지금 현재 구조에서는 나눠서 처리하기가 애매하네요.

제가 피드백 드리려고 했던 부분은 외부에서 각 라운드마다 처리하고 매번 출력을 하면 이중 Collection을 제거할 수 있기 때문에 그쪽으로 안내해드리려고 한 의도였습니다.

Hyunta

페드로 2단계 요구사항을 다 충족시켜서 이만 머지하도록 하겠습니다.
제 의견을 너무 직설적으로 말하면 그걸 정답으로 생각하실 것 같아서,
최대한 생각을 이끌어내려고 했는데 잘 됐는지 모르겠네요ㅎㅎ

모호하게 피드백 해드린점 양해바랍니다.
피드백이 이해가 안되거나, 궁금한 점이 있으면DM 주세요.

hw0603 · 2024-02-19T14:12:47Z

페드로 2단계 요구사항을 다 충족시켜서 이만 머지하도록 하겠습니다. 제 의견을 너무 직설적으로 말하면 그걸 정답으로 생각하실 것 같아서, 최대한 생각을 이끌어내려고 했는데 잘 됐는지 모르겠네요ㅎㅎ

모호하게 피드백 해드린점 양해바랍니다. 피드백이 이해가 안되거나, 궁금한 점이 있으면DM 주세요.

친절한 리뷰 감사했습니다🙂 아서 덕분에 당연하다고 생각하고 넘어갔던 것들도 한번 더 고민해 보는 계기가 됐던 것 같아요. 좀 더 공부해 보고, 질문드릴게 있으면 DM 드리겠습니다! ㅎㅎ

hw0603 added 11 commits February 17, 2024 23:46

fix(carTest): 테스팅 값을 경계값으로 수정

3771b25

refactor(gameController): 입력 재 시도 로직 구현 개선

6f956b9

- 과도한 메서드 분리 제거 - 입력을 검증하는 부분과 입력을 받는 부분 메서드 분리 - read~() 메서드는 입력된 값을 반환하도록 시그니처 수정

refactor(racingGame): 라운드를 반복하는 역할을 할 클래스 추가

84e5cda

refactor(carGroup): 메서드명 변경 (race -> playRound)

213cea5

- RacingGame 클래스가 추가됨에 따라 메서드 명을 보다 직관적으로 수정

refactor(gameController): RacingGame 에서 게임을 관리하도록 변경

bbba032

- 모든 라운드가 종료된 후 일괄적으로 라운드 별 자동차의 위치를 출력하도록 수정

refactor(outputView): printPosition()이 모든 라운드의 위치를 출력하도록 변경

238304c

- 개행 문자를 System 모듈에서 참조하도록 변경

refactor(outputView): 자동차 이름 출력 시 구분자 상수 포장

7a6cda9

refactor(inputView): 사용자 입력 안내 문구는 InputView 에서 처리하도록 변경

5864569

refactor(/view, /utils): 정적 메서드만을 가지는 클래스의 인스턴스화 방지

f38b89a

refactor(inputValidator): 검증에 사용되는 상수값 포장

f40aba2

fix(CarGroupTest): 우승자 판별 메서드의 반환형 변경에 따른 테스트 값 수정

7085a22

hw0603 commented Feb 18, 2024

View reviewed changes

hw0603 changed the title ~~[2단계 - 자동차 경주] 페드로(류형욱) 미션 제출합니다.~~ [2단계 - 자동차 경주 리팩터링] 페드로(류형욱) 미션 제출합니다. Feb 18, 2024

Hyunta requested changes Feb 18, 2024

View reviewed changes

Hyunta reviewed Feb 19, 2024

View reviewed changes

Hyunta approved these changes Feb 19, 2024

View reviewed changes

Hyunta merged commit 4228c2a into woowacourse:hw0603 Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 자동차 경주 리팩터링] 페드로(류형욱) 미션 제출합니다. #757

[2단계 - 자동차 경주 리팩터링] 페드로(류형욱) 미션 제출합니다. #757

hw0603 commented Feb 18, 2024 •

edited

Loading

hw0603 Feb 18, 2024

Hyunta Feb 18, 2024

Hyunta left a comment

Hyunta Feb 18, 2024

hw0603 Feb 19, 2024 •

edited

Loading

Hyunta Feb 19, 2024

hw0603 Feb 19, 2024

Hyunta Feb 18, 2024

hw0603 Feb 19, 2024 •

edited

Loading

Hyunta Feb 19, 2024

hw0603 Feb 19, 2024

Hyunta Feb 18, 2024

hw0603 Feb 19, 2024

Hyunta Feb 19, 2024

hw0603 Feb 19, 2024

Hyunta Feb 18, 2024

Hyunta commented Feb 18, 2024

hw0603 commented Feb 19, 2024 •

edited

Loading

Hyunta Feb 19, 2024

Hyunta left a comment

hw0603 commented Feb 19, 2024

		List<Map<String, Integer>> raceResponse = racingGame.race();
		OutputView.printPosition(raceResponse);

[2단계 - 자동차 경주 리팩터링] 페드로(류형욱) 미션 제출합니다. #757

[2단계 - 자동차 경주 리팩터링] 페드로(류형욱) 미션 제출합니다. #757

Conversation

hw0603 commented Feb 18, 2024 • edited Loading

입력 재 시도 로직 구현의 변경

RacingGame 클래스의 추가

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyunta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Feb 19, 2024 • 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

hw0603 Feb 19, 2024 • 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

Hyunta commented Feb 18, 2024

hw0603 commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Hyunta left a comment

Choose a reason for hiding this comment

hw0603 commented Feb 19, 2024

hw0603 commented Feb 18, 2024 •

edited

Loading

`RacingGame` 클래스의 추가

hw0603 Feb 19, 2024 •

edited

Loading

hw0603 Feb 19, 2024 •

edited

Loading

hw0603 commented Feb 19, 2024 •

edited

Loading