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

[스티치] 자동차 경주 게임 미션 코드리뷰 제출합니다. #93

Merged
merged 43 commits into from
Feb 17, 2020
Merged

Conversation

lxxjn0
Copy link

@lxxjn0 lxxjn0 commented Feb 13, 2020

최대한 TDD로 구현하고자 노력했습니다.
아직까지 설계적인 부분이나 코드적인 부분이나 부족한 점이 많습니다.
무자비한 리뷰 부탁드리겠습니다 😃
리뷰해주셔서 감사합니다!!!

1. null과 공백 문자열을 0으로 처리하는 기능을 calculate 메소드에 구현.
2. null과 공백 문자열을 0으로 처리하는 테스트 구현.
1. 콤마와 콜론 구분자로 나누어서 더하는 기능을 calculate 메소드에 추가.
2. 콤마와 콜론 구분자로 나누어서 더하는 테스트 구현.
1. 커스텀 구분자로 나누어서 더하는 기능을 calculate 메소드에 추가.
2. 커스텀 구분자로 나누어서 더하는 테스트 구현.
3. 리팩토링 진행.
1. 음수에 대해 RuntimeException throw을 calculate 메소드에 추가.
2. 숫자 이외의 값 또는 음수에 대해 RuntimeException throw 테스트 구현.
3. 리팩토링 진행.
1. StringUtil 클래스에 자동차의 이름을 쉼표(,)로 나누는 메소드 구현.
2. 자동차의 이름을 쉼표(,)로 나누는 테스트 구현.
1. Name 객체에 자동차 이름의 길이가 5자를 넘지 않는지 확인하는 메소드 구현.
2. 자동차 이름의 길이가 5자를 넘지 않는지 확인하는 테스트 구현.
1. MovementNumber 객체에 이동 횟수 입력이 숫자인지 검증하는 메소드 구현.
2. 이동 횟수 입력이 양수인지 검증하는 메소드 구현.
3. 이동 횟수 입력에 대한 유효성을 검증하는 테스트 구현.
1. RandomGenerator 클래스에 0에서 9까지의 Random 값을 생성하는 메소드 구현.
2. 0에서 9까지의 Random 값을 생성하는 테스트 구현.
3. package 구조 변경.
1. MovementStrategy 클래스에 Random 값에 따라 전진 여부를 결정하는 메소드 구현.
2. Random 값에 따라 전진 여부를 결정하는 테스트 구현.
1. Car 객체에 Random 값에 따라 전진 여부를 결정하는 메소드 구현.
2. Random 값에 따라 전진 여부를 결정하는 테스트 구현.
1. 자동차들의 이름 중복에 대한 추가적인 유효성 검사 기능 목록 작성.
2. 자동차의 관리와 이동에 대한 추가적인 기능 목록 작성.
1. Car 객체에 자동차가 전진하는 경우 위치를 1 증가시키는 메소드 구현.
2. 자동차가 전진하는 경우 위치를 1 증가시키는 테스트 구현.
3. 테스트 코드 리팩토링.
1. Car 객체의 isMove 메소드의 범위지정자를 private으로 변경함에 따른 테스트 코드 수정.
1. Cars 객체에 자동차의 중복된 이름이 존재하는지 여부를 확인하는 메소드 구현.
2. 자동차의 중복된 이름이 존재하는지 여부를 확인하는 테스트 구현.
3. 테스트 코드 리팩토링.
4. 프로덕션 코드의 예외 처리 메시지 추가.
1. StringUtil 클래스에 자동차의 위치를 문자로 변환하는 메소드 구현.
2. 자동차의 위치를 문자로 변환하는 테스트 구현.
1. Car 객체에 우승한 위치의 자동차인지 확인하는 메소드 구현.
2. 우승한 위치의 자동차인지 확인하는 테스트 구현.
1. RacingGame 객체에 우승한 자동차들을 반환해주는 메소드 구현.
2. 우승한 자동차들을 반환해주는 테스트 구현.
1. stringCalculator 패키지를 추가하고 해당 패키지 안에 StringCalculator 이동.
1. ParameterizedTest를 활용하여 테스트 코드 리팩토링.
1. Position 객체에 잘못된 위치를 생성하는지 확인하는 메소드 구현.
2. 잘못된 위치를 생성하는지 확인하는 테스트 구현.
1. Position 객체를 추가함으로 인한 전체 코드 리팩토링.
1. 입력받은 자동차의 이름을 Cars 객체로 만드는 CarsFactory 클래스와 메소드 구현.
2. 입력받은 자동차의 이름을 Cars 객체로 만드는 테스트 구현.
1. Name 객체에 자동차 이름이 null 또는 공백인지 확인하는 메소드 구현.
2. 자동차 이름이 null 또는 공백인지 확인하는 테스트 구현.
1. 우승한 자동차의 이름을 이어붙이는 메소드 구현.
2. String 배열을 사용한 부분을 List로 변경.
3. RacingGame 객체를 Result로 명명 수정.
4. 전체적인 리팩토링 진행.
1. Result 객체에서 게임을 한 번 진행한 후 결과를 반환하는 메소드 구현.
2. 게임을 한 번 진행한 후 결과를 반환하는 테스트 구현.
1. StringUtil 클래스의 메소드를 이름과 위치 상태를 함께 변환하여 반환하도록 리팩토링 진행.
1. 입력을 담당할 InputView 객체와 출력을 담당할 OutputView 객체를 구현.
2. Result 객체의 package 수정 및 내부 코드 리팩토링.
1. 프로그램의 동작을 위한 Application 클래스 구현.
2. RacingGame을 담당하는 컨트롤러 구현.
1. 테스트 코드에서의 반복을 제거하기 위한 Car 객체 생성자 추가.
1. 역할이 너무 적은 RacingGame 객체를 제거 후 Application 클래스 수정.
Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

