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

Step5 - 자동차 경주(리팩토링) 및 피드백 반영 #2396

Merged
merged 9 commits into from Jul 26, 2021
19 changes: 10 additions & 9 deletions src/main/java/racingcar/RacingGame.java
@@ -1,12 +1,15 @@
package racingcar;

import racingcar.domain.Car;
import racingcar.domain.Winner;
import racingcar.utils.RandomNumber;
import racingcar.view.InputView;
import racingcar.view.ResultView;

import java.util.Collections;
import java.util.List;
import java.util.Random;

public class RacingGame {
private static final int MAX_VALUE = 10;

public static void main(String[] args) {

Choose a reason for hiding this comment

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

지금은 main 메서드에서 많은 역할을 담당하고 있는데요
비즈니스 로직의 흐름제어를 담당하는 일종의 Controller와 같은 객체를 분리하여 추상화해보면 좋을 것 같습니다!

image

해당 이미지를 참고하여 주세요!

InputView inputView = new InputView();
ResultView resultView = new ResultView();
Expand All @@ -23,18 +26,16 @@ public static void main(String[] args) {
Collections.reverse(cars);

Winner winner = Winner.of(Winner.resultPointOfFirst(cars));
System.out.println(winner.firstResultPoint());

winner.extracted(resultView, cars, winner);
winner.winnerSelection(resultView, cars);
}

public static void printAll(List<Car> cars) {
RandomNumber randomNumber = new RandomNumber();
ResultView resultView = new ResultView();
for(Car car: cars) {
Random random = new Random();

int randomNumber = random.nextInt(MAX_VALUE);
int moveNumber = car.move(randomNumber);

int moveNumber = car.move(randomNumber.producedRandomNumber());
resultView.print(car.getName(), moveNumber);
}
System.out.println();
Expand Down
@@ -1,4 +1,4 @@
package racingcar;
package racingcar.domain;

import java.util.ArrayList;
import java.util.List;
Expand Down
18 changes: 18 additions & 0 deletions src/main/java/racingcar/domain/Name.java
@@ -0,0 +1,18 @@
package racingcar.domain;

public class Name {
private static final int LIMIT_CAR_NAME_LENGTH = 5;
private String carName;

Choose a reason for hiding this comment

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

#2362 (comment) 해당 코멘트를 확인해주세요!


private Name(String carName) {
this.carName = carName;
}

public static Name validateByName(String carName) {

Choose a reason for hiding this comment

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

해당 메서드가 하는 역할은 검증 보다는 이름의 생성에 가까워보이는데요
더욱 적절한 네이밍으로 변경해보면 어떨까요?

if(carName.length() >= LIMIT_CAR_NAME_LENGTH) {
throw new IllegalArgumentException("이름은 5글자가 최대입니다.");

Choose a reason for hiding this comment

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

예외처리 메시지에서 검증 조건의 값을 하드코딩한다면 어떠한 문제가 있을지 고민해보고 상수를 활용해보아요

Copy link
Author

Choose a reason for hiding this comment

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

오타의 우려가 있는것 같습니다.

}
return new Name(carName);
}

}
@@ -1,4 +1,6 @@
package racingcar;
package racingcar.domain;

import racingcar.view.ResultView;

import java.util.List;
import java.util.stream.Collectors;
Expand All @@ -14,6 +16,12 @@ public static Winner of(int firstResultPoint) {
return new Winner(firstResultPoint);
}

public static int resultPointOfFirst(List<Car> cars) {
return cars.stream().map(Car::getMoveSpace)
.max(Integer::compareTo)
.get();
}

public int firstResultPoint() {
return getFirstResultPoint();
}
Expand All @@ -22,17 +30,11 @@ private int getFirstResultPoint() {
return firstResultPoint;
}

public void extracted(ResultView resultView, List<Car> cars, Winner winner) {
public void winnerSelection(ResultView resultView, List<Car> cars) {

Choose a reason for hiding this comment

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

domain 패키지의 객체는 view 패키지 객체에 의존하지 않도록 구현한다.의 조건을 지켜보아요

List<Car> winners = cars.stream()
.filter(car -> car.getMoveSpace() == winner.firstResultPoint())
.filter(car -> car.getMoveSpace() == firstResultPoint())
.collect(Collectors.toList());

resultView.getWinner(winners);
}

public static int resultPointOfFirst(List<Car> cars) {
return cars.stream().map(Car::getMoveSpace)
.max(Integer::compareTo)
.get();
}
}
19 changes: 19 additions & 0 deletions src/main/java/racingcar/utils/RandomNumber.java
@@ -0,0 +1,19 @@
package racingcar.utils;

import java.util.Random;

public class RandomNumber {
private static final int MAX_VALUE = 10;

private int randomNumber;

public int producedRandomNumber() {
randomNumber = createRandomNum();

Choose a reason for hiding this comment

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

해당 메서드 분리는 어떠한 목적으로 분리해주셨을까요?

Copy link
Author

Choose a reason for hiding this comment

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

랜덤값에 의존하지 않고 테스트하기 위해 분리했습니다.

Choose a reason for hiding this comment

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

producedRandomNumber()에서 createRandomNum()를 바로 호출하고 있는데 랜덤에 대한 의존을 분리할 수 있을까요!?

return randomNumber;
}

private int createRandomNum() {
Random random = new Random();

Choose a reason for hiding this comment

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

매번 Random을 생성하지 않고 재사용할 수 있도록 수정한다면 어떠한 장점이 있을까요?
충분한 고민 후 이펙티브 자바 아이템 6. 불필요한 객체 생성을 피하라를 참고해주세요!

Copy link
Author

Choose a reason for hiding this comment

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

random 객체를 새로 생성해서 난수를 발생시키는줄 알았었는데 불필요했네요.
확인하고 수정했습니다.👍

return random.nextInt(MAX_VALUE);
}
}
@@ -1,4 +1,6 @@
package racingcar;
package racingcar.view;

import racingcar.domain.Name;

import java.util.Scanner;

Expand All @@ -7,7 +9,6 @@
*/
public class InputView {
private static final Scanner sc = new Scanner(System.in);
private static final int LIMIT_CAR_NAME_LENGTH = 5;

public String[] insertCarNames() {
System.out.println("경주할 자동차 이름을 입력하세요");
Expand All @@ -24,18 +25,11 @@ public String[] splitCarName(String text) {
private String[] createNameOfCarsArray(String text) {
String[] cars = text.split(",");
for (int i = 0; i < cars.length; i++) {
carLengthValidate(cars[i]);
Name.validateByName(cars[i]);

Choose a reason for hiding this comment

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

View에서 Name을 통해 검증을 하여도 실제 도메인에서 Name을 사용하지 않는다면 동일한 문제가 남아있을 것 같습니다
Car에서 String으로 가지고 있는 name을 Name으로 변경한다면 어떠한 차이가 있을지 고민해보아요

}
return text.split(",");
}

private void carLengthValidate(String car) {
if (car.length() > LIMIT_CAR_NAME_LENGTH) {
throw new IllegalArgumentException("이름은 5글자가 최대입니다.");
}
}


public int getNumberOfTry() {
System.out.println("시도할 횟수는 몇 회 인가요?");
return sc.nextInt();
Expand Down
@@ -1,4 +1,6 @@
package racingcar;
package racingcar.view;

import racingcar.domain.Car;

import java.util.List;

Expand Down
2 changes: 2 additions & 0 deletions src/test/java/racingcar/CarTest.java
Expand Up @@ -3,6 +3,8 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import racingcar.domain.Car;
import racingcar.view.InputView;

import java.util.List;
import java.util.Random;
Expand Down
1 change: 1 addition & 0 deletions src/test/java/racingcar/RacingGameTest.java
Expand Up @@ -2,6 +2,7 @@

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import racingcar.domain.Car;

import java.util.List;

Expand Down