Conversation
- 구분자, 기호 등 상수 처리 - 컨트롤러 서비스로 이동 - buildString dsl 활용
catsbi
left a comment
catsbi
left a comment
There was a problem hiding this comment.
안녕하세요 민균님. 포기하지 않고 꾸준히 해주셔서 감사합니다 👍 💯
5단계의 핵심 키워드는 객체간의 의존성을 분리하고, 핵심 로직들이 테스트하기 쉽게 만들어지는 것인데요. 현재 비즈니스 로직이 있는 부분들에 UI 로직들이 있다보니 테스트하기가 힘들어보이는데, 이를 분리해보면 좋을 것 같습니다!
리뷰 확인 후 다시 리뷰요청 부탁드려요!
| return racingService.play(join()) | ||
| } | ||
|
|
||
| private fun join(): Cars = Cars.of(inputCarName().split(RACING_CAR_DELIMITER)) |
There was a problem hiding this comment.
자동차 이름을 입력받은 것을 컨트롤러 계층에서 분리하고 있네요. 즉 비즈니스 로직이 컨트롤러에서 수행되는 것인데, 이 경우 자동차 이름을 정상적으로 분리를 하는지에 대한 테스트를 하기 힘들 것 같아요. 🤔
| fun play(racingCars: Cars): Cars { | ||
| repeat(inputTryCount()) { | ||
| racingCars.move() | ||
| OutputView.printRacing(racingCars) |
There was a problem hiding this comment.
지금 구조를 보면 의존성의 흐름이 Controller -> Service -> View Layer입니다.
보통 의존성은 밖에서 안으로 단방향으로 흐르는 편이에요. 즉 서비스 계층에서 출력 계층으로 의존성이 흐르는건 청신호는 아니죠. 서비스 계층이라면 테스트 객체인데, 저희는 콘솔창에 어떻게 출력되는지를 테스트할건 아니니까요(요구사항중 UI는 테스트하지 않는다는 내용도 있었죠. )
이런 출력 계층에 대한 정보는 컨트롤러 이상이 알 필요가 없도록 고쳐보면 좋을 것 같아요
There was a problem hiding this comment.
controller 와 service 간에 레이싱 스냅샷 데이터를 출력할 수 있는 DTO 를 추가했습니다
| private const val RACING_COURSE = "-" | ||
| fun printRacing(racingCars: Cars) { | ||
| val result = buildString { | ||
| racingCars.cars.forEach { |
There was a problem hiding this comment.
a.b.c() 현재 a라는 객체에서 c라는 함수호출에 대해 알 필요가 있을까요? 디미터 법칙에 위배됩니다.
또한 OOP관점으로 볼 때 우리는 객체간에 요청과 응답을 하면서 서로의 내부정보를 알 필요 없도록하도록 캡슐화를 제공하는데, 이렇게 내부 값을 꺼내어 연산을 수행하면 캡슐화가 깨지게 되겠죠!
racingCar.contentForEach { ... } 이런식으로 함수를 만들어서 제공해도 좋을 것 같아요.
|
혹시 피드백 주신 부분 개선하려다보니 일부 수정이 많이 된 것 같습니다. 아쉬운 부분이나 추가 개선 주실 부분 있으시면 말씀부탁드립니다. |
catsbi
left a comment
catsbi
left a comment
There was a problem hiding this comment.
민균님, 피드백 반영하느라 고생하셨습니다. 다만, 현재 새로 작성된 CarsTest는 테스트가 통과되지 않고 있으며, 전체적으로 만든 객체들(UI제외)에 대한 테스트가 너무 부족한 것 같습니다. TDD 미션인만큼 테스트 코드는 좀 더 작성해보면 어떨까요?
그리고 ktlint도 검사한 뒤 제출해주시면 좋을 것 같아요 현재 ktlint check에서 실패를 하고 있어요.
자동차 경주 유종의 미를 거둘 수 있도록 다음 항목들을 해결해봅시다.
- UI 제외 모든 public API에 대한 성공 및 실패 테스트 작성
- ktlintCheck해서 리팩터링
피드백 반영 후 다시 리뷰요청 부탁드려요!
| fun of(names: List<String>): Cars { | ||
| private const val RACING_CAR_DELIMITER = "," | ||
|
|
||
| fun of(inputName: String): Cars { |
There was a problem hiding this comment.
보통 정적 팩토리 함수에서 매개변수를 하나만 받는 경우에는 from이나 valueOf등을 사용하는걸 관례로 하고 있습니다.
참고: https://kyucumber.github.io/posts/book/effective-kotlin/effective-kotlin-item-33
| } | ||
| } | ||
|
|
||
| data class CarsDto(val carsDto: List<CarDto>) { |
There was a problem hiding this comment.
DTO의 의미는 data transfer object입니다. 즉 데이터 전송용 바스켓 같은 목적으로 사용되죠.
그런데 이 dto에는 출력과, 최대 거리 찾기 등 비즈니스 로직들이 있는 것 같아요 🤔
| repeat(inputTryCount()) { | ||
| racingCars.move() | ||
| OutputView.printRacing(racingCars) | ||
| snapshots.add(CarsDto(racingCars.toCarDtoList())) |
There was a problem hiding this comment.
서비스에서 dto를 반환해도 되는가? 다른 서비스에서 같은 로직을 수행하는데 dto가 아닌 domain을 받고 싶다면 어떻게 하지? 이런 고민을 해보면 좋은 경험이 될 것 같아요 ㅎㅎ (정답이 있거나 이게 틀렸다는 피드백이 아닙니다 ㅎㅎ)
| import org.junit.jupiter.params.provider.ValueSource | ||
| import racingcar.domain.Cars | ||
|
|
||
| class CarsTest { |
2 participants
안녕하세요. 마지막 5단계입니다. 일전 피드백을 잘 주셔서 감사합니다.
추가 적용 사항 있으시면 말씀부탁드립니다.
늦은시간에 리뷰요청드리곤 했는데 상세히 잘 봐주셔서 감사합니다.