-
Notifications
You must be signed in to change notification settings - Fork 357
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] 로또 (수동) #1123
base: sarahan774
Are you sure you want to change the base?
[step4] 로또 (수동) #1123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요!
step4까지 작성 잘해주셨네요 👍
몇가지 코멘트를 남겼으니 확인부탁드릴게요!
@@ -50,4 +50,15 @@ object LottoController { | |||
) | |||
return result | |||
} | |||
|
|||
private fun createMyLottoList(totalLottoCount: Int): List<Lotto> { | |||
val manualLottoCount = InputView.readManualLottoCount(InputView.ENTER_MANUAL_LOTTO_COUNT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InputView에 InputView의 메시지를 넣어주는 방식으로 변경하신 이유가 있을까요~?
프롬프트 메시지는 View의 영역이라고 생각해서요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
호출부 쪽에서 어떤 메시지를 보여주는 함수인지 볼수 있도록 한것이긴 한데요, InputView 쪽으로 넣어도 좋을거같습니다.
private fun readManualLotto(): List<Int> { | ||
val input = | ||
readlnOrNull()?.trim() | ||
?.split(",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버가 보이네요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 감사합니다.
val input = | ||
readlnOrNull()?.trim() | ||
?.split(",") | ||
?.map { it.trim().toIntOrNull() ?: 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrNull 이후에 throw가 아닌 0으로 결정하신 이유가 있을까요??
View에서 숫자가 아닌경우의 validation은 해줘도 되지 않을까싶어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 과제에서 throw 하지 않는쪽으로 피드백을 받았던 기억이 있어서 기계적으로 반영했는데요,
한번 수정해도 좋을거같네요 !
promptMessage: String, | ||
count: Int, | ||
): List<List<Int>> { | ||
val manualLottoNumberList = mutableListOf<List<Int>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/
네이밍에 대해 고민해보고 List postfix를 붙이는 것에 대한 고민을 해보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컬렉션 타입이 변경되면 변수명도 바뀌어야 하는 상황이 발생할 수 있군요! 좋은 의견 감사합니다.
} | ||
|
||
private fun autoGeneratedLottoList() = | ||
List(autoGeneratedLottoCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoGeneratedLottoCount가 음수가 될 확률은 없을까요!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 그러네요. 감사합니다!
} | ||
} | ||
|
||
private fun autoGeneratedLottoList() = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중괄호 없이 단일 표현식으로 작성을 해주셨는데요!
단일 표현식으로 작성하시는 기준이 있을까요~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저같은 경우는 보통 Boolean 이나 String 타입을 리턴하는 1줄짜리 함수에 많이 사용하는데요,
여기는 List() {} 생성자 호출만 하고 있어서 간하게 단일 표현식으로 썼었나 싶네요.
다만 3줄이 넘어가서 괄호로 가도 좋을거같습니다.
@@ -23,8 +23,8 @@ object LottoController { | |||
// 총 구매 로또 개수 | |||
ResultView.printTotalPurchaseCount(totalLottoCount) | |||
|
|||
val myLottoList = createMyLottoList(totalLottoCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당첨번호라는 의미를 가진 변수인데, myLotto가 괜찮을기 고민 해보면 좋을 것 같아요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!
@@ -1,4 +1,4 @@ | |||
package lotto.domain.model | |||
package lotto.domain | |||
|
|||
@JvmInline | |||
value class Lotto(val value: List<LottoNumber>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금보니까 LottoTest가 없네요!! 테스트를 작성해보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러네요 이상하게 커버리지에는 초록색으로 뜨던 ..;?!
작성하도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가희님 안녕하세요 🙂
어느덧 step4 마무리시점이 왔는데요!
로또 피드백 : https://edu.nextstep.camp/s/l3Ppxbm8/ls/ROS71xiT
내용 중에 아래 두가지 내용을 고민해보고 적용해보면 어떨까 싶어서 연락드렸어요!
-
모든 원시 값과 문자열을 포장한다.
- LOTTO_PRICE가 생각나네요 :thinking_face:
-
일급 컬렉션을 쓴다.
- List도 한번 묶어볼까요?
현재 프로젝트에서 위 내용에 해당 하는 부분을 찾아보시고 도전해보면 어떨까 싶습니다!
적용해보시구 이해 안가는부분은 편하게 질문주세요!! :)
마지막까지 화이팅입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가희님 안녕하세요 :)
수정완료 되시면 다시한번 요청주세요~~!!
피드백 미리 감사드립니다.