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

🚀 3단계 - 로또(2등) #1141

Open
wants to merge 12 commits into
base: sjjeong
Choose a base branch
from
Open

🚀 3단계 - 로또(2등) #1141

wants to merge 12 commits into from

Conversation

sjjeong
Copy link

@sjjeong sjjeong commented Jan 8, 2025

리뷰어님 안녕하세요

로또 미션 3단계 진행 했습니다.

일급 컬렉션을 자주 사용하지 않았던 방법이라 피드백 적극 환영입니다~

image

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

안녕하세요 석준님!

3단계 구현을 잘 해주셨습니다 💯 깔끔한 구현이 좋았습니다
일급 컬렉션 관련 개선 제안을 남겼으니 확인해주시면 감사하겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 💪

Comment on lines 6 to 19
object LottoVendingMachine {
private const val LOTTO_PRICE = 1000

fun buyLotto(money: Int?): List<Lotto> {
fun buyLotto(money: Int?): Lottos {
requireNotNull(money) {
"구매 금액은 null이 아니어야 함"
}
require(money >= LOTTO_PRICE) {
"구매 금액은 1000 보다 커야 함"
}

return List(money / LOTTO_PRICE) { Lotto() }
return Lottos(List(money / LOTTO_PRICE) { Lotto() })
}
}
Copy link
Member

Choose a reason for hiding this comment

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

외부에서 Lottos를 직접 생성해주지 말고, Lottos 스스로 생성할 수 있도록 해보면 어떨까요?

이 과정에서 팩토리 함수나 부 생성자를 사용해볼 수 있겠네요.

다음 글이 도움되길 바랍니다!
https://kt.academy/article/ek-factory-functions


4 -> Prize.FOURTH
3 -> Prize.FIFTH
else -> Prize.NONE
Copy link
Member

Choose a reason for hiding this comment

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

3단계 힌트를 참고하여 Prize에 메세지만 보내서 어떤 Prize인지 가져올 수 있도록 개선해보면 어떨까요? 외부에서 Prize를 판단하지 말고 메세지만 보내 보세요!

entries를 활용하시는걸 추천드립니다 🙂
https://engineering.teknasyon.com/kotlin-enums-replace-values-with-entries-bbc91caffb2a

bonusLottoNumber: LottoNumber,
lottos: Lottos,
): LottoResult {
val result = lottos.lottos.groupingBy { lotto ->
Copy link
Member

Choose a reason for hiding this comment

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

일급 컬렉션 구현을 의도하신 것으로 보이는데요,
Lottos 객체에게도 적절한 역할과 책임을 할당할 수 있을 것 같아요 🙂
lottos.lottos.groupingBy와 같이 데이터를 꺼내어(get) 조작하기보다, 메세지를 던져 객체가 일하도록 해보면 어떨까요?
다음 글이 도움되길 바랍니다.
https://jojoldu.tistory.com/412


constructor() : this(LottoPreset.shuffled().take(6).sortedBy { it.number }.toSet())

constructor(vararg number: Int) : this(number.map { LottoNumber(it) }.toSet())
Copy link
Member

Choose a reason for hiding this comment

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

saso1

이러한 구문을 reference로 넘길 수 있습니다.

Suggested change
constructor(vararg number: Int) : this(number.map { LottoNumber(it) }.toSet())
constructor(vararg number: Int) : this(number.map(::LottoNumber).toSet())

lambda로 작성하는 것과 어떤 차이가 있는지 참고할 수 있는 글을 소개해드릴게요 🙂
https://proandroiddev.com/kotlin-lambda-vs-method-reference-fdbd175f6845

참고로 저는 가능한 곳에서는 꼭 함수 reference를 활용하는 것을 지향합니다. lambda로 가져온 데이터를 별도로 조작하지 않고 그대로 함수 시그니처로 넘긴다는 것을 드러낼 수 있기 때문입니다.

constructor(vararg number: Int) : this(number.map { LottoNumber(it) }.toSet())

fun countMatch(winningLotto: Lotto): Int {
return lottoNumbers.intersect(winningLotto.lottoNumbers).count()
Copy link
Member

Choose a reason for hiding this comment

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

saso2

https://kotlinlang.org/docs/coding-conventions.html#wrap-chained-calls
코틀린 공식 문서에 따르면 함수 체인의 경우 아래와 같이 체인별 개행을 해주어야 합니다.
(일반적으로 체인의 첫 번째 호출에는 개행이 있어야 하지만 개행하지 않는 편이 더 합리적이라면 생략해도 괜찮습니다)

Suggested change
return lottoNumbers.intersect(winningLotto.lottoNumbers).count()
return lottoNumbers
.intersect(winningLotto.lottoNumbers)
.count()

Comment on lines +3 to +13
@JvmInline
value class LottoNumber(val number: Int) {
init {
require(number in START_LOTTO_NUMBER..END_LOTTO_NUMBER)
}

companion object {
const val START_LOTTO_NUMBER = 1
const val END_LOTTO_NUMBER = 45
}
}
Copy link
Member

Choose a reason for hiding this comment

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

LottoNumber 객체가 매번 생성될 때마다 1~45까지의 Range 객체가 매번 생성되어야 할까요?

}

companion object {
private val LottoPreset = List(LottoNumber.END_LOTTO_NUMBER) { LottoNumber(it + 1) }
Copy link
Member

Choose a reason for hiding this comment

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

여러 번 인스턴스화하지 않아도 되는 객체에 대해 고려해주셨네요 👏

Copy link
Member

Choose a reason for hiding this comment

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

이번 기회에 ParamterizedTest에 대해 익히고 적용해보면 어떨까요?
물론 어떤 테스트가 실패했는지 직관적으로 알 수 있기 때문에 함수 이름을 나누어 분할하는건 충분히 의미있는 것 같아요.
실패하는 케이스를 모두 하나의 실패 시나리오로 볼 것인지, 각각의 시나리오로 보고 구분할 것인지는 개발자가 스스로 판단하면 됩니다.

@@ -1,26 +1,27 @@
package lotto

import io.kotest.matchers.shouldBe
import lotto.domain.Prize
import org.junit.jupiter.api.Test

class PrizeTest {
Copy link
Member

Choose a reason for hiding this comment

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

Prize에 메세지를 던지는 구조로 변경되고 나면, 새로운 시나리오도 추가될 수 있겠네요 🙂

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