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

Step2 로또 자동 #992

Open
wants to merge 9 commits into
base: sendkite
Choose a base branch
from
Open

Step2 로또 자동 #992

wants to merge 9 commits into from

Conversation

sendkite
Copy link

@sendkite sendkite commented Nov 26, 2023

리뷰어님 안녕하세요!

로또 자동 PR 드립니다.
bottom up 방식으로 구현하려고 노력했습니다.
오브젝트책의 영화 티켓 판매기에 영감을 받아 비슷하게 작성해보려 했는데 쉽지 않네요.

작업내역

  • 지난 계산기 PR 리뷰 내용 반영
  • 로또 계산기 자동 구현

@sendkite sendkite changed the title Step2 Step2 로또 자동 Nov 26, 2023
Copy link
Member

@malibinYun malibinYun left a comment

Choose a reason for hiding this comment

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

2단계 미션 고생 많으셨어요!
생각해보면 좋을 점들에 코멘트 남겨두었어요.
피드백 반영 후 다시 리뷰 요청 부탁드려요!

Comment on lines +3 to +8
fun isNumber(number: String): Boolean {
return Regex("[0-9]+").matches(number)
}

fun findCustomDelimiter(text: String): String {
return Regex("//(.)\n(.*)").find(text)?.groupValues?.get(1) ?: ""
Copy link
Member

Choose a reason for hiding this comment

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

#981 (comment)
여전히 함수를 부를 때마다 Regex 객체를 생성하고 있어요!
Regex를 상수로 분리해보는 건 어떨까요?

또, 해당 메서드들은 이 역할을 가져야할 객체에게 위치시켜보는 건 어떨까요?

Comment on lines +3 to +4
class Lotto(
val numbers: List<Int> = generateLottoNumbers(),
Copy link
Member

Choose a reason for hiding this comment

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

Int에는 1~45 라는 로또 번호를 제한하기에는 너무 범용적인 자료형이라 생각해요.
로또 번호를 나타내는 도메인 객체를 만들어보는 건 어떨까요?

Comment on lines +12 to +14
fun match(target: Lotto) {
matchCount = numbers.intersect(target.numbers).count()
}
Copy link
Member

Choose a reason for hiding this comment

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

target 과 비교한 후, 이 로또는 matchCount를 그대로 갖고있을 것 같아요.
이후 이 값은 어떤 로또와 비교했었는지 추적하기 어려워질 것으로 우려돼요.
matchCount에 사이드이펙트를 발생시키는 것 보다, match 한 결과를 반환하게 만드는 것은 어떨까요?

Comment on lines +3 to +5
class LottoStore {

fun sell(money: Int): List<Lotto> {
Copy link
Member

Choose a reason for hiding this comment

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

LottoStore입장에서는 sell 하는 것이 맞는데, 이 메서드를 사용하는 사람 입장에서는 굉장히 어색할 것으로 예상돼요.
returnNumber 같은 말 보다는, getNumber를 쓰듯이요!

Comment on lines +7 to +11
val lottos = mutableListOf<Lotto>()
for (i in 1..count) {
lottos.add(Lotto())
}
return lottos
Copy link
Member

Choose a reason for hiding this comment

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

이런 형태는 코틀린의 확장함수를 활용해보셔도 좋겠어요 :)

(1..count).map { }

혹은, List의 fake constructor를 활용해보셔도 좋겟네요 :)

List(count) { ... }

Comment on lines +24 to +28
println("3개 일치 (5000원) - ${lottos.count { it.matchCount == 3 }}개")
println("4개 일치 (50000원) - ${lottos.count { it.matchCount == 4 }}개")
println("5개 일치 (1500000원) - ${lottos.count { it.matchCount == 5 }}개")
println("6개 일치 (2000000000원) - ${lottos.count { it.matchCount == 6 }}개")
println("총 수익률은 ${lottos.size * Lotto.PRICE / inputMoney}입니다.")
Copy link
Member

Choose a reason for hiding this comment

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

로또 등수에 대해 도메인 객체로 표현해보는 건 어떨까요?

Comment on lines +13 to +16
@CsvSource(value = ["1000,1", "2000,2", "3000,3"])
fun `로또를 여라장 구매한다`(money: Int, expected: Int) {
val lotto = LottoStore().sell(money)
lotto should haveSize(expected)
Copy link
Member

Choose a reason for hiding this comment

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

몇 장을 샀는지 검증하는 것은 테스트에 큰 의미를 가지지 못한다고 생각해요.
개발자가 제어 가능한 형태로, 어떤 티켓이 반환되었는지까지 테스트할 수 있게 만들어보는 건 어떨까요?

Comment on lines +44 to +68
@Test
fun `test lotto matching`() {

val targetLottoNumbers = listOf(1, 2, 3, 4, 5, 6)
val targetLotto = Lotto(targetLottoNumbers)

val countAndLotto = listOf(
Pair(1, listOf(1, 7, 8, 9, 10, 11)),
Pair(2, listOf(1, 2, 8, 9, 10, 11)),
Pair(3, listOf(1, 2, 3, 9, 10, 11)),
Pair(4, listOf(1, 2, 3, 4, 10, 11)),
Pair(5, listOf(1, 2, 3, 4, 5, 11)),
Pair(6, listOf(1, 2, 3, 4, 5, 6))
)


for ((expectedMatchCount, lottoNumbers) in countAndLotto) {

val lotto = Lotto(lottoNumbers)
targetLotto.match(lotto)

val actualMatchCount = targetLotto.matchCount
actualMatchCount shouldBe expectedMatchCount
}
}
Copy link
Member

Choose a reason for hiding this comment

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

이런 종류의 테스트는 @ParameterizedTest를 활용해보는 건 어떨까요?

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