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

[Step4] 4단계 - 자동차 경주(우승자) #1740

Open
wants to merge 9 commits into
base: devtaebong
Choose a base branch
from

Conversation

devtaebong
Copy link

안녕하세요! 리뷰 잘 부탁 드립니다 🙇‍♂️
지난 피드백에서 불변을 유지하면 어떤점이 좋은지 찾아보면 좋을것 같다는 피드백을 주셨는데요. 관련해서 리서치해 본 내용은 다음과 같습니다.

  1. 프로퍼티가 불변이면 복제본을 통해 값을 변경하게 됨 -> 원본값이 보장됨
  2. 복제본이 변경되는 지점이 명확해짐
  3. 특정 함수 내에서 복제본을 생성하고 변경하기 때문에 부수효과가 없는 함수(side effect free)를 작성할 수 있음
  4. 안정성 있는 시스템을 설계할 수 있음

코틀린이 모든 프로퍼티나 객체를 불변으로 만드는 것을 권장하는 것도 위와 같은 맥락에서 이해했습니다!

@BeokBeok
Copy link

지난 피드백에서 불변을 유지하면 어떤점이 좋은지 찾아보면 좋을것 같다는 피드백을 주셨는데요. 관련해서 리서치해 본 내용은 다음과 같습니다.

  1. 프로퍼티가 불변이면 복제본을 통해 값을 변경하게 됨 -> 원본값이 보장됨
  2. 복제본이 변경되는 지점이 명확해짐
  3. 특정 함수 내에서 복제본을 생성하고 변경하기 때문에 부수효과가 없는 함수(side effect free)를 작성할 수 있음
  4. 안정성 있는 시스템을 설계할 수 있음

코틀린이 모든 프로퍼티나 객체를 불변으로 만드는 것을 권장하는 것도 위와 같은 맥락에서 이해했습니다!

@devtaebong
와우 너무 정리를 잘 해주셨는데요!? 👍 💯

Copy link

@BeokBeok BeokBeok left a comment

Choose a reason for hiding this comment

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

이전 단계 피드백과 자동차 경주(우승자) 미션하시느라 고생하셨습니다. 👍
고민해볼만한 의견들을 코맨트로 작성하였으니, 충분히 고민해보시고 도전해보세요. 💪

Comment on lines +31 to +34
fun findWinners(racingCars: List<Car>): List<Car> {
val maxPosition = racingCars.maxOfOrNull { it.position } ?: 0
return racingCars.filter { it.position == maxPosition }
}

Choose a reason for hiding this comment

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

우승자를 구하는 로직을 Util로 봐야할까요? 🤔
다른 개발자가 봤을 때 만약 우승자에 대한 로직이 추가된다면 어디에 로직을 추가할 것이라 예상하시나요?

Comment on lines +15 to +20
fun String?.convertToListOrThrow(): List<String> =
this?.takeIf { it.matches(Regex(REGEX_VALID_CHARACTERS)) }
?.split(",")
?.map { it.trim() }
?.takeIf { it.isNotEmpty() && it.all { name -> name.length <= MAXIMUM_CAR_NAME_LENGTH } }
?: throw IllegalArgumentException()

Choose a reason for hiding this comment

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

함수명만 봐서는 단순히 List로 반환하는 것으로 보이지만, 내부 코드를 살펴보니 Car와 관련이 있네요. 🤔
관련이 있는 클래스에서 처리될 수 있도록 개선해보면 어떨까요?

Comment on lines +26 to +31
private fun createCarOrThrow(
racingCarNames: List<String>,
numberOfCars: Int,
): List<Car> {
validateCarInput(racingCarNames, numberOfCars)
return (1..numberOfCars).map { id -> Car(id = id, racingCarNames[id - 1]) }

Choose a reason for hiding this comment

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

Car의 List를 만드는 함수를 GameManager에서 구현을 해주셨네요.
태현님은 GameManager의 책임은 어디까지라고 생각하시나요? 🤔
하나의 클래스에서는 하나의 책임만 가지도록 개선해보면 어떨까요?

Comment on lines +22 to +29
fun validateCarInput(
carNameList: List<String>,
numberOfCars: Int,
) {
require(numberOfCars == carNameList.size) {
"자동차 이름과 생성할 대수가 일치하지 않음"
}
}

Choose a reason for hiding this comment

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

관련이 있는 클래스에서 처리될 수 있도록 개선해보면 어떨까요?

Comment on lines +12 to +13
// compile error
// car.position = 10

Choose a reason for hiding this comment

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

학습 테스트를 잘 작성해주셨네요. 👍
학습 테스트도 통과되도록 개선해보면 어떨까요?


fun printWinners(winnersCar: List<Car>) {
val winnerNames = winnersCar.joinToString(", ") { it.name }
println("${winnerNames}이 최종 우승했습니다.")

Choose a reason for hiding this comment

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

Input과 관련된 문구는 enum으로 관리하시는데, Output과 관련된 문구는 관리하지 않는 이유는 무엇일까요?

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