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] 로또(자동) #601

Open
wants to merge 11 commits into
base: kminkyung
Choose a base branch
from
Open

Conversation

kminkyung
Copy link

  • 지난 PR에서 코멘트 주신 내용을 반영했습니다.
  • 2단계 로또를 구현했습니다.

리뷰 부탁드립니다!

Copy link

@BeokBeok BeokBeok left a comment

Choose a reason for hiding this comment

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

이전 단계 피드백과 로또(자동) 미션하시느라 고생하셨습니다. 👍
고민해볼만한 의견들을 코맨트로 작성하였으니, 충분히 고민해보시고 도전해보세요. 💪

) {
init {
if (numbers.isNotEmpty()) {
check(numbers.size == 6)
Copy link

Choose a reason for hiding this comment

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

숫자 6은 어떤 것을 의미할까요?

package lottery.domain.lotto

class Lotto(
var numbers: List<Int> = emptyList()
Copy link

Choose a reason for hiding this comment

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

Suggested change
var numbers: List<Int> = emptyList()
val numbers: List<Int> = emptyList()

컬렉션은 외부에서 주솟값을 바꾸지 않아도 add, remove 함수를 통해 데이터를 조작할 수 있습니다.
컬렉션은 val을 활용해보면 어떨까요?

Comment on lines +14 to +16
private fun createNumbers() {
numbers = numberRange.shuffled().subList(0, LOTTO_SLOT).sorted()
}
Copy link

Choose a reason for hiding this comment

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

로또 번호가 중복되면 어떻게 될까요?

Comment on lines +10 to +12
val earning = Ranking.values().sumOf {
result[it.rank]!!.times(it.prize)
}
Copy link

Choose a reason for hiding this comment

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

CalculatorService 객체에서 Ranking 객체에 직접 접근해서 계산을 해주고 있네요.
만약 Ranking에 값이 추가가 되거나 제거가 된다면, Ranking 클래스 뿐만 아니라 CalculatorService에도 영향을 미치게 됩니다.
Ranking 클래스에 메시지를 보내서, Ranking이 변경되더라도 CalculatorService에는 영향을 미치지 않도록 코드를 개선해보면 어떨까요?

@@ -0,0 +1,3 @@
package lottery.domain.winningresult

typealias NumberOfWins = Int
Copy link

Choose a reason for hiding this comment

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

Int타입을 NumberOfWins로 타입의 이름을 변경해주셨네요.
다만, 지금처럼 사용하게 된다면 NumberOfWins외 다른 Int를 사용하는 경우와 햇갈릴 수 있습니다.
typealias 보다 value class를 활용해보면 어떨까요?

ref. https://quickbirdstudios.com/blog/kotlin-value-classes/


class WinningResultServiceSpec : BehaviorSpec({

Given("당첨 결과 서비스는") {
Copy link

Choose a reason for hiding this comment

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

테스트 코드를 잘 작성해주셨네요. 👍
하지만 한번에 모든 경우를 테스트를 작성하게 되면, 당첨에 대한 요구사항이 늘어나도 테스트가 통과하니 그냥 넘어가는 경우가 많습니다.
1등부터 꼴등까지 모든 케이스를 하나하나 분리해보면 어떨까요?

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