-
Notifications
You must be signed in to change notification settings - Fork 358
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
base: sjjeong
Are you sure you want to change the base?
step2: 로또(자동) #717
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.
안녕하세요
몇가지 코멘트 남겨두었으니 확인 부탁드립니다.
@@ -0,0 +1,19 @@ | |||
package lotto | |||
|
|||
class Lotto(val numbers: List<Int>) { |
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.
makeLotto
메서드를 통해 만들면 항상 1-45까지의 6개의 숫자라는 제약이 지켜지겠지만, 생성자가 public으로 열려있어 임의로 생성할 수 있을 것 같아요.
LottoNumber
를 추출해 1-45까지의 제약을 지키는 별도의 래핑된 클래스를 만들고Lotto
클래스에 6개 및 중복 체크 validation을 추가한다.- 생성자를 private으로 변경하고 정적 팩터리인 makeLotto만을 이용해 생성하도록 한다.
위 두가지 방법 중 하나를 골라 제약 조건을 지킬 수 있도록 하면 좋을 것 같네요.
@@ -0,0 +1,39 @@ | |||
package lotto | |||
|
|||
sealed class Prize { |
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.
여기서 로또 당첨 결과에 대한 부분을 정의하는건 열거형(Enum)을 사용하는게 더 적절하지 않을까 하는 생각이 드네요.
다른 데이터를 보유하거나 다른 논리를 코드로 풀어낼 목적의 상수가 필요한 경우라면 sealed class가 적절하겠지만 지금은 money라는 데이터도 동일하고 별도로 다른 논리를 코드로 풀어낼 필요가 없어보여요.
정답은 없으니 이 부분에 대해 한번 고민해보시면 좋겠습니다.
package lotto | ||
|
||
class LottoResult { | ||
private val prizeList = mutableListOf<Prize>() |
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.
모든 당첨 결과를 저장하고 있는건 다소 비효율적이지 않나 하는 생각이 드는데요.
정답이 정해진 것은 아니지만, groupingBy와 eachCount를 활용해 Map<�Prize, Long> 형태로 자료구조를 바로 뽑아 활용하는건 어떨까요?
requireNotNull(money) { | ||
"구매 금액은 null이 아니어야 함" | ||
} | ||
require(money >= LOTTO_PRICE) { | ||
"구매 금액은 1000 보다 커야 함" | ||
} |
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.
여기서 의도한 대로 예외가 발생하는지 등에 대한 단위 테스트를 작성해보시면 좋겠습니다.
@@ -0,0 +1,34 @@ | |||
package lotto | |||
|
|||
class LottoResult { |
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.
이 부분도 단위 테스트를 작성해볼 수 있지 않을까요?
|
||
class LottoTest { | ||
|
||
@Test |
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.
junit의 테스트 어노테이션을 사용하고 계시네요.
자동차 경주 미션 이후로 코틀린이 조금 익숙해지셨다면 kotest의 여러 테스트 레이아웃 및 assertion을 활용해보셔도 좋을 것 같아요.
step1이 아직 완료되지 않았지만 step2와 영향도가 없어서 미리 진행 했습니다