5단계 - 자동차 경주(리팩터링) #918

relkimm · 2022-11-21T15:23:29Z

안녕하세요 인근님~!
5단계 우선 완료되어서 리뷰 요청 드립니다.🙇‍♂️

relkimm · 2022-11-21T15:28:18Z

src/main/kotlin/racingcar/domain/Car.kt

+    constructor(car: Car) : this(
+        id = car.id,
+        name = car.name,
+        position = car.position,
+    )
+
+    fun move(movePolicy: MovePolicy): Car {
        if (movePolicy.canMove()) {
            this.position = this.position.forward()
        }
+        return Car(car = this)


immutable 하게 만들어 보았습니다~🙆‍♂️

새로운 car 객체를 잘 생성해주신 거 같은데요~
position 프로퍼티를 불변(val)으로 가지고 있어야 진정한 불변 객체가 아닐까 싶습니다! 🤔
현재는 car 객체의 position 값이 var 로 선언되어있네요~

relkimm · 2022-11-21T15:30:11Z

src/main/kotlin/racingcar/domain/CarGroup.kt

+class CarGroup(private var cars: List<Car>) {
    fun move() {
-        this.cars.forEach { car ->
+        this.cars = this.cars.map { car ->
            val oil = OilStation.generateOilRandomly()
            car.move(movePolicy = OilPolicy(oil = oil))
        }


CarGroup 이 move 할 때도 immutable 하게 cars 를 반환할까 싶었는데!
아래 getPositions api 를 사용하는 것도 괜찮은 것 같아서 그대로 두었습니다.😭😭

CarGroup 이 move 할 때도 immutable 하게 cars 를 반환하는 것이 좀 더 안전한 프로그램 아닐까 싶은데요~ 🤔
getPositions() 를 호출하기 전까지 CarGroup 이 변경 불가능하다는 것을 보장받는 것이 전 좋아보입니다!

relkimm · 2022-11-21T15:30:33Z

src/main/kotlin/racingcar/domain/CarName.kt

+value class CarName(
+    val value: String,
+) {
+    init {
+        require(this.value.isNullOrBlank().not()) { "자동차 이름은 비어 있을 수 없습니다." }
+        require(this.value.length <= MAX_NAME_LENGTH) { "자동차 이름은 5 글자를 초과할 수 없습니다." }
+    }


자동차 이름이 비어있을 경우의 유효성 검사도 추가해 보았어요~

꼼꼼하게 피드백 반영해주셨네요~ 👍

relkimm · 2022-11-21T15:33:07Z

src/main/kotlin/racingcar/RoundResult.kt

-package racingcar
-
-data class RoundResult(
-    val carPositions: List<CarPosition>
-)


RoundResult 가 도메인 모델이라기 보다는 화면에 표시하기 위한 dto 느낌이 들어서
RoundResult 대신 CarGroup 의 getCarPositions 를 활용하는 방향으로 리팩토링해 보았습니다.🙇‍♂️

relkimm · 2022-11-21T15:39:13Z

src/main/kotlin/racingcar/view/container/Container.kt

+package racingcar.view.container
+
+import racingcar.view.component.Component
+
+interface Container : Component


요 친구는 프론트 개발 패턴 중에서 container / presentational 컴포넌트 패턴 을 표현하기 위해 만들어 보았네요!

container 컴포넌트 는 (Container) 로직을 수행하거나 store 에서 데이터를 조회하고
presentational 컴포넌트 에게 데이터를 전달하는 역할을 수행하고 있고

presentational 컴포넌트(Component) 는 단순히 생성자 통해서 전달 받은 데이터를
화면에 표시하는 역할을 수행하도록 만들어 보았네요~!

view 영역에 대한 설계를 많이 고민하시고 적용해주셨네요~
프론트 개발 영역은 제가 잘 모르는 부분이라 해당 패턴에 대해서는 코멘트를 드리기 쉽지 않네요.. 😭
자동차 미션의 목적에 좀 더 초점을 맞춰서 리뷰 드리도록 하겠습니다~ 🙏

이때 domain 영역이 view 에 의존하지 않도록 잘 분리해주신 거 같습니다
아쉬웠던 건 각종 InputComponent 가 생성되자마자 onCommand()를 바로 실행해 입력을 받고 있는데
입력을 받도록 명령하는 것은 controller 영역에서 따로 통제하고 controller 를 통해 입력값을 모델에 전달하는 것이 역할 분리가 좀 더 낫지 않을까요?

마찬가지로 ResultComponent 는 controller 에 의해 전달받은 데이터를 그려주는 역할만 하는 것이 적절해보이는데
render() 메소드 안에서 store 에 있는 모델 데이터를 받아오는 역할까지 수행해 여러 책임을 갖는 것으로 보이네요 🤔

현재 구조에서는 controller 의 영역이 따로 없이 view 에서 모델 데이터를 통제하는 역할까지 수행하는 것 같네요~
MVC 패턴을 고민해보면서 관심사의 분리를 시도해보는 것은 어떨까요? 🙇‍♂️

relkimm · 2022-11-21T15:42:41Z

src/main/kotlin/racingcar/view/container/RoundContainer.kt

+class RoundContainer(private val round: Round) : Container {
+    init { this.roundStart() }
+    private fun roundStart() {
+        val carGroup = CarGroupStore.getState()
+        this.round.start(carGroup = carGroup)
+        CarGroupStore.setState(state = carGroup)
+    }


컴포넌트가 마운트 되었을 때의 동작을 어디서 수행하면 좋을까? 고민을 하다가 코틀린의 init 함수를 활용해 보았습니다~

relkimm · 2022-11-21T15:44:33Z

src/main/kotlin/racingcar/view/store/Store.kt

+abstract class Store<T> {
+    protected val listeners = HashSet<Listener>()
+
+    abstract fun getState(): T
+    abstract fun setState(state: T)
+    abstract fun subscribe(listener: Listener): UnSubscribe
+}


listeners 와 subscribe 함수를 추가해 보았는데! UI 가 독립적으로 그리고 비동기적으로
랜더링된다면 의미가 있을 것 같은데 지금은 동기적으로 실행되고 있다 보니, 활용을 못 했네요😭😭

relkimm · 2022-11-21T16:07:36Z

src/main/kotlin/racingcar/view/store/Store.kt

-abstract class Store<T> {
-    protected val listeners = HashSet<Listener>()
+open class Store<T>(initialState: T) {
+    private var state = initialState
+    private val listeners = HashSet<Listener>()

-    abstract fun getState(): T
-    abstract fun setState(state: T)
-    abstract fun subscribe(listener: Listener): UnSubscribe
+    fun getState(): T {
+        return this.state
+    }
+    fun setState(state: T) {
+        this.state = state
+        this.listeners.forEach { listener -> listener() }
+    }
+    fun subscribe(listener: Listener): UnSubscribe {
+        listeners.add(listener)
+        return { listeners.remove(listener) }
+    }
 }


전역 상태 관리할 수 있는 store 를 공통으로 사용하기 위해 요렇게 개선해 보았습니다~🙆‍♂️

YooInKeun

5단계 미션 수행하시느라 고생 많으셨습니다! 👍
몇가지 코멘트 남겼으니 확인 부탁드려요~
자동차 미션 얼마 남지 않았네요!
마지막까지 화이팅입니다~ 🔥 🔥

YooInKeun · 2022-11-22T13:49:20Z

src/main/kotlin/racingcar/domain/Car.kt

+    constructor(car: Car) : this(
+        id = car.id,
+        name = car.name,
+        position = car.position,
+    )
+
+    fun move(movePolicy: MovePolicy): Car {
        if (movePolicy.canMove()) {
            this.position = this.position.forward()
        }
+        return Car(car = this)


새로운 car 객체를 잘 생성해주신 거 같은데요~
position 프로퍼티를 불변(val)으로 가지고 있어야 진정한 불변 객체가 아닐까 싶습니다! 🤔
현재는 car 객체의 position 값이 var 로 선언되어있네요~

YooInKeun · 2022-11-22T13:53:34Z

src/main/kotlin/racingcar/domain/CarGroup.kt

+class CarGroup(private var cars: List<Car>) {
    fun move() {
-        this.cars.forEach { car ->
+        this.cars = this.cars.map { car ->
            val oil = OilStation.generateOilRandomly()
            car.move(movePolicy = OilPolicy(oil = oil))
        }


CarGroup 이 move 할 때도 immutable 하게 cars 를 반환하는 것이 좀 더 안전한 프로그램 아닐까 싶은데요~ 🤔
getPositions() 를 호출하기 전까지 CarGroup 이 변경 불가능하다는 것을 보장받는 것이 전 좋아보입니다!

YooInKeun · 2022-11-22T13:56:48Z

src/main/kotlin/racingcar/domain/CarName.kt

+    val value: String,
+) {
+    init {
+        require(this.value.isNullOrBlank().not()) { "자동차 이름은 비어 있을 수 없습니다." }


마이너하지만 .not() 을 사용하게 되면서 가독성이 약간은 떨어지게 된 거 같아요~
아래와 같이 좀 더 간결하게 작성하는 것도 좋을 거 같습니다!

Suggested change

require(this.value.isNullOrBlank().not()) { "자동차 이름은 비어 있을 수 없습니다." }

require(this.value.isNotBlank()) { "자동차 이름은 비어 있을 수 없습니다." }

YooInKeun · 2022-11-22T13:58:17Z

src/main/kotlin/racingcar/domain/CarName.kt

+value class CarName(
+    val value: String,
+) {
+    init {
+        require(this.value.isNullOrBlank().not()) { "자동차 이름은 비어 있을 수 없습니다." }
+        require(this.value.length <= MAX_NAME_LENGTH) { "자동차 이름은 5 글자를 초과할 수 없습니다." }
+    }


꼼꼼하게 피드백 반영해주셨네요~ 👍

YooInKeun · 2022-11-22T14:01:52Z

src/main/kotlin/racingcar/view/container/Container.kt

+package racingcar.view.container
+
+import racingcar.view.component.Component
+
+interface Container : Component


view 영역에 대한 설계를 많이 고민하시고 적용해주셨네요~
프론트 개발 영역은 제가 잘 모르는 부분이라 해당 패턴에 대해서는 코멘트를 드리기 쉽지 않네요.. 😭
자동차 미션의 목적에 좀 더 초점을 맞춰서 리뷰 드리도록 하겠습니다~ 🙏

이때 domain 영역이 view 에 의존하지 않도록 잘 분리해주신 거 같습니다
아쉬웠던 건 각종 InputComponent 가 생성되자마자 onCommand()를 바로 실행해 입력을 받고 있는데
입력을 받도록 명령하는 것은 controller 영역에서 따로 통제하고 controller 를 통해 입력값을 모델에 전달하는 것이 역할 분리가 좀 더 낫지 않을까요?

마찬가지로 ResultComponent 는 controller 에 의해 전달받은 데이터를 그려주는 역할만 하는 것이 적절해보이는데
render() 메소드 안에서 store 에 있는 모델 데이터를 받아오는 역할까지 수행해 여러 책임을 갖는 것으로 보이네요 🤔

현재 구조에서는 controller 의 영역이 따로 없이 view 에서 모델 데이터를 통제하는 역할까지 수행하는 것 같네요~
MVC 패턴을 고민해보면서 관심사의 분리를 시도해보는 것은 어떨까요? 🙇‍♂️

YooInKeun · 2022-11-22T14:04:56Z

src/test/kotlin/racingcar/domain/CarTest.kt

+                // when
+                sut.move(movePolicy = OilPolicy(oil = 충분한_연료))
+                //
+                sut.position shouldBe Position(value = 1)


car 는 position 값이 변할 수 있는 객체네요~
불변 객체를 만드는 것을 다시 시도해보면 어떨까요?

relkimm added 16 commits November 21, 2022 20:51

refactor: 패키지를 domain & view 로 분리

41be7a3

refactor: CarName 글자수 검사 시 require 사용하도록 변경

32edb84

feat: 기존 자동차를 통해 새로운 자동차 생성하는 로직 추가

3392656

refactor: 자동차가 주행하고 나서 새로운 자동차를 반환하도록 수정

ffe32fd

refactor: CarGroup 에서 자동차가 주행하더라도 불변하도록 수정

ccb8ca3

refactor: Store pub/sub 구조로 변경

28acace

feat: 라운드 관련 UI 를 표시하는 RoundComponent 추가

ee967ec

refactor: Winner 를 표시하는 UI container/presentational 컴포넌트로 분리

51ab1c0

refactor: Round 를 표시하는 UI container/presentational 컴포넌트로 분리

b31626d

feat: 자동차 위치를 표시하는 UI container/presentational 컴포넌트 추가

e8b33e2

refactor: 불필요한 RoundResult 정리

81e58af

refactor: RoundList 와 Round 를 표시하는 UI 분리

459562a

refactor: DistanceComponent 네이밍 변경

b4497de

refactor: 프로퍼티에는 this 붙이도록 수정

a0bb3ab

refactor: RoundContainer 에서 라운드 시작하는 로직 render 와 분리

6aed63f

feat: 자동차 이름 공백인 경우 유효성 검사 추가

3e6350e

relkimm marked this pull request as ready for review November 21, 2022 15:25

relkimm commented Nov 21, 2022

View reviewed changes

refactor: store 공통으로 사용할 수 있도록 개선

133c53c

relkimm commented Nov 21, 2022

View reviewed changes

YooInKeun requested changes Nov 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5단계 - 자동차 경주(리팩터링) #918

5단계 - 자동차 경주(리팩터링) #918

relkimm commented Nov 21, 2022

relkimm Nov 21, 2022 •

edited

Loading

YooInKeun Nov 22, 2022

relkimm Nov 21, 2022 •

edited

Loading

YooInKeun Nov 22, 2022

relkimm Nov 21, 2022

YooInKeun Nov 22, 2022

relkimm Nov 21, 2022 •

edited

Loading

relkimm Nov 21, 2022

YooInKeun Nov 22, 2022 •

edited

Loading

relkimm Nov 21, 2022 •

edited

Loading

relkimm Nov 21, 2022

relkimm Nov 21, 2022

YooInKeun left a comment

YooInKeun Nov 22, 2022

YooInKeun Nov 22, 2022

YooInKeun Nov 22, 2022

YooInKeun Nov 22, 2022

YooInKeun Nov 22, 2022 •

edited

Loading

YooInKeun Nov 22, 2022 •

edited

Loading

	require(this.value.isNullOrBlank().not()) { "자동차 이름은 비어 있을 수 없습니다." }
	require(this.value.isNotBlank()) { "자동차 이름은 비어 있을 수 없습니다." }

5단계 - 자동차 경주(리팩터링) #918

Are you sure you want to change the base?

5단계 - 자동차 경주(리팩터링) #918

Conversation

relkimm commented Nov 21, 2022

relkimm Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relkimm Nov 21, 2022 • 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

relkimm Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YooInKeun Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

relkimm Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YooInKeun 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

YooInKeun Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

YooInKeun Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

relkimm Nov 21, 2022 •

edited

Loading

relkimm Nov 21, 2022 •

edited

Loading

relkimm Nov 21, 2022 •

edited

Loading

YooInKeun Nov 22, 2022 •

edited

Loading

relkimm Nov 21, 2022 •

edited

Loading

YooInKeun Nov 22, 2022 •

edited

Loading

YooInKeun Nov 22, 2022 •

edited

Loading