Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Feb 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 28 additions & 30 deletions src/main/java/racingcar/controller/GameController.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,35 @@
import java.util.Map;
import racingcar.model.Car;
import racingcar.model.CarGroup;
import racingcar.model.RacingGame;
import racingcar.utils.NameParser;
import racingcar.utils.InputValidator;
import racingcar.view.InputView;
import racingcar.view.OutputView;

public class GameController {
private final CarGroup carGroup = new CarGroup();
private List<String> names;
private int moveCount;
private RacingGame racingGame;

public void init() throws IOException {
readCarNames();
readMoveCount();
initCars(names);
List<String> names;
int moveCount;

do {
names = readCarNames();
} while (!isNameValid(names));
do {
moveCount = readMoveCount();
} while (!isMoveCountValid(moveCount));
Comment on lines +21 to +26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

@hw0603 hw0603 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 모호하게 설명을 드렸었네요.
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;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


racingGame = new RacingGame(createCarGroup(names), moveCount);
}

private void readCarNames() throws IOException {
boolean isValid = false;
while (!isValid) {
isValid = doReadCarNames();
}
private List<String> readCarNames() throws IOException {
return NameParser.parse(InputView.inputNames());
}

private boolean doReadCarNames() throws IOException {
private boolean isNameValid(List<String> names) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@hw0603 hw0603 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
OutputView.printlnInputName();
names = NameParser.parse(InputView.inputNames());
InputValidator.validateCarName(names);
} catch (IllegalArgumentException e) {
OutputView.printException(e.getMessage());
Expand All @@ -40,17 +42,8 @@ private boolean doReadCarNames() throws IOException {
return true;
}

private void readMoveCount() throws IOException {
boolean isValid = false;
while (!isValid) {
isValid = doReadMoveCount();
}
}

private boolean doReadMoveCount() throws IOException {
private boolean isMoveCountValid(int moveCount) {
try {
OutputView.printlnInputMoveCount();
moveCount = InputView.inputMoveCount();
InputValidator.validateMoveCount(moveCount);
} catch (IllegalArgumentException e) {
OutputView.printException(e.getMessage());
Expand All @@ -59,22 +52,27 @@ private boolean doReadMoveCount() throws IOException {
return true;
}

private void initCars(List<String> carNames) {
private int readMoveCount() throws IOException {
return InputView.inputMoveCount();
}

private CarGroup createCarGroup(List<String> carNames) {
CarGroup carGroup = new CarGroup();

for (String name : carNames) {
carGroup.add(new Car(name));
}
return carGroup;
}

public void play() {
OutputView.printResultDescription();
for (int i = 0; i < moveCount; i++) {
Map<String, Integer> raceResponse = carGroup.race();
OutputView.printPosition(raceResponse);
}
List<Map<String, Integer>> raceResponse = racingGame.race();
OutputView.printPosition(raceResponse);
Comment on lines +70 to +71
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#700 (comment)

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

public void finish() {
List<String> winners = carGroup.findWinners();
List<String> winners = racingGame.findWinners();
if (winners.isEmpty()) {
OutputView.printNoWinner();
return;
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/racingcar/model/CarGroup.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ public void add(Car car) {
cars.add(car);
}

public Map<String, Integer> race() {
Map<String, Integer> raceResponse = new HashMap<>();
public Map<String, Integer> playRound() {
Map<String, Integer> roundResponse = new HashMap<>();
for (Car car : cars) {
int nextPosition = car.move(RandomNumberGenerator.generate());
raceResponse.put(car.getName(), nextPosition);
roundResponse.put(car.getName(), nextPosition);
}

return raceResponse;
return roundResponse;
}

public List<String> findWinners() {
Expand Down
25 changes: 25 additions & 0 deletions src/main/java/racingcar/model/RacingGame.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package racingcar.model;

import java.util.List;
import java.util.Map;
import java.util.stream.IntStream;

public class RacingGame {
CarGroup cars;
int moveCount;

public RacingGame(CarGroup cars, int moveCount) {
this.cars = cars;
this.moveCount = moveCount;
}

public List<Map<String, Integer>> race() {
return IntStream.range(0, moveCount)
.mapToObj(c -> cars.playRound())
.toList();
}
Comment on lines +16 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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


public List<String> findWinners() {
return cars.findWinners();
}
}
10 changes: 8 additions & 2 deletions src/main/java/racingcar/utils/InputValidator.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
import java.util.List;

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

private InputValidator() {
}

Comment on lines 5 to +11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public static void validateCarName(List<String> names) {
validateDuplicateNames(names);
for (String name : names) {
Expand All @@ -12,7 +18,7 @@ public static void validateCarName(List<String> names) {
}

private static void validateNameLength(String name) {
if (name.length() > 5) {
if (name.length() > NAME_LENGTH_LIMIT) {
throw new IllegalArgumentException("자동차의 이름은 5글자를 초과할 수 없습니다.");
}
}
Expand All @@ -28,7 +34,7 @@ private static void validateDuplicateNames(List<String> names) {
}

private static void validateNameCharacters(String name) {
if (!name.matches("^[a-zA-Z]*$")) {
if (!name.matches(NAME_REGEX_PATTERN)) {
throw new IllegalArgumentException("자동차의 이름은 영어로만 이루어져야 합니다.");
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/racingcar/utils/NameParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
public class NameParser {
private static final String DELIMITER = ",";

private NameParser() {
}
hw0603 marked this conversation as resolved.
Show resolved Hide resolved

public static List<String> parse(String rawNames) {
String[] names = rawNames.split(DELIMITER);
return Arrays.stream(names)
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/racingcar/utils/RandomNumberGenerator.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
public class RandomNumberGenerator {
private static final int RANDOM_BOUND = 10;

private RandomNumberGenerator() {
}

public static int generate() {
Random random = new Random();
return random.nextInt(RANDOM_BOUND);
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/racingcar/view/InputView.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,22 @@
import java.io.InputStreamReader;

public class InputView {
private static final String NAME_INPUT_DESCRIPTION = "경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).";
private static final String MOVE_COUNT_INPUT_DESCRIPTION = "시도할 회수는 몇회인가요?";

private static final BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

// 인스턴스화 방지
private InputView() {
}
hw0603 marked this conversation as resolved.
Show resolved Hide resolved

public static String inputNames() throws IOException {
System.out.println(NAME_INPUT_DESCRIPTION);
return br.readLine();
}

public static int inputMoveCount() throws IOException {
System.out.println(MOVE_COUNT_INPUT_DESCRIPTION);
String moveCount = br.readLine();
return Integer.parseInt(moveCount);
}
Expand Down
38 changes: 19 additions & 19 deletions src/main/java/racingcar/view/OutputView.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,45 @@ public class OutputView {
private static final String WINNER_DESCRIPTION = "가 최종 우승했습니다.";
private static final String RESULT_DESCRIPTION = "실행 결과";
private static final String NO_WINNER_DESCRIPTION = "최대 이동 거리가 0이므로 우승한 자동차가 없습니다.";
private static final String NAME_INPUT_DESCRIPTION = "경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).";
private static final String MOVE_COUNT_INPUT_DESCRIPTION = "시도할 회수는 몇회인가요?";

private static final String POSITION_METER = "-";
private static final String CAR_NAME_DELIMITER = ",";
private static final String NEW_LINE = System.lineSeparator();

// 인스턴스화 방지
private OutputView() {
}

public static void printResultDescription() {
System.out.println();
System.out.println(RESULT_DESCRIPTION);
}

public static void printPosition(Map<String, Integer> carPositions) {
private static String createRoundResponseMessage(Map<String, Integer> roundResponse) {
StringBuilder sb = new StringBuilder();
carPositions.forEach(
(name, position) -> sb.append(name).append(":").append(POSITION_METER.repeat(position)).append("\n")
roundResponse.forEach(
(name, position) -> sb.append(name).append(":").append(POSITION_METER.repeat(position)).append(NEW_LINE)
);
System.out.println(sb);
sb.append(NEW_LINE);
return sb.toString();
}

public static void printPosition(List<Map<String, Integer>> raceResponse) {
StringBuilder sb = new StringBuilder();
raceResponse.forEach(r -> sb.append(createRoundResponseMessage(r)));
System.out.print(sb);
}

public static void printException(String message) {
System.out.println(message);
}

public static void printWinnerList(List<String> winners) {
List<String> names = winners.stream()
.toList();

String winnerNames = String.join(", ", names);
List<String> names = winners.stream().toList();
String winnerNames = String.join(CAR_NAME_DELIMITER, names);
System.out.println(winnerNames + WINNER_DESCRIPTION);
}

public static void printNoWinner() {
System.out.println(NO_WINNER_DESCRIPTION);
}

public static void printlnInputName() {
System.out.println(NAME_INPUT_DESCRIPTION);
}

public static void printlnInputMoveCount() {
System.out.println(MOVE_COUNT_INPUT_DESCRIPTION);
}
}
2 changes: 1 addition & 1 deletion src/test/java/racingcar/model/CarGroupTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void findWinnerTest() {
neo.move(moveNumber);
pedro.move(moveNumber);

assertThat(carGroup.findWinners()).isEqualTo(List.of(neo));
assertThat(carGroup.findWinners()).isEqualTo(List.of("네오"));
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/racingcar/model/CarTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ void moveTest() {
@DisplayName("생성된 난수가 이동 기준을 만족하지 않을 때 위치는 변하지 않는다.")
void notMoveTest() {
final Car car = new Car("몰리");
final int randomNumber = 2;
final int randomNumber = 3;

car.move(randomNumber);

Expand Down