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

자동차 경주 - 5단계 #6083

Merged
merged 1 commit into from
Mar 22, 2025
Merged

Conversation

jorippppong
Copy link

@jorippppong jorippppong commented Mar 21, 2025

자동차 경주 - 5단계 구현했습니다 :)

노로 바이러스에 걸려서 미션이 늦었습니다ㅜㅜ 미션 수행 중에 질문이 있어서 남깁니다.

  • RaceController와 RaceService를 하나의 클래스로 합칠까 고민했는데, 그렇게 할 경우 Race 객체와 Cars 객체를 생성자 내부에서 초기화해서 final로 유지하려면 생성자 내부에서 객체 생성과는 직접적 관련이 없는 InputView를 호출하는 등의 로직을 추가해야 하는데, 해당 행위가 생성자의 역할과는 맞지 않는 것 같아서 RaceController와 RaceService는 분리한 상태로 두었습니다. 아래의 코드와 같이 이렇게 객체 생성시에 호출해야 하는 메서드를 생성자 내부에 넣는게 객체지향적인 방식인지 궁금합니다.
public RaceService() {
	String carInput = InputView.getCarInput();
	int roundInput = InputView.getRoundInput();
	InputView.closeScanner();

	List<String> names = Stream.of(carInput.split(","))
		.map(String::trim)
		.toList();
	this.race = new Race(roundInput);
	this.cars = new Cars(names);
}
  • 자동차 경주의 결과를 관리하는 클래스인 RaceResult를 새로 추가했고, 해당 클래스는 상태를 관리하지 않아서 Util 클래스로 설정해두었습니다.

항상 세심한 리뷰에 항상 감사드립니다. 마지막 단계 리뷰 잘 부탁드립니다!

@jorippppong 유리님, 노로바이러스라니요 굴을 잘못드셨군요 생각보다 고통스러운데
얼른 체력회복하시길 기원하겠습니다 🙏

문의 주신 부분에 대해서 답변 드리자면 지금 구조가 더 좋습니다 😄
RaceControllerRaceService에 대해서 분리한 지금 상태가 저도 더 좋은 구조라고 생각합니다.
객체 생성 시에 호출해야 하는 메서드를 생성자 내부에 넣는다면 이미 잘 알고 계신대로
생성시점부터 호출해야 하는 클래스에 대한 강한 결합이 생깁니다.
carInput이나 roundInput등을 인자로 받아서 생성자를 생성할 수 있도록 해야
생성시점에 꼭 필요한 값이 무엇인지, 그리고 그 값을 받거나 처리하는 클래스와의 분리도 되고
각각 클래스 간에 역할과 책임도 명확해집니다.
무엇보다 InputView에 문제가 생겼는데 그로인해 RaceService()까지 문제가 발생하게 되고
초기화 자체도 안된다면 그야말로 더 큰 문제라고 볼 수 있을 것 같습니다 😄

오히려 전략을 설정하는 부분을 생성자를 통해 받는다면
RaceService를 생성할 때 전략 선택에 대한 유연한 구조를 가져갈 수 있을 것 같습니다 😄

// AS IS
private MovingStrategy randomMovingStrategy() {
	return new RandomMovingStrategy();
}

// TO BE
public RaceService(String carInput, int roundInput) {
	this(carInput, roundInput, new RandomMovingStrategy());
}

public RaceService(String carInput, int roundInput, MovingStrategy movingStrategy) {
	List<String> names = Stream.of(carInput.split(","))
				.map(String::trim)
				.toList();
	this.race = new Race(roundInput);
	this.cars = new Cars(names);
	this.movingStrategy = movingStrategy;
}

// USAGE
RaceService randomRuleRace = new RaceService(carInput, roundInput, new RandomMovingStrategy());
RaceService gogoRuleRace = new RaceService(carInput, roundInput, () -> true); // AllwaysGoMovingStrategy()
RaceService defaultRace = new RaceService(carInput, roundInput);

고민에 도움 되셨기를 바라 봅니다 🙏

@neojjc2 neojjc2 self-requested a review March 22, 2025 06:35
@neojjc2 neojjc2 self-assigned this Mar 22, 2025
@neojjc2
Copy link

neojjc2 commented Mar 22, 2025

#tag @neojjc2

Copy link

@neojjc2 neojjc2 left a comment

Choose a reason for hiding this comment

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

안녕하세요 유리님 🙇

마지막 단계도 잘 진행해주셨네요 😄
정리 잘 해주셨습니다.
문의 주신 내용은 별도 코멘트로 남겨드렸습니다 🙇

소소한 의견 드렸는데요,
개선이라기 보다는 마지막으로 정리하시면서
한번 참고 정도만 해주시면 될 것 같습니다.

어느덧 미션을 마칠 때가 왔네요 😄
너무 많이 괴롭혀드린게 아닌가 걱정입니다만,
그많큼 고민많이 해주시고 잘 따라와 주셔서 좋은 결과물
만들 수 있었던 것 같습니다.

그럼 남은 미션도 잘 마무리하시고
꼭 완주하시기를 응원하겠습니다.

그동안 자동차 미션 하시느라 정말 고생많으셨습니다 🙇

LGTM 👍

@@ -1,6 +1,7 @@
package racingcar;
package racingcar.domain;
Copy link

Choose a reason for hiding this comment

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

정리 잘 해주셨네요 👍

public class RaceService {
private final Race race;
private final Cars cars;

public RaceService(String carInput, int roundInput) {
List<String> names = Stream.of(carInput.split(","))
Copy link

Choose a reason for hiding this comment

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

아주 소소한 의견이긴 한데, InputView에서 이 역할까지 하는건 어떨까 생각이 들었습니다 😄
사용자로부터 문자값을 입력 받고 그걸 처리한 결과를 전달하는 역할을 부여하면
좀 더 생성자 부분도 정리 될 것 같습니다 😄

public RaceService(List<String> names, int roundInput) {
// ...
}

개선까지는 아니고 정리 차원에서 이런 의견도 있구나 정도만 생각해주시면 될 것 같습니다 🙇

@@ -14,7 +15,7 @@ private void validateName(String name) {
if (name == null || name.isBlank()) {
throw new RuntimeException("자동차 이름은 공백일 수 없습니다.");
}
if (name.length() > 5) {
if (name.length() > MAX_CAR_NAME_LENGTH) {
Copy link

Choose a reason for hiding this comment

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

요청사항 반영 💯

for (String name : names) {
tempCars.add(new Car(name));
}
this.cars = tempCars;
Copy link

Choose a reason for hiding this comment

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

이 부분은 stream으로 간결하게 해볼 수 있을 것 같습니다 😄

// 16이전 
this.cars = names.stream()
		.map(Car::new)
		.collect(Collectors.toList());
// 16이후 
this.cars = names.stream()
		.map(Car::new)
		.toList();


import java.util.List;

public class RaceResult {
Copy link

Choose a reason for hiding this comment

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

유틸도 좋은데 아예 List<CarInfo>를 관리하는 객체여도 나쁘지 않을 것 같습니다.
경기 결과를 담당하고 그에 따른 비즈니스 요건을 처리하는 역할을 담당하면 될 것 같네요 😄
다만 CarInfo를 상속받아 처리하는 다양한 Info들이 존재한다면 유틸 구조가 적당할 것 같습니다.

@neojjc2 neojjc2 merged commit 8eecc16 into next-step:jorippppong Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants