-
Notifications
You must be signed in to change notification settings - Fork 355
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
base: sendkite
Are you sure you want to change the base?
Step2 로또 자동 #992
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.
2단계 미션 고생 많으셨어요!
생각해보면 좋을 점들에 코멘트 남겨두었어요.
피드백 반영 후 다시 리뷰 요청 부탁드려요!
fun isNumber(number: String): Boolean { | ||
return Regex("[0-9]+").matches(number) | ||
} | ||
|
||
fun findCustomDelimiter(text: String): String { | ||
return Regex("//(.)\n(.*)").find(text)?.groupValues?.get(1) ?: "" |
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.
#981 (comment)
여전히 함수를 부를 때마다 Regex 객체를 생성하고 있어요!
Regex를 상수로 분리해보는 건 어떨까요?
또, 해당 메서드들은 이 역할을 가져야할 객체에게 위치시켜보는 건 어떨까요?
class Lotto( | ||
val numbers: List<Int> = generateLottoNumbers(), |
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.
Int에는 1~45 라는 로또 번호를 제한하기에는 너무 범용적인 자료형이라 생각해요.
로또 번호를 나타내는 도메인 객체를 만들어보는 건 어떨까요?
fun match(target: Lotto) { | ||
matchCount = numbers.intersect(target.numbers).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.
target 과 비교한 후, 이 로또는 matchCount를 그대로 갖고있을 것 같아요.
이후 이 값은 어떤 로또와 비교했었는지 추적하기 어려워질 것으로 우려돼요.
matchCount에 사이드이펙트를 발생시키는 것 보다, match 한 결과를 반환하게 만드는 것은 어떨까요?
class LottoStore { | ||
|
||
fun sell(money: Int): List<Lotto> { |
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.
LottoStore입장에서는 sell 하는 것이 맞는데, 이 메서드를 사용하는 사람 입장에서는 굉장히 어색할 것으로 예상돼요.
returnNumber 같은 말 보다는, getNumber를 쓰듯이요!
val lottos = mutableListOf<Lotto>() | ||
for (i in 1..count) { | ||
lottos.add(Lotto()) | ||
} | ||
return lottos |
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..count).map { }
혹은, List의 fake constructor를 활용해보셔도 좋겟네요 :)
List(count) { ... }
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}입니다.") |
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.
로또 등수에 대해 도메인 객체로 표현해보는 건 어떨까요?
@CsvSource(value = ["1000,1", "2000,2", "3000,3"]) | ||
fun `로또를 여라장 구매한다`(money: Int, expected: Int) { | ||
val lotto = LottoStore().sell(money) | ||
lotto should haveSize(expected) |
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.
몇 장을 샀는지 검증하는 것은 테스트에 큰 의미를 가지지 못한다고 생각해요.
개발자가 제어 가능한 형태로, 어떤 티켓이 반환되었는지까지 테스트할 수 있게 만들어보는 건 어떨까요?
@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 | ||
} | ||
} |
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.
이런 종류의 테스트는 @ParameterizedTest
를 활용해보는 건 어떨까요?
리뷰어님 안녕하세요!
로또 자동 PR 드립니다.
bottom up 방식으로 구현하려고 노력했습니다.
오브젝트책의 영화 티켓 판매기에 영감을 받아 비슷하게 작성해보려 했는데 쉽지 않네요.
작업내역