[로또 2단계] 메다 미션 제출합니다. #130
Conversation
| @@ -0,0 +1,14 @@ | |||
| package lotto.enums | |||
There was a problem hiding this comment.
이번엔 패키지도 신경을 써볼까요? enums 안에 GainLoss Rank 두 가지가 존재하는데 enums라는 패키지 하나로 관리를 한다면 이 패키지는 Constants object 처럼 무진장 많아 질거 같아요.
그래서 제가 추천하는 방법은 이 enum의 역할에 대해서 신경을 써서 패키징을 해보면 좋을거 같아요. 만약 이 enum이 domain에서 사용을 하기 위해서 만들어진 Model이다! 하면 domain으로 갈 수도 있죠
There was a problem hiding this comment.
확실히 역할대로 모아두는게 관리가 더 용이할 것 같네요
도메인의 여러곳에서 사용되는 Rank는 도메인에, GainLoss는 winning과 관련해서만 사용되고 있으니 패키지를 따로 분리해 모아 보았습니다.
There was a problem hiding this comment.
아직 Rank는 enums 안에 있네요 🫠
그러면 나중에 sealed class 같은 것들이 추가되면 sealed 라는 패키지로 들어가는게 맞을까요?
패키지명의 경우 전체적인 객체의 역할을 표현하는게 좋습니다.
domain을 나눈 것도 domain 역할을 하는 것들을 모아두기 위함이니 이 부분도 반영이 된다면 좋겠네요.
There was a problem hiding this comment.
여러곳에 사용되어 어디에 둘지 애매해서 Rank는 일단 보류했었습니다
다음엔 역할을 의미하도록 도메인 이름을 지어 나눠보겠습니다!
| enum class GainLoss( | ||
| private val text: String, | ||
| private val postposition: String, | ||
| ) { | ||
| GAIN("이득", "이라는"), | ||
| PRINCIPAL("본전", "이라는"), | ||
| LOSS("손해", "라는"), | ||
| ; |
There was a problem hiding this comment.
GainLoss가 만들어지는 곳은 EarningInfo 인데요.
EarningInfo는 domain model인데 "이득", "이라는" 같은 View에 표시될 Text를 알 필요가 있을까요?
#127 (comment) 여기 남겨주신 의견과 이 구현은 뭔가 상충하는 느낌이네요 🫠
There was a problem hiding this comment.
enum class에는 단순히 해당 상태값과 관련한 정보를 같이 지니고 있으면 좋겠다고 생각해서 저렇게 처리했는데 결과적으로 Model에 View의 책임까지 가지고 있게 되어 버린 것 같습니다.
enum class의 역할과 사용법부터 다시 공부해봐야겠네요...
There was a problem hiding this comment.
Domain Model의 경우 어떠한 상태 인지만 명시를 해두고 View에 그 상태에 따라 데이터를 표시하는 것도 하나의 방법이죠! enum 자체를 잘못 사용하신 것은 아닙니다. MVC 관점에서 이건 진짜 Model인가? 🤔 라는 의구심에 남겼었습니다
src/test/kotlin/lotto/service/RandomLottoNumbersGeneratorTest.kt
Outdated
Show resolved
Hide resolved
| fun generateLottos( | ||
| payInfo: LottoPayInfo, | ||
| manualTicketsNumbers: List<List<Int>>? = null, | ||
| ): Lottos { | ||
| val manualTickets: List<Lotto> = manualTicketsNumbers?.map { Lotto.createManual(it.toSet()) } ?: listOf() | ||
| val autoTickets: List<Lotto> = List(payInfo.autoLottoQuantity) { Lotto.createRandom() } | ||
| return Lottos(manualTickets + autoTickets) | ||
| } |
There was a problem hiding this comment.
나중에 만약 반자동 로또가 나오게 된다면 이 함수는 이제 반자동 로또 넘버까지 받게 될텐데요 🤔
LottoMachine을 한 번 추상화를 시켜보면 어떨까요? 저는 지금처럼 LottoMachine을 만든 다음 이렇게 비슷한 도메인의 일부 바라보는 비즈니스적인 것만 달라진다면 한 번 추상화를 시켜보는 노력을 하곤 합니다.
There was a problem hiding this comment.
그리고 저희는 모든 원시값과 문자열을 포장한다. 하기로 했습니다. 이 점을 항상 기억해주세요
There was a problem hiding this comment.
리뷰주신 내용을 어떻게 구현해야할지 고민해보다가
interface LottoMachine {
fun generateLottoBundle(
payInfo: LottoPayInfo,
manualTicketsNumbers: List<List<LottoNumber>>? = null,
): List<Lotto>
}로 추상화해서 자동으로 티켓들을 주는 로또 발권기, 수동으로 티켓들을 주는 로또 발권기 등으로 구체화 했습니다. 무심코 사용한 List<List원시값도 포장 된 것 같네요
There was a problem hiding this comment.
이전 피드백에 관한 내용 전부 제 나름대로 해석해 답글로 달면서 반영해 보았는데 패키지는 이렇게 분리하는게 맞는건지, 로또 머신 추상화를 이렇게 처리하는게 맞는지, 원시값을 쓰지 않기 위해 List<List> 를 List<List>로 썼더니 더 복잡해지는것 같아서 구현 방향이 잘못된건지 등 구현할수록 점점 많은 의문이 추가되는 것 같습니다
LottoMachine을 추상화는 �진행을 해주셨는데요.
단 고민을 해보아야 할 것이 있는데요. AutoLottoMachine 의 입장에서 generateLottoBundle 을 할 때 manualTicketsNumbers 을 알아야만 할 이유가 있을까요? 실제로도 사용이 안 되고 있는데 이런 추상화는 오히려 악영향을 끼치게 됩니다. 추상화는 결국 공통 로직을 뽑아내서 이것을 구현하는 얘들은 전부 필요한 기능들이야! 라고 해놓은 것이기도 하기 때문이죠.
LottoMachine 을 추상화 할수도 있고, 다양한 방법들이 있는데요.
그냥 이런 추상화도 있다. 라고 생각을 해주시고 LottoGenerator 라는 것을 추상화를 한다면 이런 느낌의 코드로도 만들어 볼 수 있죠.
// 공통 인터페이스 정의
interface LottoGenerator {
fun generate(): List<Int>
}
// 자동 로또 생성기
class AutoLottoGenerator : LottoGenerator {
override fun generate(): List<Int> {
return (1..45).shuffled().take(6)
}
}
// 수동 로또 생성기
class ManualLottoGenerator(private val numbers: List<Int>) : LottoGenerator {
override fun generate(): List<Int> {
...
return numbers
}
}
// 하이브리드 로또 생성기
class HybridLottoGenerator(private val manualNumbers: List<Int>) : LottoGenerator {
override fun generate(): List<Int> {
...
return manualNumbers + autoNumbers
}
}
// LottoMachine은 다양한 생성기를 받아서 처리할 수 있음
class LottoMachine(private val generators: List<LottoGenerator>) {
fun generateAll(): List<List<Int>> {
return generators.map { it.generate() }
}
}혹은 LottoMachine에게 너는 이런 전략을 사용해서 로또를 만들어! 하고 LottoMachine 자체에게 명령을 하기도 하는데요.
enum class LottoMachinePolicy {
AUTO,
MANUAL
}
class LottoMachine(
private val lottoMachinePolicy: LottoMachinePolicy
) {
fun publishLottoTickets(lottoPurchaseInfo: LottoPurchaseInfo): List<Lotto> {
when (lottoMachinePolicy) {
LottoMachinePolicy.AUTO -> { ... }
LottoMachinePolicy.MANUAL -> { ... }
}
}
}아마 메다가 처음부터 다시 구현할지 고민하는게 추상화 라는게 어려워서 그런 것일텐데요. 추상화 는 원래 어려운 개념이 맞습니다. 어떠한 것이 공통이고 어디서 부터 각각의 역할을 나눠서 구현부를 만들어야 할지도 어렵기 때문인데요. 그래도 메다가 계속 고민을 했던게 앞으로의 미션에서도 좋은 발판이 될 테니 지금 너무 기죽지 않았으면 좋겠어요.
There was a problem hiding this comment.
친절한 예제와 격려 감사합니다 코니!
자바로 개념만 빠르게 배우고 넘어가서 헷갈리던 추상화를 다시 이해하는데 도움이 된 것 같아요👍
재구현을 하면서 LottoTicket을 추상화하는 연습을 해볼 예정입니다!
|
하나 놓친 내용이 있어서 코멘트로 추가드립니다.
로또 숫자가 우리는 1~45라는 것을 도메인적으로 정의를 했어요. 그런데 46이 들어왔으면 그건 |
46이 들어오는것과 같이 |
- require와 check로 던져진 논리 예외만 던지고 나머지는 null처리
셀프 체크리스트
어떤 부분에 집중하여 리뷰해야 할까요?안녕하세요 코니! 특히 테스트 코드 작성 부분은 테스트 함수명을 결정하는것부터 함수명에 맞게 제대로 구현을 맞게 하고 있긴 한건지, 말도 안되는 테스트를 작성하고 있는건 아닌지 스스로 잘 모르겠어서 이번 연휴에 테스트 코드 작성법과 TDD를 좀 더 공부하면서 "모든 코드를 지우고 다시 TDD로 작성해 본다."를 시도 해볼까 고민중인데 이 점 참고하셔서 리뷰해 주시면 감사하겠습니다! |
There was a problem hiding this comment.
메다 안녕하세요! 로또도 어느정도 마무리를 향해 가네요.
수동 로또 에 대한 것이 나오면서 추상화를 어떻게 할지 혹은 전략적으로 어떻게 코딩을 할지를 고민을 많이 하는 모습이 보기 좋습니다! 하지만 추상화에서 조금 아쉬운 부분이 있어서 리뷰를 남겼으며, 메다가 한 번 고민해보면 좋을 것 같네요!
여담으로 지금 TDD로 작성했던 코드들을 리팩토링을 하면서 메다가 많이 혼란을 느끼는 것 같아요.
실제로 프로덕션에서도 옛날 코드를 리팩토링을 할 것인가? 아니면 새로 짤 것인가?
이 둘의 의견이 갈리는데요. 옛날 코드를 리팩토링 하는 것은 정말 어려운 일이에요. 분명 제가 반년 전에 짠 코드도 리팩하는 것을 어렵다고 전부터 느껴 오고 있거든요 🫠...
그렇기에 제이슨이 아예 엎고 새로 만드는 것은 어떤지에 대한 것도 남기기도 했죠.
그렇기에 메다가 지금 겪는 상황이 어찌 보면 저는 지금부터 리팩토링을 어떻게 하면 더 좋을지 고민을 할 수 있는 계기가 된다고 생각하니 너무 혼란스러워 하지는 않았으면 좋겠네요!
| @@ -0,0 +1,14 @@ | |||
| package lotto.enums | |||
There was a problem hiding this comment.
아직 Rank는 enums 안에 있네요 🫠
그러면 나중에 sealed class 같은 것들이 추가되면 sealed 라는 패키지로 들어가는게 맞을까요?
패키지명의 경우 전체적인 객체의 역할을 표현하는게 좋습니다.
domain을 나눈 것도 domain 역할을 하는 것들을 모아두기 위함이니 이 부분도 반영이 된다면 좋겠네요.
| enum class GainLoss( | ||
| private val text: String, | ||
| private val postposition: String, | ||
| ) { | ||
| GAIN("이득", "이라는"), | ||
| PRINCIPAL("본전", "이라는"), | ||
| LOSS("손해", "라는"), | ||
| ; |
There was a problem hiding this comment.
Domain Model의 경우 어떠한 상태 인지만 명시를 해두고 View에 그 상태에 따라 데이터를 표시하는 것도 하나의 방법이죠! enum 자체를 잘못 사용하신 것은 아닙니다. MVC 관점에서 이건 진짜 Model인가? 🤔 라는 의구심에 남겼었습니다
| fun generateLottos( | ||
| payInfo: LottoPayInfo, | ||
| manualTicketsNumbers: List<List<Int>>? = null, | ||
| ): Lottos { | ||
| val manualTickets: List<Lotto> = manualTicketsNumbers?.map { Lotto.createManual(it.toSet()) } ?: listOf() | ||
| val autoTickets: List<Lotto> = List(payInfo.autoLottoQuantity) { Lotto.createRandom() } | ||
| return Lottos(manualTickets + autoTickets) | ||
| } |
There was a problem hiding this comment.
이전 피드백에 관한 내용 전부 제 나름대로 해석해 답글로 달면서 반영해 보았는데 패키지는 이렇게 분리하는게 맞는건지, 로또 머신 추상화를 이렇게 처리하는게 맞는지, 원시값을 쓰지 않기 위해 List<List> 를 List<List>로 썼더니 더 복잡해지는것 같아서 구현 방향이 잘못된건지 등 구현할수록 점점 많은 의문이 추가되는 것 같습니다
LottoMachine을 추상화는 �진행을 해주셨는데요.
단 고민을 해보아야 할 것이 있는데요. AutoLottoMachine 의 입장에서 generateLottoBundle 을 할 때 manualTicketsNumbers 을 알아야만 할 이유가 있을까요? 실제로도 사용이 안 되고 있는데 이런 추상화는 오히려 악영향을 끼치게 됩니다. 추상화는 결국 공통 로직을 뽑아내서 이것을 구현하는 얘들은 전부 필요한 기능들이야! 라고 해놓은 것이기도 하기 때문이죠.
LottoMachine 을 추상화 할수도 있고, 다양한 방법들이 있는데요.
그냥 이런 추상화도 있다. 라고 생각을 해주시고 LottoGenerator 라는 것을 추상화를 한다면 이런 느낌의 코드로도 만들어 볼 수 있죠.
// 공통 인터페이스 정의
interface LottoGenerator {
fun generate(): List<Int>
}
// 자동 로또 생성기
class AutoLottoGenerator : LottoGenerator {
override fun generate(): List<Int> {
return (1..45).shuffled().take(6)
}
}
// 수동 로또 생성기
class ManualLottoGenerator(private val numbers: List<Int>) : LottoGenerator {
override fun generate(): List<Int> {
...
return numbers
}
}
// 하이브리드 로또 생성기
class HybridLottoGenerator(private val manualNumbers: List<Int>) : LottoGenerator {
override fun generate(): List<Int> {
...
return manualNumbers + autoNumbers
}
}
// LottoMachine은 다양한 생성기를 받아서 처리할 수 있음
class LottoMachine(private val generators: List<LottoGenerator>) {
fun generateAll(): List<List<Int>> {
return generators.map { it.generate() }
}
}혹은 LottoMachine에게 너는 이런 전략을 사용해서 로또를 만들어! 하고 LottoMachine 자체에게 명령을 하기도 하는데요.
enum class LottoMachinePolicy {
AUTO,
MANUAL
}
class LottoMachine(
private val lottoMachinePolicy: LottoMachinePolicy
) {
fun publishLottoTickets(lottoPurchaseInfo: LottoPurchaseInfo): List<Lotto> {
when (lottoMachinePolicy) {
LottoMachinePolicy.AUTO -> { ... }
LottoMachinePolicy.MANUAL -> { ... }
}
}
}아마 메다가 처음부터 다시 구현할지 고민하는게 추상화 라는게 어려워서 그런 것일텐데요. 추상화 는 원래 어려운 개념이 맞습니다. 어떠한 것이 공통이고 어디서 부터 각각의 역할을 나눠서 구현부를 만들어야 할지도 어렵기 때문인데요. 그래도 메다가 계속 고민을 했던게 앞으로의 미션에서도 좋은 발판이 될 테니 지금 너무 기죽지 않았으면 좋겠어요.
| interface LottoMachine { | ||
| fun generateLottoBundle( | ||
| payInfo: LottoPayInfo, | ||
| manualTicketsNumbers: List<List<LottoNumber>>? = null, | ||
| ): List<Lotto> | ||
| } |
There was a problem hiding this comment.
아마 로또 머신이 이렇게 추상화가 된 것은
모든 값을 다 전달을 받고 생성만 해주는 역할을 하게 되면서 너무 빈약 해진 것 같다 라는 생각이 들어요.
사용자가 얼마를 넣었다 -> Manual / Auto에 따라서 LottoNumber를 만든다 -> 그 Lotto들을 반환 한다.
이렇게 흘러간다면 조금 더 이 LottoMachine 자체에게 역할을 부여해줄 수도 있을 것 같아요.
셀프 체크리스트
어떤 부분에 집중하여 리뷰해야 할까요?이번에는 코니와 주고받은 피드백을 참조하고 연휴를 활용해서 제로부터 다시 시작해봤습니다! 개인적으로는 이전 코드보다는 상당히 개선되었다고 생각하는데 코니가 보기에 이전 코드들보다 개선된점, 오히려 나빠진 점이 어떻게 느껴지실지 궁금합니다. TDD로 구현을 진행 하기에 앞서 테스트코드와 테스트명은 어떻게 작성하면 좋을까 고민하고 검색과 AI를 사용해서 아래와 같은 템플릿을 사용하는게 좋아보여서 적용 해 보았습니다.
로또 머신을 추상화 예시를 들어주셔서 추상화를 이해하는데 도움이 되었고 똑같이 로또 머신을 만드는건 재미없으니 도메인 객체 어떻게 나눌지도 구현하기 전에 다시 생각하면서 로또 티켓을 추상화하면 괜찮겠다 싶어 구현해보았는데 여전히 제가 올바르게 사용하고 있는지 확신이 서진 않는 것 같습니다... |
There was a problem hiding this comment.
메다 안녕하세요! 아침마다 메다가 새벽까지 커밋을 한 것들을 중간중간 보긴 했었는데요. 처음부터 다시 시작한다는 생각으로 진행하신 것만 해도 대단하다고 생각합니다.
단 처음부터 시작을 할 때 TDD 로 하기로 했지만 커밋상에서는 그것을 제대로 확인이 어려웠다는 것이 아쉽네요. 그리고 추상화 관련해서는 코멘트로 남겨놨으니 확인해 보시면 좋을 것 같습니다.
물론 변경하면서 전반적인 Model의 역할이나 코드의 흐름이 읽기 좋게 변경이 되었다는 점은 느껴졌습니다. 이전에는 runLotto 내부 코드를 볼 때 흐름이 좀 어색한 감이 있었는데 지금은 훨씬 나아졌네요.
추가로 논리적 비논리적 오류에 대해서도 이야기를 해볼까 했는데 지금은 객체지향에 조금 더 집중을 하는 게 좋을 것 같아서 리뷰를 남겨두지는 않았습니다. 이 부분은 앞으로의 미션을 하면서 챙겨보시면 좋을 것 같습니다.
매다는 처음부터 다시 구현을 한 것이니 지금 바로 머지를 하기보다는 메다가 원할 때 머지를 하도록 하겠습니다. 시간을 조금 더 갖고 제 리뷰에 대해서 고민을 해보셔도 좋고 아니면 메다가 다음 미션에 집중을 원한다면 제 의견은 머릿속에만 넣어두시고 다음 미션을 진행하셔도 좋습니다.
연휴 동안에도 고생 많으셨습니다.
| @@ -0,0 +1,20 @@ | |||
| package lotto.domain.valueobject.validator | |||
There was a problem hiding this comment.
#130 (comment) 이 리뷰에서 이어지는 내용인데요. valueobject 라는 네이밍은 사용하지 않으셨으면 합니다.
아마 value class 들을 모아둔 것들을 표현하고자 한 것인데요. 이게 어떤 의미가 있을까요? 패키징 관점에서 어떠한 타입인지는 명시하지 않는 것이 좋은데요.
예를 들어서 value object -> data class 로 변경이 되어도 그 클래스의 역할 자체는 변경이 된 것이 아닌데도 패키지까지 변경이 이뤄져야 하는 상황이죠.
그리고 valueobject라는 패키지가 생길 것이면 dataclass 도 생겨야하지 않을까요?
| @ParameterizedTest | ||
| @ValueSource(ints = [-1, -1000]) | ||
| fun `로또 수량이 음수라면 인스턴스를 생성하지 않는다`(quantity: Int) { |
There was a problem hiding this comment.
이전에 Exception이랑 이어지는 내용인데요. 인스턴스를 생성한다. 생성하지 않는다. 이것도 Exception을 적어두는 것이랑 비슷하다고 생각을 하는데요.
TDD를 하면서 테스트를 하고자 했던 것은 로또 수량이 음수일 수는 없다. 이것이 아니였을까요?
| class OutputView { | ||
| fun showParagraphSeparation() { | ||
| println() | ||
| } | ||
| } |
There was a problem hiding this comment.
feat: 로또 수량 클래스 구현 및 수동 로또 수량 검증 로직 구현 이 커밋 내용에서 이런 것들이 포함이 되었는데요. 나중에는 하나하나 신경 써보면 좋을 것 같습니다.
| import lotto.view.InputView | ||
| import kotlin.runCatching | ||
|
|
||
| class LottoController(private val inputView: InputView) { |
There was a problem hiding this comment.
TDD 로 다시 구현을 해주셨다고는 했지만 커밋 단위에서는 그것을 느끼기는 조금 어려운 감이 있네요.
feat: 로또 지불금 클래스 구현 및 적용 이렇게 됐을 때 로또 지불금인 LottoPaymentMoney 와 LottoPaymentMoneyTest만 있는게 맞지 않았을까요?
There was a problem hiding this comment.
확실히 커밋 단위를 어떻게 나눌지 대해서는 많이 소홀했던것 같습니다
이번에 재구현 하면서 도메인 테스트코드 작성->도메인 구현->관련 컨트롤러 및 뷰 코드 작성 후 커밋을 진행했는데
하나의 커밋에 너무 많은 내용이 들어가있고 커밋과 코드만 봤을 때는 TDD의 느낌이 없긴 한 것 같네요
다음 미션에서는 페어와 합의하에 테스트 코드 먼저 커밋후 진행하는 중이고
컨트롤러와 뷰 코드도 의식적으로 분리해서 커밋해보도록 연습해 보겠습니다.
| - [x] 입력 뷰에서 수동 구매수량을 Int로 입력 받아 컨트롤러에서 ObjectQuantity로 인스턴스화 한다 | ||
| (ObjectQuantity에서 논리오류 예외를 던진 뒤 컨트롤러에서 캐칭 후 재입력 처리) |
There was a problem hiding this comment.
LottoQuantity 에서 ObjectQuantity로 바꾸면서 어떤 이점이 생긴걸까요?
validateManualLottoQuantity 함수만 봐도
private fun validateManualLottoQuantity(lottoPaymentMoney: LottoPaymentMoney): ObjectQuantity {
val manualLottoQuantity = ObjectQuantity(inputView.readManualLottoQuantity())
}manualLottoQuantity 라고 변수명을 했지만 실제로는 ObjectQuantity 라고 하고 있는데요. 처음 보는 입장에서는 이 ObjectQuantity 가 어떠한 도메일 모델의 역할인거지? 라는 의구심이 들게 하네요.
There was a problem hiding this comment.
구매 티켓 개수든 당첨된 티켓 개수든 둘 다 개수를 저장하니까 재사용성을 높일 수 있지 않을까 라는 생각에 하나로 합쳐봤는데 확실히 잘못된 생각이었던 것 같습니다...
class Example1(
val quantity1: ObjectQuantity, val quantity2: ObjectQuantity,
)
class Example2(
val quantity1: LottoQuantity, val quantity2: WinningQuantity,
) 하나의 예로 클라이언트가 Example1을 호출할때는 실수로 quantity1과 2 인자 순서를 바꿔서 입력해 버릴 수 있을 것 같네요 Example2는 그러한것을 미연에 방지 할 수 있을것이구요
코니의 리뷰를 보고 코드를 다시 돌아보면서 객체를 만들때는 추상적으로 만들어서 돌려 쓰지 않고 구체적으로 여러 클래스로 나눠서 사용하는것이 좋을 것 같다고 생각했습니다.
| fun validate( | ||
| lottoPaymentMoney: LottoPaymentMoney, | ||
| manualLottoQuantity: ObjectQuantity, | ||
| ) { |
There was a problem hiding this comment.
제가 이 클래스를 사용을 한다면 validate 를 검사하는 것이니 내가 넣은 인자에 대한 validate 결과가 나오겠구나! 하고 있을 텐데 갑자기 throw를 당해버릴 것 같아요.
| private fun createWholeAutoLottoTickets(autoLottoQuantity: ObjectQuantity): List<LottoTicket> { | ||
| if (autoLottoQuantity.quantity == 0) return emptyList() | ||
| return List(autoLottoQuantity.quantity) { AutoLottoTicket() } | ||
| } | ||
|
|
||
| private fun createWholeManualLottoTickets(manualLottoQuantity: ObjectQuantity): List<LottoTicket> { | ||
| if (manualLottoQuantity.quantity == 0) return emptyList() | ||
| inputView.showManualLottoNumbersAlert() | ||
| val manualLottoTickets = List(manualLottoQuantity.quantity) { retryUntilSuccess { createSingleManualLottoTicket() } } | ||
| outputView.showParagraphSeparation() | ||
| return manualLottoTickets | ||
| } |
There was a problem hiding this comment.
전반적으로 quantity 가 0개일 때 emptyList를 반환하고 있는데 emptyList 로 미리 반환을 할 때 controller 가 무언가를 할 수 있을까요? 저는 딱히 없어 보이는데요
fun main() {
val list = List(0) { 0 }
println(list)
}위 코드를 실행하면 그냥 emptyList 인 모습인데요. 지금 코드에도 그렇게 해도 되지 않았을까? 하는 의구심이 좀 있네요
| fun getSortedLottoNumbers(): List<LottoNumber> = lottoNumbers.sortedBy { it.value } | ||
|
|
||
| fun getRankByWinInfo(winTicketInfo: WinTicketInfo): Rank { | ||
| val countOfMatch = | ||
| winTicketInfo.winLottoTicket.lottoNumbers | ||
| .intersect(lottoNumbers) | ||
| .size | ||
| val isMatchedBonus = winTicketInfo.bonusNumber in lottoNumbers | ||
|
|
||
| return Rank.valueOf(countOfMatch, isMatchedBonus) | ||
| } |
There was a problem hiding this comment.
https://github.com/woowacourse/kotlin-lotto/pull/130#discussion_r1976203814를 들어주셔서 추상화를 이해하는데 도움이 되었고 똑같이 로또 머신을 만드는건 재미없으니 도메인 객체 어떻게 나눌지도 구현하기 전에 다시 생각하면서 로또 티켓을 추상화하면 괜찮겠다 싶어 구현해보았는데 여전히 제가 올바르게 사용하고 있는지 확신이 서진 않는 것 같습니다...
LottoTikcet 은 왜 interface로 구현을 하셨을까요?
interface 는 보통 단순히 “계약(Contract)”을 정의한다고도 많이들 이야기를 하는데요. 그런 관점에서 봤을 때 지금 interface는 너무 많은 것을 직접 구현 중인 것 같아요.
interface와 abstrace class 의 차이점을 찾아보시면 좋을 것 같습니다.
결과적으로 추상화를 하긴 했지만 LottoTicket 자체에 너무 많은 일이 부여가 된 것 같네요
There was a problem hiding this comment.
LottoTikcet를 interface로 구현을 한 이유는 솔직하게 말씀드리면 코틀린에서의 인터페이스, 추상클래스, 상속 관련해서는 명확하게 알지 못했기 때문입니다. 앞으로 미션을 진행하면서 공부를 더 해 나가야 할 것 같네요
자바를 많이 써보진 않았지만 자바에선 인터페이스에 기능구현이 아예 불가능 했던것으로로 기억하는데 코틀린에서는 디폴트 메서드라는 저런 방식으로 구현 가능하다는게 구현하면서 신기했었습니다.
공통적인 상태나 초기화 로직이 필요 없고 단순히 공통 로직만 필요한 경우에는 인터페이스와 디폴트 메서드 사용이 권장되는 것 같고 현재는 공통적으로 로또 번호를 가지고 있어야 하므로 추상 클래스로 전환하는것이 좋겠네요!
| class ManualLottoTicket( | ||
| private val _lottoNumbers: Collection<LottoNumber>, | ||
| ) : LottoTicket { |
There was a problem hiding this comment.
LottoTicket에서 어떠한 Collection이든 받고, 받은 값은 private하게 감춘 뒤 Set으로만 반환하게 하신 것 좋습니다 👍
lottoNumbers 를 사용하는 입장에서는 Set 으로 되어있다면 그 값은 중복이 없겠구나! 라는 것을 명확히 알 수 있기 때문이죠
| val lottoPaymentMoney: LottoPaymentMoney, | ||
| val winRankCounts: Map<Rank, ObjectQuantity>, |
- 숫자 한개가 틀리면 3등으로 예측해야 하지만 실수로 2등으로 예측값을 설정한 부분 수정
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 코니!
step1 피드백을 보고 저의 생각 (Companion Factory 패턴, Output View의 관점)대로 우선 반영 한 뒤 step2 구현을 해보았습니다.
논리적인 오류가 아니면 예외를 던지지 말고 null을 반환한다요구 조건에서 논리적인 오류와 그렇지 않은 오류의 구별과 null을 반환하고 어떻게 처리할지 아직 개념이 확실하게 잡히지 않아 일단 나머지만 빠르게 반영해 보았습니다.step1 피드백을 의도대로 제가 구현한게 맞는지, VIew의 관점에서 보너스볼 일치나 다른 더 생각 해볼 만한 점이 있는지, step2 구현 방향은 나쁘지 않은지 확인해 주시면 감사하겠습니다!