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

Open
wants to merge 1 commit into
base: sjjeong
Choose a base branch
from
Open

step2: 로또(자동) #717

wants to merge 1 commit into from

Conversation

sjjeong
Copy link

@sjjeong sjjeong commented Jun 19, 2023

step1이 아직 완료되지 않았지만 step2와 영향도가 없어서 미리 진행 했습니다

@sjjeong sjjeong changed the base branch from main to sjjeong June 19, 2023 12:39
Copy link

@kyucumber kyucumber left a comment

Choose a reason for hiding this comment

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

안녕하세요

몇가지 코멘트 남겨두었으니 확인 부탁드립니다.

@@ -0,0 +1,19 @@
package lotto

class Lotto(val numbers: List<Int>) {

Choose a reason for hiding this comment

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

makeLotto 메서드를 통해 만들면 항상 1-45까지의 6개의 숫자라는 제약이 지켜지겠지만, 생성자가 public으로 열려있어 임의로 생성할 수 있을 것 같아요.

  1. LottoNumber를 추출해 1-45까지의 제약을 지키는 별도의 래핑된 클래스를 만들고 Lotto 클래스에 6개 및 중복 체크 validation을 추가한다.
  2. 생성자를 private으로 변경하고 정적 팩터리인 makeLotto만을 이용해 생성하도록 한다.

위 두가지 방법 중 하나를 골라 제약 조건을 지킬 수 있도록 하면 좋을 것 같네요.

@@ -0,0 +1,39 @@
package lotto

sealed class Prize {

Choose a reason for hiding this comment

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

여기서 로또 당첨 결과에 대한 부분을 정의하는건 열거형(Enum)을 사용하는게 더 적절하지 않을까 하는 생각이 드네요.

다른 데이터를 보유하거나 다른 논리를 코드로 풀어낼 목적의 상수가 필요한 경우라면 sealed class가 적절하겠지만 지금은 money라는 데이터도 동일하고 별도로 다른 논리를 코드로 풀어낼 필요가 없어보여요.

정답은 없으니 이 부분에 대해 한번 고민해보시면 좋겠습니다.

package lotto

class LottoResult {
private val prizeList = mutableListOf<Prize>()

Choose a reason for hiding this comment

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

모든 당첨 결과를 저장하고 있는건 다소 비효율적이지 않나 하는 생각이 드는데요.

정답이 정해진 것은 아니지만, groupingBy와 eachCount를 활용해 Map<�Prize, Long> 형태로 자료구조를 바로 뽑아 활용하는건 어떨까요?

Comment on lines +7 to +12
requireNotNull(money) {
"구매 금액은 null이 아니어야 함"
}
require(money >= LOTTO_PRICE) {
"구매 금액은 1000 보다 커야 함"
}

Choose a reason for hiding this comment

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

여기서 의도한 대로 예외가 발생하는지 등에 대한 단위 테스트를 작성해보시면 좋겠습니다.

@@ -0,0 +1,34 @@
package lotto

class LottoResult {

Choose a reason for hiding this comment

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

이 부분도 단위 테스트를 작성해볼 수 있지 않을까요?


class LottoTest {

@Test

Choose a reason for hiding this comment

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

junit의 테스트 어노테이션을 사용하고 계시네요.

자동차 경주 미션 이후로 코틀린이 조금 익숙해지셨다면 kotest의 여러 테스트 레이아웃assertion을 활용해보셔도 좋을 것 같아요.

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.

None yet

2 participants