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단계 - 자동차 경주 리팩토링] 연로그(권시연) 미션 제출합니다. #419

Merged
merged 20 commits into from
Feb 22, 2022
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
7 changes: 6 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,19 @@
- 공동 우승은 ,로 구분해 출력
(ex: 오찌, 연로그가 최종 우승했습니다.)

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

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

Choose a reason for hiding this comment

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

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

- domain 패키지의 객체는 view 패키지 객체에 의존하지 않는다.
- 테스트 가능/불가능 부분을 분리해 테스트 가능 부분에 대해서만 단위 테스트

## 출력 예제
```
경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/racingcar/Application.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package racingcar;

import racingcar.controller.RacingController;
import racingcar.domain.strategy.MoveStrategy;
import racingcar.domain.strategy.RandomMoveStrategy;

public class Application {
public static void main(String[] args) {
RacingController racingController = new RacingController();
racingController.start();
MoveStrategy moveStrategy = new RandomMoveStrategy();
racingController.start(moveStrategy);
}
}
34 changes: 18 additions & 16 deletions src/main/java/racingcar/controller/RacingController.java
Original file line number Diff line number Diff line change
@@ -1,45 +1,47 @@
package racingcar.controller;

import racingcar.model.Cars;
import racingcar.model.TryCount;
import racingcar.domain.Cars;
import racingcar.domain.TryCount;
import racingcar.domain.strategy.MoveStrategy;
import racingcar.view.InputView;
import racingcar.view.OutputView;

public class RacingController {
private Cars cars;
private TryCount tryCount;
Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

@yeon-06 yeon-06 Feb 20, 2022

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

public void start(MoveStrategy moveStrategy) {
Cars cars = inputCars();

Choose a reason for hiding this comment

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

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

TryCount tryCount = inputTryCount();
race(cars, tryCount, moveStrategy);
printRaceResult(cars);
}

public void start() {
private Cars inputCars() {
try {
cars = new Cars(InputView.inputCarNames());
inputTryCount();
race();
terminate();
return new Cars(InputView.inputCarNames());
} catch (IllegalArgumentException e) {
OutputView.printException(e);
start();
return inputCars();
}
}

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

private void race() {
private void race(Cars cars, TryCount tryCount, MoveStrategy moveStrategy) {
int nowTryCnt = 0;
OutputView.printStartMessage();
while (tryCount.isNotSame(nowTryCnt++)) {
cars.moveAll();
cars.moveAll(moveStrategy);
OutputView.printCarsStatus(cars.getCars());

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

}
}

private void terminate() {
private void printRaceResult(Cars cars) {
OutputView.printCarsStatus(cars.getCars());
OutputView.printWinners(cars.getWinners());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package racingcar.model;
package racingcar.domain;

public class Car {
private static final int MOVING_DISTANCE = 1;
private static final int STANDARD_VALUE = 4;
import racingcar.domain.strategy.MoveStrategy;

public class Car {
private final Name name;
private final Position position;

Expand All @@ -12,10 +11,8 @@ public Car(String nameString) {
this.position = new Position();
}

public void goOrStop(int random) {
if (random >= STANDARD_VALUE) {
position.move(MOVING_DISTANCE);
}
public void go(MoveStrategy strategy) {
position.move(strategy.move());
}

public String getName() {
Expand All @@ -26,4 +23,11 @@ public int getPosition() {
return this.position.toInt();
}

@Override
public String toString() {
return "Car{" +
"name=" + name +
", position=" + position +
'}';
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
package racingcar.model;
package racingcar.domain;

import static racingcar.util.StringUtils.splitByDelimiter;
import static racingcar.util.StringUtils.stripStringArray;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import racingcar.domain.strategy.MoveStrategy;
import racingcar.message.ErrorMessages;
import racingcar.util.RandomGenerator;
import racingcar.util.RandomUtils;

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<>();

Choose a reason for hiding this comment

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

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


Expand All @@ -24,9 +24,9 @@ public Cars(String carNames) {
}
}

public void moveAll() {
public void moveAll(MoveStrategy strategy) {
for (Car car : cars) {
car.goOrStop(RandomGenerator.generateNumber(MIN, MAX + 1));
car.go(strategy);
}
}

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

@Override
public String toString() {

Choose a reason for hiding this comment

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

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

Copy link
Author

@yeon-06 yeon-06 Feb 20, 2022

Choose a reason for hiding this comment

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

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

return "Cars{" +
"cars=" + cars +
'}';
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package racingcar.model;
package racingcar.domain;

import java.util.Objects;
import racingcar.message.ErrorMessages;
Expand All @@ -13,11 +13,6 @@ public Name(String name) {
this.name = name;
}

@Override
public String toString() {
return this.name;
}

private void validate(String name) {
validateNotEmpty(name);
validateLength(name);
Expand Down Expand Up @@ -51,4 +46,9 @@ public boolean equals(Object obj) {
public int hashCode() {
return Objects.hash(name);
}

@Override
public String toString() {
return this.name;
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package racingcar.model;
package racingcar.domain;

public class Position {
private static final int INITIAL_POSITION = 0;
Expand All @@ -17,4 +17,10 @@ public int toInt() {
return this.position;
}

@Override
public String toString() {
return "Position{" +
"position=" + position +
'}';
}
}
Original file line number Diff line number Diff line change
@@ -1,21 +1,41 @@
package racingcar.model;
package racingcar.domain;

import static racingcar.message.ErrorMessages.*;
import static racingcar.message.ErrorMessages.TRY_CNT_NOT_NUMBER;
import static racingcar.message.ErrorMessages.TRY_CNT_NOT_VALID_NUMBER;

import java.util.Objects;

public class TryCount {
private static final int TRY_MAX_CNT = 1000;
private final int tryCount;

public TryCount(String countString) {
validateStringIsNumber(countString);
isValidNumber(countString);
Copy link
Author

Choose a reason for hiding this comment

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

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

this.tryCount = convertStringToInt(countString);
validatePositive(tryCount);
}

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

private void validateStringIsNumber(String input) {
if (!input.matches("[0-9]")) {
throw new IllegalArgumentException(TRY_CNT_NOT_NUMBER);
}
}

private void isValidNumber(String input) {
long count = Long.parseLong(input);
if (count < 0 || count > TRY_MAX_CNT) {
throw new IllegalArgumentException(TRY_CNT_NOT_VALID_NUMBER);
}
}

private int convertStringToInt(String string) {
return Integer.parseInt(string);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
Expand All @@ -33,17 +53,10 @@ public int hashCode() {
return Objects.hash(tryCount);
}

private int convertStringToInt(String string) {
try {
return Integer.parseInt(string);
} catch (NumberFormatException e) {
throw new IllegalArgumentException(TRY_CNT_NOT_NUMBER);
}
}

private void validatePositive(int count) {
if (count < 0) {
throw new IllegalArgumentException(TRY_CNT_NOT_NATURAL_NUMBER);
}
@Override
public String toString() {
return "TryCount{" +
"tryCount=" + tryCount +
'}';
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package racingcar.model;
package racingcar.domain;

import java.util.List;
import java.util.stream.Collectors;
Expand All @@ -7,20 +7,14 @@ public class Winners {
private static final int MIN_POSITION = 0;

private final List<Car> winners;
private final int maxPosition;

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

Choose a reason for hiding this comment

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

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

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

winners = cars.stream()
.filter(car -> car.getPosition() == maxPosition)
.collect(Collectors.toList());
}


public int getMaxPosition() {
return maxPosition;
}

public List<String> getNames() {

Choose a reason for hiding this comment

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

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

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

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

return winners.stream()
.map(Car::getName)
Expand All @@ -34,4 +28,10 @@ private int getMaxPosition(List<Car> cars) {
.orElse(MIN_POSITION);
}

@Override
public String toString() {
return "Winners{" +
"winners=" + winners +
'}';
}
}
20 changes: 20 additions & 0 deletions src/main/java/racingcar/domain/strategy/FixMoveStrategy.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package racingcar.domain.strategy;

public class FixMoveStrategy implements MoveStrategy {

Choose a reason for hiding this comment

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

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

  • 해당 클래스 자체는 나중에 랜덤값이 아닌 고정 값을 전달하고 싶은 경우 사용할 수 있으니 재사용의 가능 여부는 충분하지만 이런 식으로 '테스트를 위한 클래스'이 늘어나도 괜찮은건지에 대한 고민
    • 테스트를 위한 클래스라고 간주된다면 테스트코드 패키지에 추가하는 것이 맞을 것 같아요🙂 재사용의 가능 여부가 충분하더라도, 그게 확실하지 않다면 프로덕션코드에 구현하는 것은 오히려 혼란을 가중하는 역효과가 있다고 생각합니다.
    • 추가로 람다를 이용해서 구현체를 만들지 않고 테스트에서 사용할 수도 있습니다! (람다와 FunctionalInterface 라는 키워드로 검색해보시면 좋을 것 같아요.)

private static final int MOVING_DISTANCE = 1;
private static final int STANDARD_VALUE = 4;

private final int distance;

public FixMoveStrategy(int distance) {
this.distance = distance;
}

@Override
public int move() {
if (distance >= STANDARD_VALUE) {
return MOVING_DISTANCE;
}
return 0;
}
}
5 changes: 5 additions & 0 deletions src/main/java/racingcar/domain/strategy/MoveStrategy.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package racingcar.domain.strategy;

public interface MoveStrategy {
int move();
}
19 changes: 19 additions & 0 deletions src/main/java/racingcar/domain/strategy/RandomMoveStrategy.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package racingcar.domain.strategy;

import racingcar.util.RandomUtils;

public class RandomMoveStrategy implements MoveStrategy {
private static final int MOVE_MIN = 0;
private static final int MOVE_MAX = 9;
private static final int STANDARD_VALUE = 4;
private static final int MOVING_DISTANCE = 1;

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

Choose a reason for hiding this comment

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

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

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

  • 움직일지 아닐지를 판단한다.
  • 실제로 움직이는 양(현재 1 또는 0)을 결정하여 반환한다

Copy link
Author

Choose a reason for hiding this comment

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

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

if (random >= STANDARD_VALUE) {
return MOVING_DISTANCE;
}
return 0;
}
}
2 changes: 1 addition & 1 deletion src/main/java/racingcar/message/ErrorMessages.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ public class ErrorMessages {
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 이상이어야 합니다.";
public static final String TRY_CNT_NOT_VALID_NUMBER = "[ERROR] 시도할 횟수는 0 이상 1000 이하여야 합니다.";
}
Loading