스티치 안녕하세요! 리뷰를 맡게 된 데이브 입니다.
요구사항에 맞춰 잘 구현 하셨네요 :)
몇가지 피드백 남겼으니 적용해 보시고, 궁금한점 있으시면 편하게 DM주세요!

inputview = new InputView();
outputView = new OutputView();

final Cars cars = generateCars();
Copy link

Choose a reason for hiding this comment

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

모든 변수에 final을 붙인점이 인상적이네요 👍

Copy link
Author

@lxxjn0 lxxjn0 Feb 15, 2020

Choose a reason for hiding this comment

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

final을 적절한 상황에 맞게 붙여야 한다고 들었는데, 제가 너무 여기저기 다 붙여둔 것이 아닌가 싶은 생각이 들었습니다.

혹시 final을 어떠한 상황에서 붙이는 것이 적절한 지 알려주실 수 있으신가요?
참고할 만한 블로그 링크 정도라도 알려주신다면 정말 감사하겠습니다 👍

Copy link

Choose a reason for hiding this comment

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

아래 글을 읽어보세요~!

결국 Java에서의 final은 Immutable/Read-only 속성을 선언하는 지시어입니다.
클래스, 함수, 변수가 변하지 못하도록 의도하고 싶다면 final로 선언하자.

https://blog.lulab.net/programming-java/java-final-when-should-i-use-it/

}

