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 로또 자동 #595

Open
wants to merge 3 commits into
base: leehaeina
Choose a base branch
from
Open

Step2 로또 자동 #595

wants to merge 3 commits into from

Conversation

leehaeina
Copy link

안녕하세요 리뷰어님 ㅎㅎ
제가 이런저런일들.. 포함해서 조금 늦었습니다. ㅠㅠ
기다려주셔서 감사합니다!
얼른 로또 미션을 시작해보았는데요 만들고 나서 보니 세부적인 미션내용을 포함하지 못한것같네요 ㅠㅠ
차차 리뷰반영하며 추가해보도록하겠습니다!
늦은 밤에 리뷰를 올려 미흡한 부분이있는데 추가하도록 하겠습니다~!
새해복많이 받으세요!

@leehaeina leehaeina changed the base branch from main to leehaeina January 2, 2023 12:23
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단계 미션 고생 많으셨습니다 :)
미션을 실행시키면 아래 사진 처럼 정상적인 실행이 되지 않고 있는데요, 이 부분 꼭 고치셔서 다시 리뷰 요청 부탁드립니다 :)
image
또, 테스트 코드가 너무 부족해서, 다른 클래스들의 단위 테스트들을 모두 만들어주세요! 😀
코멘트 남겨두었으니 확인 후 다시 리뷰 요청 부탁드릴게요!

Comment on lines +12 to +13
val inputView = InputView
val outputView = OutputView
Copy link
Member

Choose a reason for hiding this comment

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

object는 변수에 할당 없이 바로 접근해서 메서드를 호출해보는 것은 어떨까요?

InputView.purchaseInputView()

Comment on lines +3 to +4
interface Lotto {
val numbers: List<Int>
Copy link
Member

Choose a reason for hiding this comment

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

Int를 래핑해서 로또 번호 하나를 나타내는 객체를 만들어보는 것은 어떨까요?

Comment on lines +8 to +16
class PurchasedLotto(override val numbers: List<Int>, override val winningCount: Int = 0) : Lotto {
fun calculateWinningCount(winningLotto: WinningLotto): Int{
return winningLotto.numbers.filter {
this.numbers.contains(it)
}.size
}
}

class WinningLotto(override val numbers: List<Int>, override val winningCount: Int? = null) : Lotto
Copy link
Member

Choose a reason for hiding this comment

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

구매한 로또가 맞은 개수를 갖고 있는 게 어색해보여요.
지금은 로또와 당첨 로또가 동일하게 6개씩 숫자를 갖고있으니, 동일한 객체로 보여요 :)
Lotto는 하나의 객체로만 만들고, Lotto끼리 비교해서 몇 개가 맞았는지 개수를 반환하는 메서드를 만들어보는 것은 어떨까요?

Comment on lines +3 to +14
class LottoInput(
val purchaseCount: Int? = 0,
val purchaseAmount: Int? = 0,
val winningLotto: List<Int>? = listOf()
) {
fun of(lottoInput: LottoInput): LottoInput {
return LottoInput(
purchaseCount = lottoInput.purchaseCount ?: this.purchaseCount,
purchaseAmount = lottoInput.purchaseAmount ?: this.purchaseAmount,
winningLotto = lottoInput.winningLotto ?: this.winningLotto
)
}
Copy link
Member

Choose a reason for hiding this comment

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

기존에 있던 객체에서 다시 재활용하면서 객체를 복사하는 일은 불필요해보입니다.
InputView에서는 필요한 입력 값들만 반환하게 만들어보는 것은 어떨까요?

Comment on lines +3 to +4
enum class Rank(val matchedCount: Int? = 0, val prize: Int = 0) {
FAIL(null, 0),
Copy link
Member

Choose a reason for hiding this comment

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

enum class의 항목들이 이미 모든 값을 갖고 있기 때문에, default parameter를 갖지 않아도 되어 보입니다.
또, 상품이 없는 것은 null로 표현하기 보다 0 등의 숫자로 표현해보는 것은 어떨까요?

Comment on lines +7 to +8
SECOND(5, 1500000),
FIRST(6, 2000000000);
Copy link
Member

Choose a reason for hiding this comment

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

숫자는 언더바 ( _ ) 로 구분해서 작성이 가능합니다.
또, 1등이 여러번 당첨 될 수도 있으니, Int 보단 Long을 활용해보는 것은 어떨까요?

Comment on lines +9 to +13
val result = mutableListOf<PurchasedLotto>()
for(i:Int in 0 until count){
result.add(PurchasedLotto(randomNumberGenerator.getGeneratedNumber()))
}
return result
Copy link
Member

Choose a reason for hiding this comment

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

코틀린의 확장함수 map등을 활용해보는 것은 어떨까요?

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