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

[숫자 야구 게임] 임형준 미션 제출합니다. #51

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

Conversation

lh99j
Copy link

@lh99j lh99j commented Oct 24, 2023

No description provided.

}
}

return Pair(strike, ball)
Copy link

Choose a reason for hiding this comment

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

Pair 대신 int 하나로도 strike와 ball의 값을 전달할 수 있을 것 같아요.
strike와 ball의 경우의 수가 0~3이니 4진법 이상의 진법으로는 각 자리별로 구분하여 나타낼 수 있습니다.
간단한 예로, 10진법으로 나타내면 12 -> 1스트라이크 2볼 등.

Choose a reason for hiding this comment

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

가독성 측면에서는 Pair이나 클래스로 래핑하는게 더 낫지 않을까요?

Choose a reason for hiding this comment

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

저도 Pair에 한표 더합니다.

Choose a reason for hiding this comment

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

저도 Pair가 가독성이 좋을 것 같습니다, 관리 자체도 편하구요

Copy link

@sseung416 sseung416 left a comment

Choose a reason for hiding this comment

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

작지만 리뷰 남겼습니다~ 수고하셨어요!

private const val END_NUMBER = 9
}

fun setComputerNumber(): MutableList<Int> {

Choose a reason for hiding this comment

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

Immutable한 List로 반환하는 것을 추천드립니다! 해당 난수를 처음 생성 후에는 외부에서 변경할 필요는 없으니까요.
이펙티브 코틀린의 가변성 제한 부분에서도 나오는 내용인데, 한 번 참고하시면 좋을 것 같아요!

Comment on lines +43 to +48
val resultHint = when {
strike == 0 && ball == 0 -> NOTHING
strike > 0 && ball == 0 -> "$strike$STRIKE"
strike == 0 && ball > 0 -> "$ball$BALL"
else -> "$ball$BALL $strike$STRIKE"
}
Copy link

@sseung416 sseung416 Oct 27, 2023

Choose a reason for hiding this comment

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

네이밍에도 신경 써 주시면 좋을 것 같아요! "$strike$STRIKE"를 봤을 때, 무엇을 출력하는지 한 번에 알아보기 힘든 것 같아요.
strikeCount라든지, STRIKE_MESSAGE 등과 같이 좀 더 구체적으로 작성해주시면 더 깔끔한 코드를 작성할 수 있을 것 같아요!

Choose a reason for hiding this comment

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

네이밍할 때 출력문 같은 경우는 뒤에 MSG를 붙이거나 STRING을 붙여 텍스트임을 알려주는게 좋을 것 같습니다!

if (isGameEnd) {
printGameEndMessage()
val restartGame = user.decideGame()
if (restartGame) {

Choose a reason for hiding this comment

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

우테코 깃헙에 있는 클린코드 체크리스트에 [한 메서드에 오직 한 단계의 들여쓰기(indent)만 허용했는가?] 라는 항목이 있습니다. gameStart() 메서드에 들여쓰기가 몇 개 존재하는지 생각해보고 이를 다른 메서드로 분리해보는 것도 좋을것 같습니다.

val computer = mutableListOf<Int>()

while (computer.size < SIZE) {
val randomNumber = Randoms.pickNumberInRange(START_NUMBER, END_NUMBER)

Choose a reason for hiding this comment

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

START_NUMBER, END_NUMBER 로 상수로 만드신 점을 칭찬합니다.

}
}

return Pair(strike, ball)

Choose a reason for hiding this comment

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

저도 Pair에 한표 더합니다.

Comment on lines +26 to +27
val hint = computer.countStrikeAndBall(computerNumber, userNumber)
isGameEnd = computer.checkGameEnd(hint.first)

Choose a reason for hiding this comment

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

hint.first가 어떤 의미인지 해당 부분을 보고서 판단하는게 어렵습니다.
간단하게 아래처럼 strike로 명시하셔도 되고 별도의 객체로 관리하시는 것이 좋을 것 같습니다.
isGameEnd = computer.checkGameEnd(strike=hint.first)

}
}

return Pair(strike, ball)

Choose a reason for hiding this comment

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

저도 Pair가 가독성이 좋을 것 같습니다, 관리 자체도 편하구요

Comment on lines +43 to +48
val resultHint = when {
strike == 0 && ball == 0 -> NOTHING
strike > 0 && ball == 0 -> "$strike$STRIKE"
strike == 0 && ball > 0 -> "$ball$BALL"
else -> "$ball$BALL $strike$STRIKE"
}

Choose a reason for hiding this comment

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

네이밍할 때 출력문 같은 경우는 뒤에 MSG를 붙이거나 STRING을 붙여 텍스트임을 알려주는게 좋을 것 같습니다!

}

private fun checkValidRestartNumber(input: String) {
if (input == RESTART || input == QUIT) {

Choose a reason for hiding this comment

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

RESTART와 QUIT으로 분리한 거 칭찬해요~
저도 바꿔야겠어요!

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.

6 participants