private void checkPositive(final int parsedNumber) {
if (!(parsedNumber > ZERO)) {
Copy link

Choose a reason for hiding this comment

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

명시적으로 볼 수 있게 parsedNumber <= ZERO로 사용하는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 if문의 조건을 양수가 아닌 경우에 확인하도록 의도하였습니다.

그래서 조금은 더 문맥에 맞게 !(양수)라는 의미로 !(parsedNumber > ZERO) 저렇게 작성했습니다.
의도한 바에 따라 표현하기 위해 코드가 조금 복잡해지는 것 보다는 한눈에 읽기 쉬운 parsedNumber <= ZERO 와 같은 형식이 더 좋은 방법인가요?

Copy link

Choose a reason for hiding this comment

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

저는 후자의 방법이 더 좋은 방법이라고 생각합니다.
또 다른 방법으로는 판별식을 메소드로 분리 하는 방법도 있습니다. :)

Comment on lines 59 to 64
private static Result attemptMove(final Cars cars) {
for (Car car : cars.getCars()) {
car.attemptMoveThrough(RandomGenerator.generateRandomNumber());
}
return new Result(cars);
}
Copy link

Choose a reason for hiding this comment

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

해당 로직은 Car의 일급컬렉션인 Cars내부에 있어도 되지 않을까요?
그리고 해당 메소드는 Car들의 이동과 결과 return 두가지 일을 하고있네요!

Copy link
Author

Choose a reason for hiding this comment

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

저도 이 부분에 대해서 어디서 역할을 담당하는게 좋을지 생각하다가 결국은 조금은 불만족스럽게 위처럼 작성했었습니다.
확실히 다시 한 번 역할과 책임에 대해 고민하고 해당 코드를 보니 부적절한 코드로 보이네요!!

Cars에서 모든 car들의 이동을 요청하는 형식으로 수정하였습니다.

Comment on lines 54 to 56
return IntStream.range(0, movementNumber.getMovementNumber())
.mapToObj(i -> attemptMove(cars))
.collect(Collectors.toList());
Copy link

Choose a reason for hiding this comment

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

Stream으로 구현한 이유가 있을까요? 아래 글을 읽어보면 좋을 것 같아요.

Java 개발자는 많은 연습을 통해 언제 함수형 프로그래밍을 쓰는 것이 좋은지, 어떨때 객체지향/절차형 방식을 고수하는 것이 좋은지 직관적으로 이해할 수 있어야 한다. 충분한 양의 연습이 동반된다면, 두 가지를 잘 혼용해서 우리가 만드는 SW를 개선할 수 있을 것이다.

https://homoefficio.github.io/2016/06/26/for-loop-%EB%A5%BC-Stream-forEach-%EB%A1%9C-%EB%B0%94%EA%BE%B8%EC%A7%80-%EB%A7%90%EC%95%84%EC%95%BC-%ED%95%A0-3%EA%B0%80%EC%A7%80-%EC%9D%B4%EC%9C%A0/

Copy link
Author

Choose a reason for hiding this comment

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

저도 단지 더 짧고 간결해 보이는 코드로 작성하고 싶어서 조금은 무리하게 Stream을 사용한 것 같았습니다.

해당 링크를 읽고 보니 성능적인 측면에서도 그렇고, 무작정인 Stream 사용이 좋지 않다는 것을 알게 되었습니다.
해당 코드는 좀 더 원시적이지만 보기 편한 for-loop로 수정하였습니다!!!


import java.util.Objects;

public class Position {
Copy link

Choose a reason for hiding this comment

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

👍

private static final int RANDOM_UPPER_BOUND = 10;

public static int generateRandomNumber() {
return new Random().nextInt(RANDOM_UPPER_BOUND);
Copy link

Choose a reason for hiding this comment

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

아래 글 읽어보면 좋을 것 같아요.
https://withseungryu.tistory.com/1

Copy link
Author

Choose a reason for hiding this comment

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

이전에 사용한 언어였던 c++에서는 당연히 인지하고 있었던 부분을 여기선 생각하지도 못했네요. 해당 부분의 코드도 수정하였습니다!!


public void printWinners(final Result finalResult) {
final String winners = StringUtil.joinNameOf(finalResult.getWinners());
System.out.println(winners + "가 최종 우승했습니다.");
Copy link

Choose a reason for hiding this comment

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

String.format을 사용해보는건 어떨까요?

}

private static String[] splitByDelimiter(final String value) {
Matcher m = Pattern.compile("//(.)\n(.*)").matcher(value);
Copy link

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.

해당 코드가 미션에서 주어진 양식이여서 그대로 가져왔었는데 데이브님께서 올려주신 링크도 다시 한 번 확인해보았습니다.

제가 영어를 잘 하질 못해서 정확히 원문의 의미를 이해하지는 못하였습니다. 그래도 최대한 해당 글에서 작성된 compliant한 예제를 보고 따라서 수정해 보았습니다.
혹시 제가 수정한 코드가 첨부해 주신 링크가 요구하는 바가 아니였다면 한 번 알려주신다면 감사하겠습니다 :)

Copy link

Choose a reason for hiding this comment

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

Pattern.compile("//(.)\n(.*)") 해당 패턴을 compile하는데 많은 리소스가 소모되니 상수로 빼써 사용해야한다는 말이었습니다 ㅎㅎ
또한 Pattern.compile("//(.)\n(.*)")해당 코드는 메소드가 호출될때마다 같은 패던을 complie해야하니까요.

Comment on lines 24 to 26
public Position increaseByMovingUnit() {
return new Position(position + MOVING_UNIT);
}
Copy link

Choose a reason for hiding this comment

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

아무래도 모든 자동차가 position 0부터 시작해서 같은 position을 가진 Position객체가 생성될 것 같습니다.
캐싱 처리 해서 같은 position을 가진 Position객체는 한번만 생성 될 수 있게 해보는건 어떨까요?

실제로 Integer와 같은 Wrapper Class도 같은 방식을 사용하고 있습니다 :)
아래 링크 참고해서 Array형태로 구현 해도 좋고 Key-Value인 Map을 이용해서 구현 해봐도 좋을 것 같아요.
https://feco.tistory.com/112

Copy link
Author

Choose a reason for hiding this comment

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

Position 객체의 인스턴스 변수를 final로 선언하는 것이 맞는 것인지 아닌지에 대해 다른 크루와 한 번 얘기를 나눴던 적이 있었습니다.

같이 대화한 크루도 이런 식으로 구현하면 만약 엄청나게 많은 자동차들이 생성되어서 매번 전진을 하게 된다면 너무 많은 인스턴스 낭비가 아닐까? 라는 의문을 던져주어서 리뷰어님의 의견도 들어보고 싶어서 위의 코드대로 제출해보았습니다.

캐싱이라는 의미를 이해하고 최대한 같은 맥락을 가질 수 있도록 Cache 클래스를 작성해 보았습니다.

그리고 만약

public class Position {
	private static final int INIT_POSITION = 0;
	private static final int MOVING_UNIT = 1;

	public static final Position ZERO = Position.valueOf(INIT_POSITION);

	private int position;

	public Position(final int position) {
		checkValid(position);
		this.position = position;
	}
        ...

	public void increaseByMovingUnit() {
		this.position += MOVING_UNIT;
	}
        ...
}

해당 코드와 같이 인스턴스 변수를 final이 아닌 수정 가능하도록 선언을 하고 위와 같이 증가하는 메소드를 작성한다면 캐싱에 대한 부분은 전혀 고민하지 않아도 되는 부분인가요???

Copy link

Choose a reason for hiding this comment

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

final이 아닌 수정가능하도록 선언하면 구현상 캐싱에 대한 부분은 고민하지 않아도 되겠죠.

하지만 개발자는 언제나 리소스에 대한 측면을 생각 하고 있어야 하고 캐싱도 한 면이라고 생각합니다.
여러 유저에게 Request가 왔을 때 공통 데이터를 내려주는데 모든 데이터를 다시 가공해서 내려 줄 필요가 없으니까요 :)

1. Application 클래스의 attemptMove 메소드 책임 재분배.
2. Cars 객체의 attemptMove 메소드 구현.
3. 불필요한 stream 제거.
4. RandomGenerator seed 값 추가.
5. OutputView에 String.format 적용.
6. Pattern 코드의 Compliant한 예제를 참고하여 수정.
1. PositionCache 클래스를 통해 Position의 객체 중복을 cache 처리.
2. 그에 따른 모든 Position 생성자 코드가 들어간 코드 수정.
1. 생성된 랜덤한 수의 유효성을 확인하기 위한 RandomNo 객체 구현.
2. 테스트 코드 일부 수정.
1. RandomNo 클래스를 통해 RandomNo의 객체 중복을 cache 처리.
2. 그에 따른 모든 RandomNo 생성자 코드가 들어간 코드 수정.
@lxxjn0
Copy link
Author

lxxjn0 commented Feb 15, 2020

피드백을 확인하여 전체적인 코드를 리팩토링 진행하였습니다.

추가적으로 자동차가 이동할 때 랜덤한 수를 받아오고 판단하여서 진행을 하는데, 이 부분에서 0이상 9이하의 수만 가능하기 때문에 이를 RandomNo 객체를 만들어서 생성자에서 유효성 검사를 진행하도록 하였습니다.

그리고 앞서 말씀해주신 캐싱에 대한 연습을 위해, 과할 수 있지만 RandomNoCache 클래스도 생성하여 구현해 보았습니다.

그래고 코드에 대해 몇 가지 궁금한 점이 있었는데 혹시 확인해 주시면 감사하겠습니다 :)

