4단계 - 로또(수동) #1128
4단계 - 로또(수동) #1128
Conversation
| @Test | ||
| fun `정해진 개수 만큼 자동 로또를 생성 할 수 있다`() { | ||
| val lottoTickets = AutoLottoIssuer.issueAutoLottoTickets(3) { createLottoNumbers(1, 2, 3, 4, 5, 6) } | ||
| lottoTickets.lottoTickets.size shouldBeEqual 3 |
There was a problem hiding this comment.
shouldHaveSize를 활용해보면 어떨까요?
There was a problem hiding this comment.
제안주신 shouldHaveSize 활용으로 수정했습니다!
덕분에 테스트의 가독성을 높이고, 테스트 의도를 더 명확하게 표현할 수 있을 것 같습니다.
이번 리뷰를 통해서 더 직관적이고 명확한 테스트 코드 작성을 위해 테스트 라이브러리 관련 문서를 정독 해야겠다고 다시금 느끼게 되네요 🥲
피드백 감사합니다!
|
|
||
| class ManualLottoTicket(override val lottoNumbers: LottoNumbers) : LottoTicket() | ||
|
|
||
| class AutoLottoTicket(private val generateLottoNumbers: () -> LottoNumbers) : LottoTicket() { |
There was a problem hiding this comment.
generateLottoNumbers는 프로퍼티는 아닌걸로 보이네요! :)
| class AutoLottoTicket(private val generateLottoNumbers: () -> LottoNumbers) : LottoTicket() { | |
| class AutoLottoTicket(generateLottoNumbers: () -> LottoNumbers) : LottoTicket() { |
There was a problem hiding this comment.
해당 함수는 AutoLottoTicket 생성 시점에만 호출되어 로또 번호를 생성하므로, 프로퍼티 대신 바로 생성자 내부에서 값을 초기화하는 방식으로 수정하는 것이 더 적절하겠네요!
덕분에 불필요한 프로퍼티 선언을 제거하고, 코드의 명확성과 간결성을 높일 수 있을 것 같습니다.
제안해주신대로 변경했습니다 👍
| @@ -1,11 +1,17 @@ | |||
| package lotto.domain | |||
|
|
|||
| data class LottoTicket(private val generateLottoNumbers: () -> Set<Int>) { | |||
| val lottoNumbers = LottoNumbers(generateLottoNumbers().map { LottoNumber.of(it) }.toSet()) | |||
| sealed class LottoTicket { | |||
There was a problem hiding this comment.
sealed class 👍
sealed interface도 있는데 sealed class로 작성하신 이유와 이를 나누는 기준이 있을까요? (질문입니다!!)
There was a problem hiding this comment.
sealed class인 LottoTicket은 상속을 통해 구체적인 구현체인 ManualLottoTicket과 AutoLottoTicket이 공통적으로 사용할 수 있는 메서드(checkLottoWinnerRank)를 제공하고 있는데요.
이처럼 구현체 간에 공통 기능이나 상태를 정의해야 할 때 sealed interface 보단 sealed class가 적합하다고 판단했습니다
만약, 클래스 내부에서 공유할 구체적인 구현이나 상태가 없다면, 특정 메서드를 구현하도록 강제하는 sealed interface를 고려했을 것 같습니다
sealed class와 sealed interface 둘 중에 무엇이 더 적절한지 다시 한번 생각해 볼 수 있는 좋은 질문을 주셔서 감사합니다!
혹시 다른 의견 있다면 피드백 부탁드립니다 🙇
|
|
||
| fun checkLottoWinnerRank(lottoWinnerNumbers: LottoWinnerNumbers): LottoWinnerRank { | ||
| val matchCount = lottoNumbers.checkLottoNumbersMatch(lottoWinnerNumbers.lottoNumbers) | ||
| val bonusCheck = lottoNumbers.contains(lottoWinnerNumbers.bonusNumber) | ||
| return LottoWinnerRank.getRankByMatches(matchCount = matchCount, bonusCheck = bonusCheck) | ||
| } | ||
|
|
||
| class ManualLottoTicket(override val lottoNumbers: LottoNumbers) : LottoTicket() |
There was a problem hiding this comment.
당시 다른 로직에 신경쓰느라 이 부분은 깊게 고민하지 못하고 자연스럽게 nested class로 선언하게 된 것 같은데요 😢
다시 한번 nested class로 선언하게 된 주된 이유를 떠올려 봤을 때,
기능 구현상 LottoTicket과 관련된 클래스가 수동 로또, 자동 로또만 고려될 수 있고,
LottoTicket의 checkLottoWinnerRank 메서드 외에 하위 클래스별로 서로 다른 메서드가 존재하지 않아
외부 클래스보다 nested class로 구현된 코드가 좀 더 이해하기 쉽고 관리하기가 용이할 것으로 판단한 것 같습니다
혹시 다른 의견이 있다면 말씀부탁드립니다!
| val lottoNumbersList = mutableListOf<LottoNumbers>() | ||
| repeat(manualLottoCount) { lottoNumbersList.add(inputManualLottoNumbers()) } | ||
|
|
||
| val lottoTickets = lottoNumbersList.map { ManualLottoTicket(it) } |
There was a problem hiding this comment.
View에서 Tickets까지 만들어내는건 과한 책임인 것 같기도 한데요! List 혹은 List정도만 넘겨주면 어떨까요?
그러면서 중첩리스트를 반환하는게 괜찮은지? DTO를 만들어야하는지도 고민해보면 좋을 것 같아요!
| val lottoNumbersList = mutableListOf<LottoNumbers>() | ||
| repeat(manualLottoCount) { lottoNumbersList.add(inputManualLottoNumbers()) } |
There was a problem hiding this comment.
이런식으로도 작성이 가능해보이네요! :)
| val lottoNumbersList = mutableListOf<LottoNumbers>() | |
| repeat(manualLottoCount) { lottoNumbersList.add(inputManualLottoNumbers()) } | |
| val lottoNumbersList = List(manualLottoCount) { inputManualLottoNumbers() } |
There was a problem hiding this comment.
제안해 주신 대로 코드를 작성하면 한 줄로 더 간결하게 표현할 수 있겠네요!
map을 통해 ManualLottoTicket을 생성하는 부분이 바로 리스트 초기화와 결합되어 코드가 더 깔끔해진 것 같습니다.
가독성을 높일 수 있는 좋은 예시를 제안해 주셔서 감사합니다.
|
|
||
| val combinedManualLottoAndAutoLotto = manualLottoTickets.lottoTickets.plus(autoLottoTickets.lottoTickets) | ||
|
|
||
| return LottoTickets(combinedManualLottoAndAutoLotto) |
There was a problem hiding this comment.
마지막 return전까지 자동 LottoTickets객체와 수동 LottoTickets객체가 유의미하게 쓰인적이 없는것같아요!! (전부다 lottoTickets 프로퍼티를 꺼내서 검증을 진행했네요!)
자동, 수동 LottoTickets객체를 미리 만들 필요가 있는지 고민해보면 좋을 것 같습니다!
선호님 안녕하세요!
4단계 미션에도 오랜 시간이 걸렸네요 😭
마지막 4단계 미션에 주요하게 반영한 부분을 정리하면 아래와 같습니다
LottoTicket을 sealed class로 변경하여 수동 로또(ManualLottoTicket), 자동 로또(AutoLottoTicket) 추가
컬렉션으로 활용하던 로또 번호들을 LottoNumber, LottoNumbers 일급 컬렉션으로 분리
LottoNumber 객체 재사용을 위한 플라이웨이트 패턴 적용
로또 등수를 관리할 LottoWinnerRank 객체로 enum 활용
당첨 번호를 관리하는 LottoWinnerNumbers 객체에 당첨 번호 추가
언제나 꼼꼼히 리뷰해주셔서 감사합니다!