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

[숫자 야구 게임] 김민지 미션 제출합니다. #232

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

Conversation

mouse0429
Copy link

No description provided.

return false
}
}
throw IllegalArgumentException()
Copy link

Choose a reason for hiding this comment

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

예외 메세지를 적어주시면 좋을 듯 합니다.

Copy link
Author

Choose a reason for hiding this comment

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

별도로 메세지를 던져도 되나? 싶어서 그냥 뒀는데 다른 분들 코드를 보니.. 되는 것 같더라구요ㅠㅠ 이번 2주차부터는 반영하겠습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

fun hasNonNumericChar(input: String): Boolean {
val convertedInput: List<Int> = input.map { s -> s.code }.toList()
for (ascii in convertedInput) {
if (ascii < 49 || ascii > 57) {
Copy link

Choose a reason for hiding this comment

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

매직넘버를 상수화 해주면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

아!! 상수화!! 이번에 문자류 상수화까지만 신경쓰고 숫자 쪽 상수화가 전반적으로 미흡했던 것 같습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

showGameInfo(InfoMsgType.StartGame)

var isProgramEnded = false
while (!isProgramEnded) {
Copy link

Choose a reason for hiding this comment

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

2중 while문 및 if문의 중첩으로 depth가 너무 깊어지고 있으므로
따로 메소드로 분리하는 것이 좋아 보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

평소에 2중 while 까지는 괜찮다고 안일하게 생각했던 것 같습니다. 메소드 분리하여 작성해보도록 하겠습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

@@ -0,0 +1,16 @@
package baseball

fun checkBallsAndStrikes(answer: Array<Char>, input: Array<Char>): Pair<Int, Int> {
Copy link

Choose a reason for hiding this comment

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

함수 단위가 아닌 클래스 단위로 분리하면 더 좋을 듯 합니다.

Copy link
Author

Choose a reason for hiding this comment

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

클래스로 분리할까 함수로 분리할까 고민을 참 많이 했습니다만 이번엔 함수로만 분리를 택하게 되었습니다!
클래스로 하려고 할수록 "닭 잡는데 소 잡는 칼을 쓰는 것이 아닐까?" 라는 의문이 들어 함수만 분리했는데...
혹시 클래스 단위를 권장하신 이유에 대해서 여쭈어 봐도 괜찮을까요?
제가 아직 OOP 쪽을 깊게 알지 못해서 위와 같은 생각을 한 것 같아 여쭈어 봅니다! 좋은 리뷰 감사드립니다!🙇‍♀️

Copy link

Choose a reason for hiding this comment

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

대부분의 프로그래밍 세계에서 현재도 객체 지향이 주류이고 앞으로도 꽤 오랜 시간 주류일 것이라는 의견이 지배적이기 때문에 앞으로도 계속해서 객체 지향에 대한 이해도를 올려야 할 필요성을 느끼고 있고,

우아한 테크코스에서 간단한 미션을 오랜 기간을 주며 과제를 권장하는 이유 중 하나가 OOP에 대한 학습을 동반한 클래스 분리 및 리팩토링 연습을 하면서 실력을 향상시키고, 추후 큰 프로젝트를 진행할 때도 잘 적응할 수 있도록 적응시키는 것이 아닐까 하는 생각이 들어 쉬운 미션일 때부터 미리 연습해두는 것이 좋겠다는 판단이 들었습니다.

Copy link
Author

Choose a reason for hiding this comment

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

아 말씀하신 부분에 공감되네요! 작은 것부터 연습해보는 습관이 있어야 정말 필요할 때 사용할 수 있으리라는 생각이 드네요! 우테코의 목적성에 대해서도 다시 한 번 생각해보게 됐습니다! 친절한 답변에 다시 한 번 감사드립니다! 1주차 미션 고생많으셨습니다!:)

Copy link

@stopkite stopkite left a comment

Choose a reason for hiding this comment

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

민지님! 1주차 과제하시느라 고생많으셨습니다 :)
저도 부족하지만 코멘트 남겨보았습니다!
앞으로 남은 과제들도 같이 파이팅해보아요 ㅎㅎ

Comment on lines 12 to 13
InfoMsgType.StartGame -> println(msgType.msg)
InfoMsgType.RequestNumberInput -> print(msgType.msg)
Copy link

@stopkite stopkite Oct 25, 2023

Choose a reason for hiding this comment

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

InfoMsgType를 ShowGameInfo 클래스에 import하시면 코드를 좀 더 간결하게 줄일 수 있을 것 같아요 :) 👍🏻

Copy link
Author

Choose a reason for hiding this comment

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

아 맞는 것 같습니다! InfoMsgType 을 enum 으로 관리하자 정도만 생각했는데 말씀해주신 방법으로 가면 좀 더 간결해질 것 같다는 생각이 듭니다! 좋은 리뷰 감사드립니다!🙇‍♀️

showGameInfo(InfoMsgType.RequestNumberInput)
val numberInput : String = Console.readLine()
val numberInput: String = Console.readLine()
if (!isValidNumberInput(numberInput)) {
Copy link

@stopkite stopkite Oct 25, 2023

Choose a reason for hiding this comment

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

isValidNumberInput 함수이름을 직관적으로 잘 지으셨어요 :) !! 👍

Copy link
Author

Choose a reason for hiding this comment

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

좋은 말씀 감사드립니다! 미흡하지만 변수명/함수명에 대해 많이 고민했는데.. 다른 분께서 함수명에 대해 직관적이라고 표현해주시니 좋은 네이밍의 중요성을 다시금 깨닫게 되는 것 같습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

Comment on lines +3 to +18
fun checkIfUserWin(strikeNum: Int, ballNum: Int): Boolean {
if (ballNum == 0 && strikeNum == 0) {
println("낫싱")
} else if (ballNum == 0) {
println("${strikeNum}스트라이크")
if (strikeNum == 3) {
showGameInfo(InfoMsgType.EndGame)
return true
}
} else if (strikeNum == 0) {
println("${ballNum}볼")
} else if (strikeNum != 0 && ballNum != 0) {
println("${ballNum}볼 ${strikeNum}스트라이크")
}
return false
}
Copy link

@stopkite stopkite Oct 25, 2023

Choose a reason for hiding this comment

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

다음에는 이 함수를 when절로 간결하게 표현해보면 어떨까요? :) !!
저는 이런식으로 작성해봤어요

val result = when {
    strikeCount == 0 && ballCount == 0 -> "낫싱"
    ballCount == 0 -> "${strikeCount}스트라이크"
    strikeCount == 0 -> "${ballCount}볼"
    else -> "${ballCount}볼 ${strikeCount}스트라이크"
}

println(result)

Copy link
Author

Choose a reason for hiding this comment

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

저도 다른 분들께서 코드를 짜신 걸 봤는데 올려주신 코드와 유사하게.. 엄청 간결하게 짜두셨더라구요
확실히 얼렁뚱땅으로 문법을 배워서 그런가 when 절에 대한 이해도가 너무 낮지 않았나라는 생각이 들었습니다.
친절하게 코드 예제까지 올려주셔서 감사합니다! 큰 도움이 됐습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

fun checkBallsAndStrikes(answer: Array<Char>, input: Array<Char>): Pair<Int, Int> {
var strikeNum = 0
var ballNum = 0
for (i in 0..2) {

Choose a reason for hiding this comment

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

2이 어떤 의미의 숫자인지 이해 할 수 있도록 별도의 상수로 만들어 사용하면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

이번 1주차는 숫자의 상수화에 큰 아쉬움이 남는 것 같습니다! 정답지 만드는 함수만 생각하고 있었는데 꼼꼼하게 체크해주신 덕에 for 문도 돌아볼 수 있었습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

if (ballNum == 0 && strikeNum == 0) {
println("낫싱")
} else if (ballNum == 0) {
println("${strikeNum}스트라이크")

Choose a reason for hiding this comment

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

해당 함수에서 점수에 따른 결과 출력과 게임 종료 여부를 수행하고 있는데 각각의 함수로 나누는 편이 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

가장 마음에 들지 않는 부분 + 함수가 하나의 역할만 하지 못하고 있어서 리팩토링이 필요하다고 느꼈던 부분이 바로 이 함순데 너무 정확하게 찔러주셔서 놀랐습니다! 함수 분리에 좀 더 신경쓰도록 하겠습니다 좋은 리뷰 감사드립니다!🙇‍♀️

}

fun hasDuplicatedNumbers(input: String): Boolean {
if (input.toList().distinct().size != 3) {

Choose a reason for hiding this comment

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

중복 검사를 깔끔하게 잘 작성하신 것 같습니다.
👍

Copy link
Author

Choose a reason for hiding this comment

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

여러 방법 중에.. 가장 간결하다고 생각하는 방법을 택했는데 딱 알아봐주셔서 너무 기뻤습니다! 좋은 좋은 리뷰 감사드립니다!🙇‍♀️

Copy link

@kimhm0728 kimhm0728 left a comment

Choose a reason for hiding this comment

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

1주차 고생 많으셨습니다! 2주차 미션도 같이 파이팅해봐요 👍

for (j in 0..2) {
if (answer[i] == input[j] && i == j) {
strikeNum += 1
} else if (answer[i] == input[j] && i != j) {

Choose a reason for hiding this comment

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

for문 안에 변수를 i, j로 작성하셨는데, 변수명에 의미를 포함시키는게 어떨까요? answerIdx, inputIdx 처럼 변수명이 조금 길어지더라도 의미를 한번에 파악할 수 있는 코드가 더 좋은 코드이지 않을까 싶네요. 저도 for문 변수를 습관적으로 i, j, k 등으로 적었었거든요..ㅎㅎ

+ 변수를 1씩 증가시킬 때는 += 1 을 사용하기 보다 증감 연산자를 사용하면 좋을 것 같아요 🙂

Copy link
Author

Choose a reason for hiding this comment

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

다시보니 확실히 파이썬으로 코테를 준비한... 잔재가 느껴지는 코드인 것 같습니다..!! 다음부터는 for 문 속 변수명과 증감연산자에도 신경을 써보도록 하겠습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

fun createGameAnswer(): Array<Char> {
val gameAnswer = mutableListOf<Char>()
while (gameAnswer.size < 3) {
val randomNum = Character.forDigit(Randoms.pickNumberInRange(1, 9), 10)

Choose a reason for hiding this comment

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

하나의 숫자를 뽑을 때마다 forDigit() 함수가 호출되네요. gameAnswer를 Int형 배열로 선언하고, 3개의 중복되지 않은 숫자를 다 뽑은 후에 배열 타입을 Char로 변경하면 불필요한 함수 호출을 줄일 수 있을 것 같아요 😄

Copy link
Author

Choose a reason for hiding this comment

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

아무래도 코틀린 문법을 벼락치기로 배운게 이런 부분에서 많이 드러나는 것 같습니다. 제가 숨기려고 했던 부분을 너무 날카롭게 캐치해주신 것 같아요!! 처음에 말씀해주신 방식으로 진행하다 마지막에 형변환 하는거 잊어먹고 왜 안되지 다르게 풀자.. 한 결과물입니다 사실ㅎㅎ..
char Array 로 선언하는게 좋은 선택이었는지도 궁금한데 혹시 이부분에 대해선 어떻게 생각하시는지 여쭤봐도 될까요? 좋은 리뷰 감사드립니다!🙇‍♀️

Choose a reason for hiding this comment

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

그건 개발하는 사람의 스타일에 따라 다르다고 생각합니다. ㅎㅎ 그런데 이번에 Random 값을 뽑는 예제 코드 자체에서 List<Int>로 선언되어 있는 걸로 기억을 해요. 그리고 민지님이 작성해주신 checkBallsAndStrikes 코드는 충분히 List<Int> 타입으로도 잘 돌아갈 것 같은데, 왜 Array<Char>로 따로 형변환을 하셨지?! 라는 의문이 들긴 하네요 😃

+ 혹시 1주차 공통 피드백 보셨나요?! 배열 대신 kotlin에서 제공하는 컬렉션을 사용하라고 적혀있더라구요. 컬렉션의 다양한 api를 사용할 수 있다는 측면에서는 저도 컬렉션을 선호하긴 하는데, 이번 미션처럼 조건이 간단한 상황에서는 Array를 사용해도 되지 않을까 싶어요!!

아직 많이 배워야할 입장이어서... 제 개인적인 의견입니다. ㅎㅎ

if (!isValidNumberInput(numberInput)) {
throw IllegalArgumentException()
}
return numberInput.toCharArray().toTypedArray()

Choose a reason for hiding this comment

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

toCharArray().toTypedArray()를 두번 이상 호출하신 것 같은데, 이 부분도 함수로 분리하면 좋을 것 같아요. 재사용성도 높아지고, 더 보기 좋은 코드가 되지 않을까요? 일반적으로 하나의 타입을 다른 타입으로 변경하는 함수는 확장 함수로 생성하더라구요. 참고해보세요 😄

Copy link
Author

Choose a reason for hiding this comment

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

오 맞는 것 같습니다! 타입 맞추는 부분에서 사용한거라 반복해서 사용한다는 점을 인지하지 못한 것 같아요! 너무 좋은 의견 감사드립니다! 확장 함수라는 키워드로 접근해보겠습니다! 좋은 리뷰 감사드립니다!🙇‍♀️

} else if (hasDuplicatedNumbers(numberInput)) { // 예외4. 중복된 숫자 입력
return false
}
return true

Choose a reason for hiding this comment

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

이 부분 else를 다 빼셔도 되지 않을까요? 조건에 해당할 때마다 return을 호출하고 있으니, 조건에 맞지 않는 경우가 else문까지 도달하는 일은 없으니깐요 😁

package baseball

enum class InfoMsgType(val msg: String) {
StartGame("숫자 야구 게임을 시작합니다."),

Choose a reason for hiding this comment

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

enum class는 고려하지 못한 부분인데, 비슷한 특성의 변수들을 enum class로 묶을 수 있겠네요. 하나 배워갑니다 👍

Copy link

@yunseoChang yunseoChang left a comment

Choose a reason for hiding this comment

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

1주차 너무 고생많으셨습니다👍👍 다음과제도 같이 화이팅해봐요!!😁😁

Comment on lines +8 to +10
if (strikeNum == 3) {
showGameInfo(InfoMsgType.EndGame)
return true

Choose a reason for hiding this comment

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

이중으로 쓰지 않고 checkIfUserWin 함수의 가장 위에서 수행하면 depth를 줄일 수 있을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

앗 좋은 리뷰 감사드립니다! 1주차는 전반적으로 불필요하게 depth 가 깊어진 부분이 많아 아쉬움이 남는 것 같습니다 ㅠㅠ

Comment on lines +8 to +10
}

fun showGameInfo(msgType: InfoMsgType) {

Choose a reason for hiding this comment

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

enum class에 대한 함수를 다음과 같이 공통 함수로 만들어서 처리하는 방법도 있을 것 같아요

enum class InfoMsgType(val msg: String) {
    StartGame("숫자 야구 게임을 시작합니다."),
    RequestNumberInput("숫자를 입력해주세요 : "),
    EndGame("3개의 숫자를 모두 맞히셨습니다! 게임 종료"),
    RequestRestartInput("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.");

    fun showGameInfo() {
        println(this.msg)
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

이번 2주차때 말씀해주신 공통함수로 처리해보았습니다! 좋은 리뷰 감사드립니다!
+) 혹시 리뷰 반사 원하시면 해당 코멘트 아래에 주소 남겨주세요 감사합니다

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