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

[코드 리뷰용 PR] - 재구현 스터디 #238

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

sh1mj1
Copy link

@sh1mj1 sh1mj1 commented Nov 30, 2023

고민되는 부분

  • 현재 OutputViewshowTryResult 메서드에서 자동차들의 현재 위치에 대해서 출력하고 있습니다.
    그래서 Cars 를 패러미터로 받아서 그에 패러미터를 가져와서 출력하고 있습니다.
    그런데 현재 이 구현보다, Cars 클래스 내부에서 toString 을 오버라이드하는 방식이 더 나을까요?,
    예를 들면 이런 식입니다.
data class Cars(val cars: List<Car>) {
    // ....
    override fun toString(): String = cars.joinToString("\n") {
        "${it.name} : " + "-".repeat(it.position)
    }
}
    fun showTryResult(cars: Cars) {
        println(cars.toString())
        println()
    }

위처럼 변경한다면 View 에서 Cars 의 프로퍼티에 직접 접근을 하지 않아도 되서 이점이 있습니다.
반면에 출력 형식은 완전히 View 의 역할이라서, View 에서만 출력 로직을 관리하도록 하는 것이 MVC 패턴의 관점에서는 더 나아보이기도 합니다.

  • OutputViewshowTotalWinner라는 메서드에서도 같습니다.

리뷰어분들 이야기를 듣고 싶습니다.

* 0 ~ 9 사이 난수가 4 이상이면 이동할 수 있다.
* 공백 이름을 허용하지 않는다.
* 자동차의 이름은 5자 이하만 가능하다.
* `withNames` 메서드로 차의 이름들을 받아서 생성자 역할을 해주는 팩토리 메서드를 구현한다.
* 이전: 단순히 전진 조건을 판별해서 조건의 참, 거짓만을 리턴한다.
* 이후: 전진 조건을 판별하고 car 의 메서드를 호출하여 자동차를 이동시킨다.
* `readCars`: 자동차를 입력 관련 가이드 메시지 출력과 자동차 입력
* `readTryCount`: 시도 횟수 입력 관련 가이드 메시지 출력과 시도 횟수 입력
* 자동차를 입력할 때 앞 뒤 공백은 제거하는 것으로 구현했다.

private fun moveCars(tryCount: Int, cars: Cars) {
repeat(tryCount) {
cars.cars.forEach { car ->

Choose a reason for hiding this comment

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

페어 프로그래밍 했을 때 논의했던 부분이긴 하지만
저는 이후에 구현을 마무리하면서 data class를 Car만 만들었는데 Cars를 만들 필요가 있을까란 생각이 들더라고요

Car class를 만들면 자동차 객체를 생성할 수 있고, 그걸 리스트의 원소로 넣으면 Cars 객체를 생성할 수 있어서 그러면 자연스럽게 cars.cars 와 같은 표현들도 사용하지 않아도 될것 같더라고요

Cars 클래스를 만들지 않는 것에 대해서 어떻게 생각하시나요?

@jihyeonbaem
Copy link

OutputView의 책임을 어디까지 볼 것인가에 따라 다를 것 같은데, 이것도 마찬가지로 그 때 이야기를 했었지만, 저와 생각이 비슷하셨던 것으로 기억하는데

mvc 패턴에 대해서는 잘 모르지만, 저는 출력형식로직은 view에서 관리를 해도 좋을 것 같다는 생각입니다.

만약 출력형식로직을 view에서 하지 않고 외부에서 한다 하더라도 그것이 Car class 내부에 있는 것은 적합하지 않는 것 같습니다.

}

companion object {
const val INPUT_CARS_GUIDE_MESSAGE = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"

Choose a reason for hiding this comment

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

저 한가지 어제 배웠던 점이 있는데요 const val의 가시성을 private로 제한 하는게 맞는 것 같습니다.

3주차 테스트코드 부분에서 private const val로 상수를 선언하더라고요

그걸보니 아 가시성제한을 하는게 맞겠구나 생각이 들었습니다!


fun readTryCount(): Int {
println(INPUT_TRY_COUNT_GUIDE)
val tryCount = readLine().toIntOrNull() ?: -1

Choose a reason for hiding this comment

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

require를 잘 사용해주셨는데요!

requireNotNull을 활용하면 좀더 명확하게 처리가 가능할 것 같습니다!

저는 원래 inputview에서 try-catch를 했었는데 페어 때 작성하신 코드를 보고 컨트롤러에서 예외를 잡는게 더 좋겠다 생각이 들어서 저도 변경했습니다!

}

companion object {
private const val NAME_MAX_LENGTH = 5

Choose a reason for hiding this comment

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

어 여긴 가시성이 private으로 설정하셨네요? 혹시 그럼 문자열 상수랑 다르게 한 이유가 있으신건가요? 제가 잘못알았던 건가


data class Car(
val name: String,
private var _position: Int = 0

Choose a reason for hiding this comment

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

저는 Car가 위치를 알고 있을 필요가 없다고 생각을 했어요

Car의 책임을 전진하거나 멈추거나로 생각을 했어서

이부분은 자동차의 책임을 어디까지 보는가에 따라서 달라질 것 같긴하네요.

그러나, position을 get()으로 내보내는 것 보다 저는 심판(가명)측에서 알고 바로 처리를하는것이 좋을 것 같다는 생각을 했습니다!

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.

2 participants