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

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

Conversation

goodbyeyo
Copy link

안녕하세요

자동 로또 리뷰 부탁드립니다.

Copy link

@vsh123 vsh123 left a comment

Choose a reason for hiding this comment

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

안녕하세요!
step2 미션 작성 잘해주셨네요 💯

몇가지 코멘트를 남겼으니 확인부탁드립니다!

object InputView {
fun getUserAmount(): Int {
println("구입 금액을 입력해 주세요.")
val amount: String = readln()
Copy link

Choose a reason for hiding this comment

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

kotlin의 타입추론을 이용해보면 어떨까요?

fun getUserAmount(): Int {
println("구입 금액을 입력해 주세요.")
val amount: String = readln()
return amount.toInt()
Copy link

Choose a reason for hiding this comment

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

toIntOrNull을 활용해서, 숫자가 아닌경우에 대한 처리를 해보면 어떨까요?

import lotto.domain.LottoTickets
import lotto.domain.WinningLotto

class LottoController {
Copy link

Choose a reason for hiding this comment

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

object를 활용해봐도 좋을 것 같아요!

companion object {
const val LOTTO_TICKET_PRICE = 1000

fun purchase(amount: Int): LottoTickets {
Copy link

Choose a reason for hiding this comment

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

amout나 quantity 의 최소값 기준은 없을까요??

Comment on lines 35 to 38
val lottoTickets = ArrayList<LottoTicket>()
repeat(quantity) {
lottoTickets.add(LottoTicket.generateLottoNumber())
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
val lottoTickets = ArrayList<LottoTicket>()
repeat(quantity) {
lottoTickets.add(LottoTicket.generateLottoNumber())
}
val lottoTickets = List(quantity) { LottoTicket.generateLottoNumber()}

위 처럼 작성이 가능해 보여요! :)

}

companion object {
const val LOTTO_TICKET_PRICE = 1000
Copy link

Choose a reason for hiding this comment

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

private 선언이 가능해보이네요!

_tickets = lottoTickets
}

fun calculateLottoRank(winningLotto: WinningLotto): LottoResults {
Copy link

Choose a reason for hiding this comment

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

해당 테스트가 Controller에 있지만, Tickets에도 단위테스트를 만들어 검증해보면 어떨까요?

@@ -0,0 +1,42 @@
package lotto.domain

class LottoTickets(lottoTickets: List<LottoTicket>) : Collection<LottoTicket> by lottoTickets {
Copy link

Choose a reason for hiding this comment

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

위임을 활용하셨군요!! 👍 다만 lottoTickets은 생성자의 인자일뿐 프로퍼티가 아닌데요!

프로퍼티를 기반으로 위임을 진행해야 하지 않을까요?

@Test
fun `로또 티켓을 생성하면 6개의 번호가 들어있다`() {
val lottoTicket = LottoTicket.generateLottoNumber()
lottoTicket.lottoNumbers.size shouldBe 6
Copy link

Choose a reason for hiding this comment

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

shouldHaveSize를 이용해보면 좋을 것 같아요!

fun `로또 티켓을 구입 금액에 맞게 발급한다`() {
val amount = 5_000
val lottoTickets = LottoTickets.purchase(amount)
lottoTickets.tickets.size shouldBe 5
Copy link

Choose a reason for hiding this comment

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

위임을 사용했다면 아래와 같이 사용이 가능해보여요 :)

Suggested change
lottoTickets.tickets.size shouldBe 5
lottoTickets shouldHaveSize 5

1. 타입추론 이용으로 자료형 제거
2. 구입금액 숫자가 아닌 경우 처리
1. repeat 연산자를 List 사용하도록 변경
1. LottoTickets 프로퍼티를 기반으로 위임하도록 변경
2. LottoTicket 위임을 유지하면서 생성자 값 명시적인 변환
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