작성하고 보니 생각보다 질문한 부분이 많네요.. ㅠㅡㅠ
꼼꼼하게 리뷰해주셔서 감사합니다 😃

final List<Result> results = playRacingGame(cars, movementNumber);

outputView.printRacingGameResult(results);
outputView.printWinners(getFinalResult(results, movementNumber));
Copy link
Author

Choose a reason for hiding this comment

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

위의 부분에서 매번 경주를 한번 진행할 때마다(자동차 경기 Lap마다) car의 이동 상황을 출력해주는 것이 좋은 방법인가요?
아니면 매번 경주마다의 이동 상황을 제가 구현한 방식처럼 Result라는 기록 저장 객체에 저장해두고 모든 게임이 끝난 후 한번에 출력하는 방법이 좋은 방법인가요?

Copy link

Choose a reason for hiding this comment

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

이 부분은 결과 데이터를 가지고 어떤 기능을 추가적으로 만들 것 인가에 따라 달라질 것 같아요.
따라서 어떤 방법이 더 좋은 방법인가에 대한 답은 요구사항에 따라 다르다라고 생각됩니다.

// NOTE : 불변객체의 이점?
public boolean isSamePosition(final int winnerPosition) {
return this.position.getPosition() == winnerPosition;
}
Copy link
Author

