[자동차 경주] 5단계 - 자동차 경주(리팩토링) #6077

luku756 · 2025-03-21T04:36:59Z

mvc 에 맞게 패키지를 이동하고, 각 객체를 좀더 객체지향에 맞게 수정했습니다.
클래스 함수나 getter 의 사용을 최소화하고 객체에 메시지를 보내는 쪽으로 변경했습니다.

확인 부탁드립니다.

wooobo

5단계 잘 진행해주셨습니다!

몇가지 코멘트 남겨놓았어요~
확인 후 재요청 부탁드립니다~

그럼 즐거운 불금되세요~

wooobo · 2025-03-21T10:59:46Z

src/main/java/racing/model/Car.java

+    public boolean isPosition(int position) {
+        return this.position.isPosition(position);


Suggested change

public boolean isPosition(int position) {

return this.position.isPosition(position);

public boolean isPosition(Position position) {

return this.position.isSame(position);

객체지향 생활 체조 원칙
규칙 3: 모든 원시값과 문자열을 포장한다.

이런 관점으로도 해볼수있겠네요 🤔

@wooobo
수정했습니다.

wooobo · 2025-03-21T11:01:40Z

src/main/java/racing/model/CarName.java

    public static final int MAX_NAME_LENGTH = 5;
    private final String name;

    public CarName(String name) {
-        if (checkNameLengthExceed(name)){
+        if (checkNameLengthExceed(name)) {
            throw new IllegalArgumentException("차 이름은 5자 이하여야 합니다. input: " + name);


Suggested change

throw new IllegalArgumentException("차 이름은 5자 이하여야 합니다. input: " + name);

throw new IllegalArgumentException("차 이름은 5자 이하여야 합니다. input: " + name);

MAX_NAME_LENGTH = 6으로 변경되면 에러메시지도 같이 변경되기 쉬울까요?
놓치기 쉬운 부분아닐까요?

wooobo · 2025-03-21T11:03:38Z

src/main/java/racing/model/FixedCar.java

@@ -0,0 +1,33 @@
+package racing.model;
+
+public class FixedCar {


CarsAtTurn, FixedCar 어떻게 해서 나온객체이고 있어야하는 이유가 무엇인지 설명해주실수 있으실까요?

PlayHistory 객체에서는 게임 전체의 진행 상황을 저장합니다.
그러기 위해서는 각 턴, 각 차의 상태를 저장할 필요가 있습니다.
요구 조건을 감안하면, 각 차의 상태를 연속적으로 가지고 있기보다는 각 턴에서 각 차의 상태를 가지고 있는 편이 좋습니다.
하여 각 턴에서, 차들의 상태를 가지고 있는 객체가 필요했고 이게 CarsAtTurn 입니다.

FixedCar는 변경되지 않는 한 시점 (과거) 의 차의 상태를 저장하기 위한 객체입니다.
carName, carPosition이 있지만 모든 필드가 final 이고, 외부에서 상태를 변경할 방법을 제공하지 않습니다.

만약 CarsAtTurn 에서 List<FixedCar> 대신 List<Car> 를 지닌다면, 해당 차의 상태가 변경될 가능성이 있습니다.
과거의 기록이 변경되는 것은 이상한 동작이기 때문에 FixedCar가 필요했습니다.

말씀하신것은 불변객체 같다고 생각이 들었는데요!

public class Car { ... public Car move(int number) { if (canProgress(number)) { return new Car(this.name, this.position.progressed()); } return this; } ...

Car를 관리하는 입장에서 상태가 어딘가에서 변경되는것에 대한 이슈라면 불변으로 만들면 어쩌면 그게 FiexdCar 처럼 될수있는게 아닐까요?

CarsAtTurn은 Cars라고 표현할수있지 않을까요?
저의 관점은 CarsAtTurn은 부르는곳에서의 이름이 아닐까생각이들어서요!

Cars carsAtTurn = new Cars(..);

@wooobo
FixedCar는 불변 객체가 맞습니다.

FixedCar 의 이름이 '움직이지 않는 차' 로 여겨져서 그런 것 같은데, 해당 객체의 정확한 의미는 'Car 의 스냅샷' 입니다.
둘 사이에 의미가 다르므로 같은 객체로 만들어서는 안된다고 생각했습니다.

그 외에도 move 함수의 존재 여부에는 차이가 있습니다.
List<Car> cars 가 있을 때, Car 가 불변이라 변경되지 않는다고 가정하겠습니다.

만약 아래와 같은 작업을 한다면, 가지고 있는 값이 달라집니다.

cars = cars.stream().map(c -> c.move()).collect(Collectors.toList());

하지만, FixedCar의 경우 move가 존재하지 않으므로 위와 같은 액션을 할 수 없습니다.
그러므로 별도의 객체 FixedCar 를 생성하여 스냅샷으로 사용하는 것이 더 안전하다고 생각했습니다.

저는 FixedCar 의 이름이 나쁘지 않다고 생각했는데, 오해의 여지가 있다면 CarSnapshot 같은 이름이 나을지도 모르겠네요.
수정했습니다.

CarsAtTurn은 Cars라고 표현할수있지 않을까요? 저의 관점은 CarsAtTurn은 부르는곳에서의 이름이 아닐까생각이들어서요!

Cars carsAtTurn = new Cars(..);

Cars 는 너무 넓은 의미를 가지고 있습니다.
CarsAtTurn 객체의 의의는 특정 시점에서의 자동차들(스냅샷) 입니다.
(List 가 아니라 List 입니다)

만약 다른 이름으로 한다면 아래와 같은 이름들도 괜찮을 것 같네요.

CarSnapshots

TurnSnapshot

TurnRecord

CarsInTurn

FixedCar -> CarSnapshot 에 맞춰, CarsAtTurn -> TurnSnapshot 으로 수정했습니다.

네! 답변 감사합니다!

충분히 고민 후 내린 결론이라면 충분히 좋은 결과라고 생각합니다! 😄

wooobo · 2025-03-21T11:10:31Z

src/main/java/racing/model/PlayHistory.java

+    }
+
+    public List<String> findWinners() {
+        CarsAtTurn lastTurn = this.positions.get(this.positions.size() - 1);


Suggested change

CarsAtTurn lastTurn = this.positions.get(this.positions.size() - 1);

CarsAtTurn lastTurn = lastTurn();

복잡한 로직을 적절한 의미를 가진 메소드로 분리시키면 어떨까요?

wooobo · 2025-03-21T11:11:17Z

src/test/java/racing/model/CarNameTest.java

+class CarNameTest {
+
+    @Test
+    void 다섯자넘는_이름_에러() {


좀 더 일반화된이름으로 표현하면 어떨까요?
다섯자에서 여섯자로 변경되면 테스트 이름이 변경되는건 좀 귀찮지 않을까요?

wooobo

피드백에 대해고민해주셔서 감사드립니다! 🙇

몇가지 코멘트 남겨 놓았는데 확인 후 재요청 부탁드립니다~

그럼 좋은 주말되세요~

wooobo · 2025-03-22T13:56:40Z

src/main/java/racing/model/FixedCar.java

@@ -0,0 +1,33 @@
+package racing.model;
+
+public class FixedCar {


말씀하신것은 불변객체 같다고 생각이 들었는데요!

public class Car { ... public Car move(int number) { if (canProgress(number)) { return new Car(this.name, this.position.progressed()); } return this; } ...

Car를 관리하는 입장에서 상태가 어딘가에서 변경되는것에 대한 이슈라면 불변으로 만들면 어쩌면 그게 FiexdCar 처럼 될수있는게 아닐까요?

wooobo · 2025-03-22T14:01:56Z

src/main/java/racing/model/FixedCar.java

@@ -0,0 +1,33 @@
+package racing.model;
+
+public class FixedCar {


CarsAtTurn은 Cars라고 표현할수있지 않을까요?
저의 관점은 CarsAtTurn은 부르는곳에서의 이름이 아닐까생각이들어서요!

Cars carsAtTurn = new Cars(..);

wooobo · 2025-03-22T14:16:40Z

src/main/java/racing/view/ResultView.java

+        for (int i=0; i<carsAtTurn.getCarSize(); i++) {
+            printCarPosition(carsAtTurn.getCarName(i), carsAtTurn.getCarPosition(i));
        }


Suggested change

for (int i=0; i<carsAtTurn.getCarSize(); i++) {

printCarPosition(carsAtTurn.getCarName(i), carsAtTurn.getCarPosition(i));

}

for (FixedCar car : carsAtTurn.getCarPositions()) {

printCarPosition(car.getName(), car.getPosition());

}

만들어주신 함수를 활용하면 이렇게도 가능하겠네요!
최대한 index를 통한 접근보다는 메소드호출(메세지 전달)을 통해 객체지향에 대한 접근방법을 고민해보아도 좋을것같네요!

@wooobo
CarsAtTurn가 iterator를 사용하게 수정하여 코드를 간소화하였습니다.

wooobo · 2025-03-22T14:18:33Z

src/main/java/racing/view/ResultView.java


 public class ResultView {

    public static void printResult(RacingResultDto racingResultDto) {
        System.out.println("실행 결과");
-        for (CarPositions carPositions : racingResultDto.getPlayHistory().getPositions()) {
-            printTurnResult(racingResultDto.getCarNames(), carPositions);
+        for (CarsAtTurn carsAtTurn : racingResultDto.getPlayHistory().getPositions()) {


Suggested change

for (CarsAtTurn carsAtTurn : racingResultDto.getPlayHistory().getPositions()) {

for (CarsAtTurn carsAtTurn : racingResultDto.carAtTurns()) {

디미터 법칙에 대해 고민해보아요~

@wooobo
PlayHistory가 iterator를 사용하게 수정하여 호출을 간소화했습니다.

wooobo

5단계까지 수고 많으셨습니다~

몇가지 코멘트 남겨놓았어요, 확인 부탁드립니다~
그럼 이만 merge진행하겠습니다!

그럼 남은 미션도 응원하겠습니다!
좋은 밤 되세요~

wooobo · 2025-03-23T13:41:20Z

src/main/java/racing/model/TurnSnapshot.java

+                .collect(Collectors.toList());
+    }
+
+    private List<CarSnapshot> findCarsAtPositions(int targetPosition) {


Suggested change

private List<CarSnapshot> findCarsAtPositions(int targetPosition) {

private List<CarSnapshot> findCarsAt(Position targetPosition) {

Position 객체라면 굳이 이름에 Position을 붙이지 않아도 될수있지않을까? 생각이 되네요 🤔

wooobo · 2025-03-23T13:42:39Z

src/main/java/racing/model/FixedCar.java

@@ -0,0 +1,33 @@
+package racing.model;
+
+public class FixedCar {


네! 답변 감사합니다!

충분히 고민 후 내린 결론이라면 충분히 좋은 결과라고 생각합니다! 😄

wooobo · 2025-03-23T13:45:38Z

src/test/java/racing/model/CarPositionTest.java

+    void 맥스() {
+        CarPosition carPosition = new CarPosition(10);
+        assertThat(carPosition.max(15)).isEqualTo(15);
+        assertThat(carPosition.max(5)).isEqualTo(10);


assertAll 을 활용해볼수있겠네요~

wooobo · 2025-03-23T13:46:14Z

src/test/java/racing/model/CarPositionTest.java

+    void 위치_확인() {
+        CarPosition carPosition = new CarPosition(10);
+        assertThat(carPosition).isEqualTo(new CarPosition(10));
+        assertThat(carPosition).isNotEqualTo(new CarPosition(15));


첫번째로 isEqualTo로 검증하고 있는데, assertThat(carPosition).isNotEqualTo(new CarPosition(15)); 이게 필요한지 모르겠어요! 🤔

wooobo · 2025-03-23T13:57:33Z

src/main/java/racing/model/RacingSession.java

+    }
+
+    public int getProgressNumber() {
+        Random random = new Random();


Random에 대한 의존성을 덜기위해 전략패턴을 활용해보면 좋을것 같아요.
추가적으로 테스트도 용이해질수있어요~
한번 고민해보셔도 좋을것 같네요 😄

kakao-luka-redwing added 3 commits March 21, 2025 10:42

refactor 패키지 이동

69b30c2

refactor Static 제거

bf82d18

refactor 히스토리 객체 분리

a7e8aad

wooobo requested changes Mar 21, 2025

View reviewed changes

luku756 added 2 commits March 22, 2025 00:26

refactor name 피드백 반영

97da0f3

refactor isPosition 에서 인자로 CarPosition 받기

6bad1f1

wooobo requested changes Mar 22, 2025

View reviewed changes

luku756 added 2 commits March 23, 2025 02:15

refactor 객체에서 iterator 를 사용하게 변경

85700a3

refactor FixedCar -> CarSnapshot, CarsAtTurn -> TurnSnapshot

ad0a2f2

wooobo approved these changes Mar 23, 2025

View reviewed changes

wooobo merged commit 41003fe into next-step:luku756 Mar 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[자동차 경주] 5단계 - 자동차 경주(리팩토링) #6077

[자동차 경주] 5단계 - 자동차 경주(리팩토링) #6077

luku756 commented Mar 21, 2025

wooobo left a comment

wooobo Mar 21, 2025

luku756 Mar 21, 2025

wooobo Mar 21, 2025

wooobo Mar 21, 2025

luku756 Mar 21, 2025 •

edited

Loading

wooobo Mar 22, 2025

wooobo Mar 22, 2025

luku756 Mar 22, 2025 •

edited

Loading

luku756 Mar 22, 2025 •

edited

Loading

wooobo Mar 23, 2025

wooobo Mar 21, 2025

wooobo Mar 21, 2025

wooobo left a comment

wooobo Mar 22, 2025

wooobo Mar 22, 2025

wooobo Mar 22, 2025

luku756 Mar 22, 2025 •

edited

Loading

wooobo Mar 22, 2025

luku756 Mar 22, 2025

wooobo left a comment

wooobo Mar 23, 2025

wooobo Mar 23, 2025

wooobo Mar 23, 2025

wooobo Mar 23, 2025

wooobo Mar 23, 2025

		public boolean isPosition(int position) {
		return this.position.isPosition(position);

	throw new IllegalArgumentException("차 이름은 5자 이하여야 합니다. input: " + name);
	throw new IllegalArgumentException("차 이름은 5자 이하여야 합니다. input: " + name);

		@@ -0,0 +1,33 @@
		package racing.model;

		public class FixedCar {

	CarsAtTurn lastTurn = this.positions.get(this.positions.size() - 1);
	CarsAtTurn lastTurn = lastTurn();

	for (CarsAtTurn carsAtTurn : racingResultDto.getPlayHistory().getPositions()) {
	for (CarsAtTurn carsAtTurn : racingResultDto.carAtTurns()) {

	private List<CarSnapshot> findCarsAtPositions(int targetPosition) {
	private List<CarSnapshot> findCarsAt(Position targetPosition) {

[자동차 경주] 5단계 - 자동차 경주(리팩토링) #6077

[자동차 경주] 5단계 - 자동차 경주(리팩토링) #6077

Conversation

luku756 commented Mar 21, 2025

wooobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luku756 Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luku756 Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

luku756 Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luku756 Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luku756 Mar 21, 2025 •

edited

Loading

luku756 Mar 22, 2025 •

edited

Loading

luku756 Mar 22, 2025 •

edited

Loading

luku756 Mar 22, 2025 •

edited

Loading