[부나] 2단계 자동차 경주 제출합니다. #64
Conversation
RacingService에서 생성자로 RandomGenerator 객체를 주입 받게 되는데, 현재 부나님이 mock라이브러리를 통해 MovementProbabilityGenerator를 테스트했지만,
OCP위배 보다는 Service에 MovementProbabilityGenerator의 강한 의존이 생기기때문에,
우선 의존 관계를 따지자면 Controller는 Service를 의존하고, Service는 도메인모델들을 의존하기 때문에, 도메인 모델이 변경된다면, Service가 변경될수 있고, 따라서 Controller까지 변경될수 있긴합니다. 반면에, 구현에는 정답이 없고, 항상 구현 방식에는 트레이드오프가 있기마련인데요, 중요한건 우선순위나, 개발원칙을 기반하여, 규칙을 정하고(팀원과 함께라면 규칙에 대한 공감도 필요할것 같아요), |
build.gradle.kts
Outdated
| @@ -14,8 +14,7 @@ dependencies { | |||
| testImplementation("org.junit.jupiter", "junit-jupiter", "5.8.2") | |||
| testImplementation("org.assertj", "assertj-core", "3.22.0") | |||
| testImplementation("io.kotest", "kotest-runner-junit5", "5.2.3") | |||
|
|
|||
| testImplementation(kotlin("test")) | |||
| testImplementation("org.mockito:mockito-inline:5.1.1") | |||
There was a problem hiding this comment.
mock 라이브러리는 저도 자주 사용하는 라이브러리인데요,
이번 미션에서는 mock라이브러리 사용하지 않고 테스트 코드를 짜보는걸 목표로 해보는게 어떨까요?
There was a problem hiding this comment.
추천해주신 것처럼 mock 라이브러리를 제거하고 Fake 객체를 사용하는 방식으로 구현을 시도해보았습니다!
이동 조건이 변경, 추가되었을 때 확장하기 편한 방식으로 구현하다보니, 기존에 RandomGenerator를 제거하고, CarRandomMoveCondition.class를 새롭게 구현하였습니다.
class FakeForSuccess : CarMoveCondition {
override operator fun invoke(): Int = SUCCESS_NUMBER
}
class FakeForFailed : CarMoveCondition {
override operator fun invoke(): Int = FAIL_NUMBER
}
구글링을 통해 찾아보니 Fake 객체를 해당 클래스 또는 인터페이스 내에 구현하는 분들이 꽤 많았습니다.
여기서 궁금했던 점은...
- '우선 이렇게 구현하는게 옳은 방식인가?'
- 'Fake 객체에 대해서는 테스트를 위한 코드를 작성해도 되는가?'
입니다..!
구현 방식이 정형화되어 있지 않다보니 어떤게 맞고 틀리다고 할 수 없지만 보편적으로 사용되는 방식이 있는지 궁금해졌습니다!
그리고 테스트를 위한 코드를 지양하라고 알고 있는데 Fake 객체는 그 범주에서 예외인지 의문이 들었습니다.
현직자의 입장에서 의견을 듣고 싶습니다..!
There was a problem hiding this comment.
실제 패키지 내에서 테스트를 위한 코드는 지양하는것이 좋습니다.
저는 Fake객체는 test 패키지 범위에서 관리되기 때문에 실제 구현이라고 보지는 않기도 합니다!
실제 프로젝트에는 객체 생성이 어렵거나, Fake가 힘든 케이스도 있기때문에, Mock 라이브러리를 많이 사용하긴 합니다.
하지만, 실제 간단한 경우에는 Fake객체 생성을 하는 경우도 많습니다 👍
| var carName: CarName = CarName(name) | ||
| private set |
There was a problem hiding this comment.
| var carName: CarName = CarName(name) | |
| private set | |
| val carName: CarName = CarName(name) |
그냥 불변으로 선언 하셔도 좋을거 같아요!
There was a problem hiding this comment.
헉 이런 실수를..!
캐치해주셔서 감사합니다!
| var position: Int = _position | ||
| private set |
There was a problem hiding this comment.
| var position: Int = _position | |
| private set | |
| class Car(name: String, private var _position: Int = 0) { | |
| ... | |
| val position: Int get() = _position |
| } | ||
| } | ||
|
|
||
| fun toDto(): CarDto = CarDto(carName.name) |
There was a problem hiding this comment.
Car객체에 CarDto에 대한 의존도가 생겼네요,
도메인 레이어의 Car에서는 Controller등에서 사용되는 객체의 존재를 알필요가 있을까요?
| } | ||
| } | ||
|
|
||
| class CarDto(_carName: String, val position: Int = 0) { |
There was a problem hiding this comment.
Dto 클래스들을 분리해주세요 😄
레이어별로 다른 패키지에 있는게 구별되기 쉽지않을까요?
There was a problem hiding this comment.
네! Dto와 Domain model을 분리하고 mapper 클래스 파일을 만들어 변환을 돕는 코드로 바꿔 보겠습니다 🙂
| var carName: CarName = CarName(_carName.trim()) | ||
| private set |
There was a problem hiding this comment.
변경이 되지않는 변수에는 기본적으로 항상 val을 사용해주세요!
| var position: Int = 0 | ||
| private set | ||
| class CarName(_name: String) { | ||
| val name: String |
There was a problem hiding this comment.
carName.name이라는 중복적인 name이 등장하게되는데
carName.value같은 변수명은 어떨까요?
There was a problem hiding this comment.
오..! name이 중복되는 네이밍보다 value를 사용하는 것에 훨씬 보기 좋아요!
하나 더 배워갑니다 😆
| interface RandomGenerator { | ||
| fun generate(): Int | ||
| } |
There was a problem hiding this comment.
인터페이스에서는 Random으로 생성된다는 정보를 포함하는게 좋은 방법일까요?
해당 인터페이스의 주요기능은 Number를 generate해주는것이지,
해당 Number가 Random으로 발생하는지, 일정한 Number가 발생하는지는
해당 인터페이스를 구현한 클래스의 책임이지 않을까욤?
There was a problem hiding this comment.
아.. 그렇군요!
메서드명과 반환타입만 보았을 때, 단순히 Integer 타입의 값을 만들어낸다고밖에 안보이는데 interface명이 Random일 필요가 없겠군요..!
리팩토링시 함께 수정해보겠습니다!
| `when`( | ||
| movementProbabilityGenerator.generate() | ||
| ) | ||
| .thenReturn(condition) |
There was a problem hiding this comment.
NumberGenerator를 인터페이스로분리한 이유는 무엇일까요?
mock라이브러리를 사용하지 않고, fake객체를 만들어서 테스트 해보는건 어떨까요?
There was a problem hiding this comment.
fake 객체에 대해 공부하고 적용해보는 시간을 가져보겠습니다!
| } | ||
|
|
||
| private fun insertCar(car: Car) = carRepository.insert(car) | ||
| private fun moveCarsRandomly(): CarsDto = | ||
| cars.moveAllRandomly(movementProbabilityGenerator).toDto() |
There was a problem hiding this comment.
(이 부분은 정답이 없는 부분이니 참고하셔서 고민만 해주세요 )
현재는 RacingService,Cars에서 generateNumber를 하는 부분의 책임을 인터페이스로 분리시켰지만,
number를 가지고 전진할지 말지에 대한 책임은 어디에 있는것이 좋을까요?
Car가 움직이는 기준이 랜덤번호가 아닌 다른 방법 변경된다면,
Car마다 움직이는 기준이 달라진다면,
Car마다 한번에 이동하는 거리가 달라진다면,
어떤구조가 좀더 유연하게 대처할수 있는 구조가 되고, 가독성이 좋은 직관적인 코드가 될까요?
좋은 코드가 무엇인지에 대해 고민하셔도 좋을거 같아요!
There was a problem hiding this comment.
이 글을 읽어보고 '어떻게 유지보수에 유연한 코드를 작성할 수 있을까?' 라는 고민을 할 수 있었습니다.
또 어떻게 변경될지에 대해 미리 고려하고 코드를 작성하시는 습관에 감탄했습니다..! 👏
number를 가지고 전진할지 말지에 대한 책임은 어디에 있는것이 좋을까요?
우선, 모든 자동차에 대해 전진할지 말지에 대한 책임은 Cars에 있다고 생각을 했습니다.
또는 Car가 자체적으로도 책임을 가질 수 있다고도 생각해 해보았는데, 개인적으로 cars에서 몇 칸씩 움직일지 car.move() 메서드의 인자로 전달해주면 해당 값만큼 이동하는 것도 방법이라고 생각했습니다.
잭슨님께서 생각하시는 또 다른 방법이 있으시다면 공유해주셨을 때 많은 도움이 될 것 같습니다 :)
Car가 움직이는 기준이 랜덤번호가 아닌 다른 방법 변경된다면,
Car마다 움직이는 기준이 달라진다면,
Car마다 한번에 이동하는 거리가 달라진다면,
말씀해주신 것처럼 주어진 요구사항이 변경되었을 때, 기존 코드를 변경해야 하는 부분이 정말 많이 생긴다는 사실을 알 수 있었습니다.
최대한 코드의 수정을 피하기 위해, Car가 움직이는 기준을 interface로 분리하고 구현체를 주입하는 방식을 떠올려 보았습니다.
// CarMoveCondition
interface CarMoveCondition {
operator fun invoke(): Int
}
// CarRandomMoveCondition
class CarRandomMoveCondition : CarMoveCondition {
override operator fun invoke(): Int =
(START_RANDOM_MOVEMENT_PROBABILITY..END_RANDOM_MOVEMENT_PROBABILITY).random()
// Cars
fun moveAll(carMoveCondition: CarMoveCondition): Cars = this.onEach { car ->
if (carMoveCondition() >= MOVE_CONDITION) {
이러한 구현 방식으로 리팩토링 하다보니 자동차 이동 조건이 변경되었을 때, CarMoveCondtion을 구현하는 또 다른 조건 클래스를 주입해주기만 하면 되는 구조로 바뀌었습니다!
즉, 기존 코드에 비해 변경에는 닫혀 있고, 확장에는 열려 있는 구조로 만들 수 있었습니다.
Car마다 움직이는 기준이 달라진다면,
이 말씀에 대해서는 Cars가 아닌, Car에게 CarMoveCondtion을 주입해주는 구현 방식을 떠올렸습니다.
그런데 이 의미가 맞는지 확신이 안 서서 아직 구현은 하지 않은 상태입니다..!
Car마다 한번에 이동하는 거리가 달라진다면,
만약 Car마다 이동하는 거리가 달라진다면, 얼마나 이동할지 Car에게 알려주면 된다고 생각했습니다.
sealed class MoveStep {
abstract fun move(): Int
}
object ZeroStep : MoveStep() {
private const val ZERO_STEP = 0
override fun move(): Int = ZERO_STEP
}
object OneStep : MoveStep() {
private const val ONE_STEP = 1
override fun move(): Int = ONE_STEP
}
sealed 클래스를 사용하여 몇 칸 움직일지 정하는 부분을 OCP에 위배되지 않게끔 시도해봤습니다.
최대한 잭슨님께서 남겨주신 피드백을 기반으로 의미를 곱씹으면서 리팩토링을 해보았는데, 제가 잘 이해했는지, 구현했는지 아직은 잘 모르겠습니다.. 🥲
긴 글 읽어주셔서 감사합니다!
There was a problem hiding this comment.
👍 👍
사실 소프트웨어 원칙중 YAGNI (You Ain't Gonna Need it)라는 원칙 또한 있습니다.
확장을 예상해서 오버 엔지니링하여 구현하게되면, 예상된 확장이 아닌 경우, 더 안좋은 결과가 나올수도 있기도하는데요,
미션이니 만큼 여러 고민과 도전을 하신부분은 좋습니다 👍 👍
하지만 변경될지에 대해 미리 고려하고 코드는 좋은 방법은 아닐수 있어요!
그렇기 때문에 클래스별로 책임을 나누어 관리하고, 좀더 자유롭게 리팩터링할수 있도록하는, 테스트 코드 기반이 중요하다는 생각이 중요한것 같아요
정말 고생많으셨습니다! 👍
|
2단계 고생많으셨습니다. |
|
|
||
| import racingcar.model.car.move.step.MoveStep | ||
|
|
||
| class Car(name: String, private var _position: Int = 0) { |
There was a problem hiding this comment.
추가적인 내용이니 참고만해주셔도 좋습니다! (다음 미션들의 내용이니 참고만해주세요!)
_position이라는 가변 상태 변수의 존재는 Car의 상태를 관리해주어야하게 만듭니다!
객체의 불변성을 유지하여 불변객체로 관리해 보아도 좋을거 같아요!
| abstract fun move(): Int | ||
| } | ||
|
|
||
| object ZeroStep : MoveStep() { |
There was a problem hiding this comment.
sealed 클래스 👍
sealed 클래스와 enum의 차이는 무엇인지 고민해보셔도 좋을거 같아요!
바로 다음 로또 미션에 나오는 내용이니 참고만해주셔도 좋아요!
| } | ||
|
|
||
| private fun insertCar(car: Car) = carRepository.insert(car) | ||
| private fun moveCarsRandomly(): CarsDto = | ||
| cars.moveAllRandomly(movementProbabilityGenerator).toDto() |
There was a problem hiding this comment.
👍 👍
사실 소프트웨어 원칙중 YAGNI (You Ain't Gonna Need it)라는 원칙 또한 있습니다.
확장을 예상해서 오버 엔지니링하여 구현하게되면, 예상된 확장이 아닌 경우, 더 안좋은 결과가 나올수도 있기도하는데요,
미션이니 만큼 여러 고민과 도전을 하신부분은 좋습니다 👍 👍
하지만 변경될지에 대해 미리 고려하고 코드는 좋은 방법은 아닐수 있어요!
그렇기 때문에 클래스별로 책임을 나누어 관리하고, 좀더 자유롭게 리팩터링할수 있도록하는, 테스트 코드 기반이 중요하다는 생각이 중요한것 같아요
정말 고생많으셨습니다! 👍
| class FakeForSuccess : CarMoveCondition { | ||
| override operator fun invoke(): Int = SUCCESS_NUMBER | ||
| } | ||
|
|
||
| class FakeForFailed : CarMoveCondition { | ||
| override operator fun invoke(): Int = FAIL_NUMBER | ||
| } | ||
|
|
There was a problem hiding this comment.
Fake객체는 실제 코드에서는 사용되지않는코드입니다,
테스트 패키지로 관리하면 어떨까요?
There was a problem hiding this comment.
Fake 관련 클래스를 어디에 구현해야 하는지 고민이었는데 답을 얻은 것 같습니다!
감사합니다 🙂
build.gradle.kts
Outdated
| @@ -14,8 +14,7 @@ dependencies { | |||
| testImplementation("org.junit.jupiter", "junit-jupiter", "5.8.2") | |||
| testImplementation("org.assertj", "assertj-core", "3.22.0") | |||
| testImplementation("io.kotest", "kotest-runner-junit5", "5.2.3") | |||
|
|
|||
| testImplementation(kotlin("test")) | |||
| testImplementation("org.mockito:mockito-inline:5.1.1") | |||
There was a problem hiding this comment.
실제 패키지 내에서 테스트를 위한 코드는 지양하는것이 좋습니다.
저는 Fake객체는 test 패키지 범위에서 관리되기 때문에 실제 구현이라고 보지는 않기도 합니다!
실제 프로젝트에는 객체 생성이 어렵거나, Fake가 힘든 케이스도 있기때문에, Mock 라이브러리를 많이 사용하긴 합니다.
하지만, 실제 간단한 경우에는 Fake객체 생성을 하는 경우도 많습니다 👍
|
1주일간 정말 많이 배울 수 있었습니다! |
안녕하세요!
1단계에서 주신 피드백을 기반으로 2단계 미션을 진행하였습니다.
우선, 데이터를 1회성으로 유지하기 때문에 Repository의 필요성이 없다는 피드백을 받고 Service에서 Cars를 관리하는 방법을 택했습니다!
또한, primitive type이지만 검증이 필요한 값들에 대해서는 클래스로 한 번 감싸는 형식으로 구현해보았습니다.
+ MVC에 대해 조금 더 공부해보는 시간을 가져보았습니다.
이번 미션을 진행하면서 궁금했던 점은 객체를 주입했을 때, 어떠한 장점이 있는지 아직은 잘 모르겠습니다..!
글을 찾아보아도 의존성이 줄어든다고 하지만 그러한 개념에 대해서 잘 이해가 되지 않네요.. 🥲
만약 Service에서 RandomGenerator 인터페이스가 아니라 MovementProbabilityGenerator를 직접 의존하면 MovementProbabilityGenerator가 다른 클래스로 바뀌었을 때 OCP에 위배되어서가 맞을까요..?!
추가로 View와 Controller에서는 DTO를, 실질적으로 비즈니스 로직을 수행하는 Service에서는 domain 모델을 의존하도록 하였습니다.
라는 이유에서 이렇게 구현하였는데 좋은 방법인지에 대한 의문이 생겼습니다..!
혹시 이 부분에 대해 잭슨님의 의견을 들을 수 있을까요?!