Choose a reason for hiding this comment

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

데이브님께서 캐싱을 활용해보라고 했던 Position 객체는 제가 인스턴스 변수가 변하지 않는 불변 객체(? 해당 용어가 맞는 용어인지 정확히 모르겠습니다)를 의도하여 구현했습니다.

그런데 이번 미션에서 보면 이동이 자주 발생할 수 있기 때문에 차라리 Car 객체마다 각각의 Position 객체를 가지고 인스턴스 변수인 int position 을 진행할 때 마다 수정하는 방법이 더 낫지 않았을까? 하는 생각을 해보았습니다.

제가 구현한 것 처럼 인스턴스 변수가 변하지 않도록 구현하는 것이 이번 미션에서 효율적이지 않은 방법일까요?
아니면 이처럼 인스턴스 변수가 변하지 않도록 객체를 구현해야 하는 상황은 어떤 경우에 적절한 지 궁금합니다 🤔

Copy link

Choose a reason for hiding this comment

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

이 글을 읽어보세요.
https://galid1.tistory.com/622

// NOTE : unmodifiable로 처리해주는 것이 의미를 가지는지 리뷰어님께 여쭤보기.
public List<Car> getCars() {
return Collections.unmodifiableList(cars);
}
Copy link
Author

Choose a reason for hiding this comment

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

제가 공부하기론 일급 컬렉션의 경우 최대한 외부에서 접근할 수 없도록 getter 메소드를 지양하라고 들었습니다.

여기선 출력을 위해서 어쩔 수 없이 getter를 사용하였는데 위처럼Collections.unmodifiableList() 로 처리하는 것이 잘 된 방법인지 궁금합니다.

Copy link

Choose a reason for hiding this comment

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

getter로 받은 원소를 변경 불가능하게 하기 위해 Collections.unmodifiableList()로 처리하는 방법은 좋은 방법이라고 생각합니다.

Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

스티치! 피드백 반영하느라 고생하셨어요ㅎㅎ
몇가지 피드백 추가로 남겨놓았으니 반영해보세요! 😁

Comment on lines 33 to 38
private Integer getMaxPosition() {
return cars.getCars().stream()
.map(Car::getPosition)
.max(Integer::compareTo)
.get();
}
Copy link

Choose a reason for hiding this comment

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

Optional에서 원소 유무를 체크하지 않고 get()을 하면 NoSuchElementException이 발생 할 수 있습니다.
아래처럼 원소가 없을 시 return 할 값을 지정해주는 것은 어떨까요?

private Integer getMaxPosition() {
        return cars.getCars().stream()
                .mapToInt(Car::getPosition)
                .max()
                .orElse(0);
	}

Copy link
Author

Choose a reason for hiding this comment

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

제가 아직 Optional에 대한 공부가 부족해서 제대로 사용하질 못했었네요 😢

Optional의 이점을 잘 활용할 수 있는 코드를 작성하도록 노력해보겠습니다!!!
해당 부분 수정하겠습니다 :)

Comment on lines 28 to 32
public void attemptMove() {
for (Car car : cars) {
car.attemptMoveThrough(RandomNo.valueOf(RandomGenerator.generateRandomNumber()));
}
}
Copy link

Choose a reason for hiding this comment

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

해당 메소드는 테스트 하기 어려울 것입니다.
Car가 이동하는 메소드는 random값을 받아 움직이기 때문이죠.
하지만 실제 프로덕션 코드에서 자동차들을 이동시키는 중요한 메소드라고 생각이 드는데요.
해당 메소드를 테스트 할 수 있도록 Car의 움직일 수 있음을 추상화 해보면 어떨까요?
잘 모르겠으면 DM주세요 :)

