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

[Sheep1sik] 자동차 경주 게임 #3

Open
wants to merge 16 commits into
base: Sheep1sik
Choose a base branch
from

Conversation

Sheep1sik
Copy link

No description provided.

Copy link
Owner

@croffle-lover croffle-lover left a comment

Choose a reason for hiding this comment

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

안녕하세요 원식님! 직장 생활을 하시면서도 미션을 수행하시다니 고생하셨습니다!

다만 이 미션의 경우 TDD를 사용하는 것도 커리큘럼의 일환이니 다음부터는 TDD로 꼭 참여하셨음 좋겠습니다!

전체적으로 코틀린에 대한 이해도가 미션이 필요 없으실 정도로 잘 아시는 것 같습니다!

몇 가지 코멘트 남겨드렸으니 반영 또는 질문에 대한 답을 해주시고, 테스트도 구현해주세요!
리뷰요청 기다리겠습니다. 😀😀

Comment on lines +3 to +5
class InputView {
fun getUserInput(): String = readln()
}
Copy link
Owner

Choose a reason for hiding this comment

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

InputView 객체의 인스턴스가 여러개 있을 필요가 있을지 고민해보시고 수정해도 좋을 것 같습니다!

Comment on lines +46 to +54
private enum class Message(private val message: String) {
RaceCarNames("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"),
NumberOfRound("시도할 횟수는 몇 회인가요?"),
RaceResult("실행 결과"),
WinnerTemplate("최종 우승자 : %s"),
RaceResultTemplate("%s : %s");

override fun toString() = message
}
Copy link
Owner

Choose a reason for hiding this comment

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

출력문에 사용 될 메시지들을 enum으로 관리해주셨군요.
좋은 아이디어입니다!

하지만 위의 companion object에 메시지들을 상수로 지정해줘도 되는데, Enum으로 따로 관리하시는 이유는 무엇인가요?
틀렸다고 하는게 아닌 의견이 궁금합니다.

추가로 enum안에 정의되는 상수들도 UPPER_CASE로 작성해야합니다!

Comment on lines +14 to +25
fun printCurrentRaceResult(gameBoard: GameBoard, round: Round) {
val message = buildString {
appendLine()
appendLine(Message.RaceResult)
round.forEach { currentRound ->
val carNameAndScoreList = gameBoard.getScoresByRound(currentRound)
appendLine(formatRaceResults(carNameAndScoreList))
appendLine()
}
}
print(message)
}
Copy link
Owner

Choose a reason for hiding this comment

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

val carNameAndScoreList = gameBoard.getScoresByRound(currentRound)

저 처럼 원식님의 코드를 처음 보는 사람 입장에서는 이 부분만 보고 자동차의 이름과 점수를 가진 리스트를 가져온다고 이해는 할 수 있지만, 정확하게 List 타입으로 오는 것인지 아니라면 다른 객체로 오는 것인지를 인지하기 힘듭니다.

특별한 이유가 아니라면 타입을 예측하기 힘들 경우 명시해주시는건 어떨까요?

또한 변수명에 자료구조의 이름을 그대로 명시해주는 것도 그다지 권장되지 않습니다.
사용하던 자료구조를 바꾸게 되면 해당 변수명도 같이 바꿔줘야하는 불상사가 생길 수도 있기 때문입니다.
만약 목록이라는 뜻으로 list를 사용하신 거라면 그것 또한 만일 다른 자료구조를 사용했다면 다른 개발자가 오해를 할 수 있습니다.

참고: Effective Kotlin - item 4: inffered type으로 리턴하지말라 , item 14: 변수 타입이 명확하지 않은 경우 확실하게 지정하라

Comment on lines +23 to +25
internal enum class Error(val message: String) {
OverflowRound("입력한 차수를 넘겼습니다.")
}
Copy link
Owner

Choose a reason for hiding this comment

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

internal 명시의 이유는 무엇인가요?

Comment on lines +8 to +11
@JvmInline
value class CarName(private val value: String) {
override fun toString(): String = value
}
Copy link
Owner

Choose a reason for hiding this comment

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

value class에서 @JvmInline을 명시해주는 이유가 무엇일까요?

Comment on lines +3 to +7
class Car(val name: CarName) {
private var _distance = 0
val distance: Int get() = _distance
fun moveForward(): Int = ++_distance
}
Copy link
Owner

Choose a reason for hiding this comment

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

distance를 외부에서 값 변경을 하지 못하게 하려고 이런식으로 백킹 프로퍼티를 사용하신거 같은데 사실 이렇게 안해줘도 됩니다!

그에 대한 답은 여기에 있습니다.
Kotlin-Properties

Comment on lines +3 to +16
@JvmInline
value class Round(private val value: Int) {
fun forEach(action: (Round) -> Unit) = toListOfRounds().forEach(action)
fun toListOfRounds(): List<Round> = (START_ROUND..value).map { Round(it) }

companion object {
private const val START_ROUND = 1

fun from(roundString: String): Round {
val round = roundString.toInt()
return Round(round)
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

value class를 아시는 거로 보아 상당히 코틀린을 잘아시는 것 같습니다!

다만 forEach라는 함수는 collection의 forEach라고 다른 개발자 입장에서 오해할 수도 있는 이름이라고 생각됩니다.
심지어 함수 내부에서는 collection의 forEach를 사용합니다.

Comment on lines +3 to +5
object RandomGenerator {
fun getRandomNumber(startNumber: Int, endNumber: Int):Int = Random.nextInt(startNumber, endNumber)
}
Copy link
Owner

Choose a reason for hiding this comment

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

테스트하기 어려워 보이는데 테스트에 유리함을 가지려면 어떤 방법이 있을까요?

@@ -1 +1,39 @@
# 자동차 경주 게임
Copy link
Owner

Choose a reason for hiding this comment

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

기능 목록을 상세히 잘 적용해주셨군요 좋습니다!

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