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

자동차 경주 3단계: 자동차 경주 #1381

Merged
merged 7 commits into from
Oct 29, 2023
Merged

Conversation

jdalma
Copy link

@jdalma jdalma commented Oct 28, 2023

안녕하세요 ㅎㅎ 3단계도 잘 부탁드립니다.

구현하면서 고민한 부분과 미흡하다고 생각되는 부분들 입니다.

  1. Car객체의 전진 카운트가 가변 필드라는점
  2. CarStadium은 경기 결과의 문자열을 반환하기만 하여서 테스트가 하기 힘든 점
  3. CarStadium의 오픈된 메소드가 테스트 코드를 작성하기 힘들어서 private 함수들도 테스트를 작성하기 힘든 점

테스트가 쉬운 코드를 작성하기와 간단한 문제이지만 경기장과 자동차의 책임을 나누기가 힘들었습니다
어떻게 하면 테스트가 쉬운 코드를 작성할 수 있을까요?? 그리고 전진할 수 있는지 판단하는 것은 정적으로 빼기보다 자동차 객체가 가지고 있어야 할까요??

문득 TDD를 사용하는 이유 중에 한 가지가 테스트하기 쉬운 코드를 작성하기 위해서 일까 싶기도 하네요.. 구현에 먼저 집중해서 테스트하기 어려운 코드를 작성했나라는 생각이 드네요

@namjackson
Copy link

테스트가 쉬운 코드를 작성하기와 간단한 문제이지만 경기장과 자동차의 책임을 나누기가 힘들었습니다

객체의 책임을 고민해보면 어떨까요?
전체적인 요구사항을 이해하고,
객체의 역할을 정하고, 테스트 케이스를 작성해보면 어떨까요?
이 부분은 정답이 없는 부분이기도 하고, 천천히 미션을 진행하시다보면 감을 잡으실수 있을거라고 생각이 들어요! 😄 남은 미션도 끝까지 완주해보아요!

어떻게 하면 테스트가 쉬운 코드를 작성할 수 있을까요?? 그리고 전진할 수 있는지 판단하는 것은 정적으로 빼기보다 자동차 객체가 가지고 있어야 할까요??

이부분 또한 정답은 없는 부분인거 같아요
자동차 객체에서 이동로직을 엔진이라는 객체로 분리할수도 있습니다!
각각의 구현방법에 장단점있고, 취향에 따라 다를수 있을거 같아요!
각각 구현방법을 생각해서, 더 나은 구조가 무엇인지 고민해봐도 좋을거같아요!

Copy link

@namjackson namjackson left a comment

Choose a reason for hiding this comment

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

안녕하세요 현준님!
3단계 고생하셨습니다!
몇가지 고민할법한 포인
트를 코멘트로 남겼으니, 확인해주세요 😄

class Car(
private val carNumber: Int
) {
private var moveForwardCount: Int = 0

Choose a reason for hiding this comment

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

Car객체의 전진 카운트가 가변 필드라는점

getter 함수를 만들어주기 보다는, backing-fields를 활용해봐도 좋을거 같아요!
https://kotlinlang.org/docs/properties.html#backing-fields

private val numberOfTrials: Int
) {

private val cars: List<Car> = IntRange(1, numberOfCars).map(::Car)

Choose a reason for hiding this comment

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

List(numberOfCars) { Car(it) }
위같은 문법도 활용가능합니다!

src/main/kotlin/racingcar/domain/CarStadium.kt Outdated Show resolved Hide resolved
Comment on lines 3 to 6
object Environment {

val CAR_RACING_RANDOM_RANGE = IntRange(0, 9)
const val CAR_MOVE_FORWARD_CONDITION = 4

Choose a reason for hiding this comment

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

매직넘버를 따로 관리하기보단, 필요한 객체에서 선언해서 사용해보면 어떨까요?
위의 값들은 CarMove에서 관리하면 좋을거같아요!

}

private fun moving() {
cars.filter { CarMove.canMoveForward() }

Choose a reason for hiding this comment

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

CarMove.canMoveForward()를 테스트를 고려해보아요!
canMoveForward를 추상화해보면 어떨까요?

Comment on lines 12 to 13
fun gameStart() : String = buildString {
this.append(RACE_RESULT_FIRST_LINE.message)

Choose a reason for hiding this comment

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

CarStadium이라는 도메인 객체에서
RACE_RESULT_FIRST_LINE로 String 를 리턴하는건 View 관련된 로직은 아닐까요?
콘솔프로그램이 아니여도 동일한 로직이 필요할까요? 고민해보면 좋을거같아요!

Choose a reason for hiding this comment

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

CarStadium은 경기 결과의 문자열을 반환하기만 하여서 테스트가 하기 힘든 점

게임 이후, History 객체를 반환하거나, Cars를 통해 게임 결과를 따로 그려주면 어떨까요?

}
}

private fun getMovingResult(symbol : Char = '-'): String = buildString {

Choose a reason for hiding this comment

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

동일하게 View 관련 로직은 아닐까요?


class CarStadiumTest : BehaviorSpec({

Given("자동차의 대수와 전진 시도 횟수가 정수로 주어지고") {

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.

CarStadium의 오픈된 메소드가 테스트 코드를 작성하기 힘들어서 private 함수들도 테스트를 작성하기 힘든 점

랜덤기능은 테스트 불가능한 기능으로, 위의 코멘트처럼 인터페이스로 분리하여, FAKE객체를 통해 테스트해보아요!

추가적으로 private 함수를 테스트해야한다면, 책임이 섞여있거나, 객체분리의 신호로 볼수도 있을거같아요!

jdalma and others added 5 commits October 29, 2023 14:35
Co-authored-by: namjackson <skawo32167@naver.com>
1. Car의 forwardCount 백킹 필드 적용
2. Environment의 상수 CarMove로 이동
1. CarStadium과 출력 로직 분리
2. RandomGenerator의 의존성 수정
3. CarMove 클래스화
@jdalma
Copy link
Author

jdalma commented Oct 29, 2023

  1. 콘솔프로그램이 아니여도 동일한 로직이 필요할까요? 고민해보면 좋을거같아요!
  2. 게임 이후, History 객체를 반환하거나, Cars를 통해 게임 결과를 따로 그려주면 어떨까요?
  3. 랜덤기능은 테스트 불가능한 기능으로, 위의 코멘트처럼 인터페이스로 분리하여, FAKE객체를 통해 테스트해보아요!

말씀해주신 세 가지 포인트들을 고민하고 수정해보았습니다. 리뷰어님 덕분에 도메인 로직과 출력 부분이 혼재해있었고 랜덤 생성 부분은 추상화되어 있지 않아 가짜 객체를 주입할 지점이 없어서 테스트 하기 힘들었다는 것을 깨닫게 되었습니다 ㅎㅎ

Copy link

@namjackson namjackson left a comment

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 racingcar.domain

abstract class RandomGenerator<TYPE>(

Choose a reason for hiding this comment

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

Functional interface 를 참고해보아요!
https://kotlinlang.org/docs/fun-interfaces.html

Choose a reason for hiding this comment

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

실제로 Random 기능이 있는 객체는 해당 객체의 구현체네요!
추상화된 클래스에는 랜덤기능이 없어요 !

this.histories.add(histories)
}

fun getRacingHistories() = histories.toList()

Choose a reason for hiding this comment

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

getter보단 backing-properties 를 활용하면 좀더 코틀린스러울거같아요!
https://kotlinlang.org/docs/properties.html#backing-properties

data class RacingHistory(
private val move: Int
) {
fun getMovingSymbol(symbol: String) = symbol.repeat(move)

Choose a reason for hiding this comment

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

이 부분도 뷰관련 객채는 아닐까요?
문자열로 move를 보여줘야하는 건 비즈니스로직에서 불필요한거같아요!

@namjackson namjackson merged commit 8f2cadc into next-step:jdalma Oct 29, 2023
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.

None yet

2 participants