Copy link
Author

Choose a reason for hiding this comment

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

Car 객체의 attemptMoveThrough 메소드를

public class CarTest {
        ...
        @Test
	void attemptMoveThrough_랜덤_값이_3_이하() {
		final int stoppedRandomNumber = 3;
		final Car car = new Car(new Name("test"), Position.valueOf(4));
		car.attemptMoveThrough(RandomNo.valueOf(stoppedRandomNumber));

		final int actual = car.getPosition();

		final int expected = 4;

		assertThat(actual).isEqualTo(expected);
	}

	@Test
	void attemptMoveThrough_랜덤_값이_4_이상() {
		final int movedRandomNumber = 7;
		final Car car = new Car(new Name("test"), Position.valueOf(4));
		car.attemptMoveThrough(RandomNo.valueOf(movedRandomNumber));

		final int actual = car.getPosition();

		final int expected = 4 + MOVING_UNIT;

		assertThat(actual).isEqualTo(expected);
	}
        ...
}

위와 같이 Car 객체에서 테스트를 했습니다.
말씀하신 Car의 움직일 수 있음 을 추상화 한다는 부분이 어떤 부분인지 제가 정확히 모르겠는데 알려주실 수 있으실까요?

Copy link

Choose a reason for hiding this comment

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

현재는 Cars에서 추가적인 로직 없이 Car들에게 randomNumber를 전달하여 이동시키고 있습니다.
하지만 추가적으로 Cars의 attemptMove에 추가적인 로직이 들어가면 어떨까요?
지금 대로면 자동차들이 전진 했는지 테스트 하기위해 추가된 로직도 테스트에 일일이 추가해 줘야 하지 않을까요? 그렇기 때문에 attemptMove메소드의 테스트는 꼭 필요하지 않나 싶습니다.

그렇다면 현재 테스트 불가능 한 이유는 무엇일까요?
random값이 어떤값이 들어 올지 불명확하다일겁니다.

if (isMovable(randomNo)) {
        this.position = this.position.increaseByMovingUnit();
}

구현 하신 코드 중 isMovable(randomNo)을 인터페이스로 구현 후 테스트 할땐 어떤 random값이 와도
isMovable(randomNo)이 무조건 true가 되게 하면 Cars의 attemptMove호출 시 모든 Car가 이동 했다는 것을 테스트 할 수 있을 것 같습니다.

Strategy pattern을 이용해보세요!
https://limkydev.tistory.com/84

Comment on lines 17 to 23
private List<String> generateRacingCarStatus() {
return cars.getCars().stream()
.map(car -> car.getName()
+ " : "
+ StringUtil.convertIntoDashBy(car.getPosition()))
.collect(Collectors.toList());
}
Copy link

Choose a reason for hiding this comment

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

자동차명 : ---- 이렇게 포맷팅 하는 부분은 View의 역활이 라고 생각되는데요.
자동차명 : position값 으로 된 Map을 View에 넘겨서 View에서 해당 로직을 수행 하도록 해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분에 대해서 데이브님과 같은 의견을 제시한 크루가 있었습니다.

자동차명과 위치가 추후 View에서 어떻게 가공되고 처리해야할 지 확신할 수 없으니, 실제로 position 값을 넘겨주는게 더 좋지 않을까? 라고 말했습니다.

위와 같은 이유로 해당 로직을 View에서 진행하는게 맞는 것인가요???

Copy link

Choose a reason for hiding this comment

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

현재는 자동차명 : --- 이지만 자동차명 : ===> 뭐 이런식으로 출력하는 형식이 바뀔 수 있으니까요.
그렇다면 이런 변화가 있을때 Model에서 수정 해주는 것이 맞는건지, MVC패턴 입장에서 생각해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

출력문의 형식에 대한 변화가 생기는 경우, View에서 수정을 해주는 것이 맞는것 같아요 :)

Comment on lines 22 to 27
public static Position valueOf(final int position) {
if (position >= PositionCache.low && position <= PositionCache.high) {
return PositionCache.cache[position + (-PositionCache.low)];
}
return new Position(position);
}
Copy link

Choose a reason for hiding this comment

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

