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 로또 #596

Open
wants to merge 8 commits into
base: mia6111
Choose a base branch
from
Open

Step2 로또 #596

wants to merge 8 commits into from

Conversation

mia6111
Copy link

@mia6111 mia6111 commented Jan 2, 2023

안녕하세요~ 로또 step2 PR 드립니다!

}
}

class LottoNumber private constructor(val value: Int) : Comparable<LottoNumber> {
Copy link
Author

Choose a reason for hiding this comment

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

값 객체인 LottoNumber 를 미리 생성해서 상수로 정의(아래 NUMBERS)해놓고 재활용하도록 하고 싶었는데요

  • LottoNumber.of (static factory method ) 를 사용하고
  • 생성자는 private으로 설정
  • LottoNumber 는 값 객체

값 객체로 만들기 위해 data 클래스로 만들경우
data 클래스 + private constructor 를 선언할 경우
Private primary constructor is exposed via the generated 'copy()' method of a 'data' class.
라는 warning 이 뜨더라구요.

일단 data 클래스를 사용하지 않고, toString, equals, hashcode 를 구현하도록했습니다
구글링 해보니 이런 내용이 있긴 한데
https://discuss.kotlinlang.org/t/data-class-with-private-constructor/25599
좋은 방법이 있을지 궁금합니다 :)

Copy link
Member

Choose a reason for hiding this comment

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

기본 생성자와 팩터리 메서드 동작이 완전히 동일한 것 같은데, 팩터리 메서드를 사용하고 싶으신 이유가 있으실까요? 팩터리 메서드를 사용하는 이유에 따라 답변이 달라질 것 같습니다.

Copy link
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

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

안녕하세요 미현님 로또 미션 2단계 잘 진행해주셨습니다.
함께 고민해보면 좋을 내용 코멘트 남겼습니다.

로또 미션 1단계 마지막 피드백이 반영되지 않은 것 같습니다. 😢
확인 후 리팩터링 진행해주세요!

궁금하신 점 언제든지 코멘트, DM 주세요 🙇‍♂️

Comment on lines +1 to +8
# Step2. 로또 기능 요구사항
* 구입금액을 입력받는다
* 구입 금액에 해당하는 로또를 발급한다 (로또 1장의 가격은 1000원이다)
* 발급한 로또를 출력한다
* 지난 주 당첨번호를 입력받는다
* 발급한 로또의 당첨 통계를 구한다
* 발급한 로또의 총 수익률을 구한다
* 당첨 통계와 수익률을 출력한다
Copy link
Member

Choose a reason for hiding this comment

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

마크다운 체크박스 기능을 이용하면 작업 내용을 중간중간 체크하기 용이합니다 😄

- [x] 구입 금액을 입력 받는다.
- [ ] 발급한 로또를 출력한다.

Comment on lines +4 to +16

fun inputPayAmount(): Int {
println("구입금액을 입력해 주세요.")
return readLine()?.toIntOrNull() ?: throw IllegalArgumentException()
}

fun inputLastWinningNumbers(): List<Int> {
println("지난 주 당첨 번호를 입력해 주세요.")

return readLine().orEmpty()
.split(",")
.map { it.toIntOrNull() ?: throw IllegalArgumentException() }
}
Copy link
Member

Choose a reason for hiding this comment

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

원하시는 효과를 가진 메서드로 readln() 이 있는거 같아요.
readLine()readln() 의 차이는 무엇일까요?

Comment on lines +16 to +20
fun buyLotteries(payAmount: Int): Lotteries {
val howMany = (payAmount / PRICE)
val lotteryList = List(howMany) { Lottery(randomLottoNumbers()) }
return Lotteries(lotteryList)
}
Copy link
Member

Choose a reason for hiding this comment

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

payAmount 가 음수일 경우 어떻게 될까요? 🤔

return Lotteries(lotteryList)
}

private fun randomLottoNumbers() = LottoNumber.allNumbers().shuffled().subList(0, Lottery.COUNT)
Copy link
Member

Choose a reason for hiding this comment

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

  • subList() 대신 take() 라는 녀석도 있네요 👀
  • '주어진 숫자중에 6개의 랜덤한 숫자를 가져온다.' 가 정상적으로 동작하는지 테스트 코드를 작성할 수 있을까요?
    • private 접근제어자로 접근이 막혀있어서 테스트가 어렵다면 어떤 해결 방법이 있을까요?

Comment on lines +6 to +20
object LotteryMachine {
private const val PRICE = 1_000

private val LOTTO_RETURN_MAP = mapOf(
3 to 5_000,
4 to 50_000,
5 to 1_500_000,
6 to 2_000_000_000
)

fun buyLotteries(payAmount: Int): Lotteries {
val howMany = (payAmount / PRICE)
val lotteryList = List(howMany) { Lottery(randomLottoNumbers()) }
return Lotteries(lotteryList)
}
Copy link
Member

Choose a reason for hiding this comment

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

  • 로또 구매(발급) 로직을 위해서도
  • 로또 구매 금액을 관리하기 위해서도
  • 로또 당첨 금액을 관리하기 위해서도

모두 LotteryMachine object 를 열어보아야하는데요, 하나의 파일을 여러가지 이유로 열게 되는 것 같습니다.
이럴 경우 어떠한 문제점이 생길까요? 🤔

Comment on lines +37 to +39
return BigDecimal.valueOf(totalPrize / payAmount.toDouble())
.setScale(2, RoundingMode.FLOOR)
}
Copy link
Member

Choose a reason for hiding this comment

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

오! 소수점 둘째자리까지 세심한 요구사항 챙기기 좋습니다 😄 👍
그런데... 소수점 둘째자리까지 표현도 중요한 도메인 지식일까요? 🤔

Comment on lines +3 to +9
class Lotteries(val lotteries: List<Lottery>) {
constructor(vararg lotteries: Lottery) : this(lotteries.toList())

fun count(): Int {
return lotteries.size
}
}
Copy link
Member

Choose a reason for hiding this comment

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

  • 다양한 생성자로 편의챙기기 👍
  • size 를 count 로 바꾸는 메서드 네이밍 👍 💯
  • 혹시 Lotteries 는 data class 가 아닌 이유가 있을까요?

Comment on lines +11 to +20
class Lottery(numbers: List<Int>) {

val numbers: List<LottoNumber>

constructor(vararg inputNumbers: Int) : this(inputNumbers.toList())

init {
require(numbers.size == COUNT)
this.numbers = numbers.map { LottoNumber.of(it) }.sorted()
}
Copy link
Member

Choose a reason for hiding this comment

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

  • 중복에 대해 검증을 진행해주는군요! 👍
    • 그런데.. 중복에 대해 완전히 배제되어야 한다면 List 말고 다른 자료구조가 더 적합한거 아닐까요?
  • 요구사항에 맞게 미리 정렬을 해주는군요!! 👍 👍
    • 그런데.. 오름차순으로 정렬하여 표현하는 것도 비즈니스 로직에 중요한 요소일까요?

}
}

class LottoNumber private constructor(val value: Int) : Comparable<LottoNumber> {
Copy link
Member

Choose a reason for hiding this comment

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

기본 생성자와 팩터리 메서드 동작이 완전히 동일한 것 같은데, 팩터리 메서드를 사용하고 싶으신 이유가 있으실까요? 팩터리 메서드를 사용하는 이유에 따라 답변이 달라질 것 같습니다.

Comment on lines +11 to +12
for (it in lotteries.lotteries) {
println("[${it.numbers.map { it.value }.toList().joinToString(", ")}]")
Copy link
Member

Choose a reason for hiding this comment

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

Lotteries 에게 "숫자값으로 변환된 리스트를 줘" 라고 메세지를 보내보면 어떨까요?

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.

3 participants