따로 PositionCache를 만들지 않고 아래처럼도 구현 가능합니다 :)

        private static final Map<Integer, Position> cache = new HashMap<>();
	public static Position valueOf(final int position) {
		if (cache.containsKey(position)) {
			return cache.get(position);
		}
		
		Position newPosition = new Position(position);
		cache.put(position, newPosition);
		return newPosition;
	}

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분을 알려주신 방법대로 수정해보았습니다 :)

1. Optional의 잘못된 사용 수정.
2. 결과 출력을 위한 Result를 Map과 같은 형식으로 저장.
1. cache 클래스 제거.
// NOTE : 아래의 random.setSeed(System.currentTimeMillis())를 적용하면 모든 random값이 동일하게 생성되는 상황이 발생.
// random.setSeed(System.currentTimeMillis());
return random.nextInt(RANDOM_UPPER_BOUND);
}
Copy link
Author

Choose a reason for hiding this comment

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

데이브님께서 이전에 알려주신 seed 적용을 했었는데, 그러다 보니 모든 랜덤값이 동일하게 나오는 상황이 발생했습니다.

image

image

위 그림처럼 디버깅때 브레이크 포인트의 위치에 따라 랜덤값 생성이 동일거나 혹은 다르게 생성되어서 결과가 저런 식으로 발생했습니다. 실제로 프로그램을 실행하면 모든 값이 동일하게 발생하였구요.

그래서 seed부분을 제외했는데, 혹시 데이브님이 의도하신대로 제가 수정한 것이 아니였다면 다른 방법을 알려주실 수 있으신가요?

Copy link

Choose a reason for hiding this comment

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

해당 부분 확인 확인 결과 currentTimeMillis값이 차이가 나지 않아서 같은 결과가 나오는 것 같아요.
(System.nanoTime()을 사용 하니 다른 값이 나오더라구요.)
그리고 Seed값과 별개로 new Random()을 할때 System.nanoTime을 고려해서 seed값을 생성되고 nextInt처럼 난수를 생성 할때마다 seed값이 바뀌고 있네요!
혼란을 드려서 죄송합니다 😭

Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

스티치 고생하셨습니다 ㅎㅎ
질문 주신 부분만 반영 해보시고 PR주시면 머지 하도록 하겠습니다 :)

Comment on lines 22 to 26
private boolean isDuplicateExist(final List<Car> cars) {
return cars.size() != Arrays.stream(cars.toArray())
.distinct()
.count();
}
Copy link

Choose a reason for hiding this comment

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

한 줄에 다 쓰는것 보단 이렇게 분리 해주는게 가독성에 더 좋을 것 같네요 :)

long distinctCarSize = Arrays.stream(cars.toArray())
				.distinct()
				.count();
return cars.size() != distinctCarSize;

Copy link
Author

Choose a reason for hiding this comment

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

넵! 해당 부분 수정하겠습니다!!!

1. MovableStrategy Interface 구현.
2. MovableStrategy의 프로덕션 코드에서 활용할 RandomMovableStrategy 구현.
3. MovableStrategy의 테스트 코드에서 활용할 PlannedMovableStrategy 구현.
@lxxjn0
Copy link
Author

lxxjn0 commented Feb 17, 2020

전략 패턴을 최대한 적용해보려고 노력해봤는데, 아직까지는 헛갈리는 부분이 많았던 것 같습니다.
혹시 코드 확인해보시고 만약 수정해야할 사항이나 더 나은 방향으로 개선해보면 좋은 점들이 보인다면 알려주시면 감사하겠습니다.

Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

스티치! 자동차 전체 이동 테스트까지 잘 구현해 주셨습니다 💯 💯
이만 머지할게요 ㅎㅎ
혹시 피드백 반영하면서 헷갈렸거나 아직 의문인 점이 있다면 편하게 DM주세요!

import java.util.HashMap;
import java.util.Map;

public class Power {
Copy link

Choose a reason for hiding this comment

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

💯

@@ -0,0 +1,10 @@
package com.woowacourse.racingGame.domain;

public class RandomMovableStrategy implements MovableStrategy {
Copy link

Choose a reason for hiding this comment

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

💯💯

}

@Test
void attemptMove_전부_이동() {
Copy link

Choose a reason for hiding this comment

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

💯💯💯

@dave915 dave915 merged commit 0c5c940 into woowacourse:lxxjn0 Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants