[로또 2단계] 시아 미션 제출합니다. #135
Conversation
|
잘못된 입력을 받았을 때 처리하는 것/논리적 오류와 비 논리적 오류 분리를 중점으로 수정해보았습니다! |
There was a problem hiding this comment.
예외 처리를 통해 에러가 발생하지 않도록 한다.
일반적으로 try-catch를 사용하지 않는 말 때문에 문제가 시작된 것으로 보이지만...
좋은 코드는 정말 이렇게 짜야 한다고..? 라는 어색함이 생기기를 바라고 있습니다 😄
예외 처리 관련해서는 더이상 개선하지는 마시고, 나중에 더 학습이 되신 후에 지금 코드를 돌아보시는 방법이 좋을 것 같아요.
나중에 자신의 코드를 돌아볼 때 느끼는 점이 훨씬 클 것 같다?
지금은 로또를 진행하기 위한 어색한 흐름을 해결하고 정리하는 것을 챙겨보시죠.
|
너무 늦게 완성했습니다. 죄송합니다. |
There was a problem hiding this comment.
시아 고생 많으셨습니다.
전부 갈아엎고 다시 시도한 것만으로도 우선 칭찬드리고 싶습니다.
TDD 관점에서 리뷰를 드렸으며 객체지향적인 사고와 API 설계 관점에서 다시 처음부터 의견 남겼습니다.
객체가 스스로 해야할 역할을 팩토리 메서드나 다른 클래스에 만들어 자율적인 객체와는 거리가 더 멀어진 것 같습니다.
더 성장하시고 싶어 하신 모습에 따라 피드백이 다소 쎄게 들어갔으니 양해 부탁해요. 😢
할 수 있는 만큼만 반영 하시고 6시 전에 제출하려고 하다말고 올리는 것보다는 차라리 전부 반영하고 나중에 다시 올려주시는게 나을 것 같아요.
src/main/kotlin/lotto/LottoNumber.kt
Outdated
| fun valueOf(value: Int): LottoNumber { | ||
| require(value in VALID_RANGE) { RANGE_ERROR } | ||
| return LottoNumber(value) | ||
| } | ||
|
|
||
| fun createOrNull(value: Int): LottoNumber? { | ||
| if (validation(value)) return LottoNumber(value) | ||
| return null | ||
| } |
There was a problem hiding this comment.
API 설계자의 입장으로서 바라보면 2가지가 아쉬운데요.
-
생성 인터페이스에 일관성이 부족하다
하나는 valueOf 다른 하나는 create로 이어지는 흐름이 부자연스러워요.
예를 들어 toInt(), toIntOrNull() 같은 예시를 들 수 있겠죠. -
private constructor가 만능은 아닙니다.
data class로 만들면 불가능한 옵션이기도 하고 클래스의 논리적인 흐름을 외부로 넘겼을 뿐이라서요.
init { require(...) }
fun valueOf(value: Int): LottoNumber {
LottoNumber(...)
}
fun valueOfOrNull(value: Int): LottoNumber ? {
runCatching { valueOf(value) }
.getOrNull()
}There was a problem hiding this comment.
19253ff
생성 일관성을 제시해주신 코드를 바탕으로 추가하였습니다!
README.md
Outdated
| - [x] 로또 1매에는 6개의 로또 번호가 존재한다. | ||
| - [x] 로또 번호는 중복되지 않는다. | ||
| - [x] 로또 번호는 오름차순으로 정렬된다. |
There was a problem hiding this comment.
feat : 로또 객체 생성
TDD로 한다면 위 기능 3가지를 모두 별도의 커밋으로 하는게 좋았을 것 같아요.
지금은 이미 완성된 모든 기능을 이해한 채로 객체부터 만들게 되니까요
src/main/kotlin/lotto/Lotto.kt
Outdated
| fun createOrNull(numberList: List<LottoNumber>): Lotto? = | ||
| when { | ||
| numberList.size != LOTTO_NUMBER_QUANTITY -> null | ||
| numberList.distinctBy { it.value }.size != numberList.size -> null | ||
| else -> Lotto(numberList) | ||
| } |
There was a problem hiding this comment.
마찬가지로 정적 팩토리 메서드가 객체를 생성하는 책임 외에도, 비즈니스 로직을 가지기 때문에 책임이 여러개로 나뉘었다고 생각합니다.
생성자에서 이미 유효성 검사를 하고 있다면 굳이 중복할 필요가 없습니다.
src/main/kotlin/lotto/Amount.kt
Outdated
| class Amount private constructor( | ||
| val money: Int, | ||
| ) { | ||
| fun getCount(lottoPrize: Int): Int = money / lottoPrize |
There was a problem hiding this comment.
Amount를 유연하게 만들었다면, 더이상 "로또" 라는 관심사를 가질 필요가 있을까요?
이 클래스는 LottoAmount가 되어야 했을까요?
src/main/kotlin/lotto/Amount.kt
Outdated
| fun paymentOrNull(payMoney: Int): Amount? { | ||
| if (money < payMoney) return null | ||
| return Amount(money - payMoney) | ||
| } |
There was a problem hiding this comment.
금액은 음수일 수 없다는 것을 null로 하는게 자연스러운 흐름일까요?
물론 마이너스가 되면 논리적으로 문제가 될 수 있으니 null을 반환할 수 도 있다고 생각하지만.
아무리 빼더라도 0보다 작아지지 않는다는 개념을 적용하면 이렇게 만들어 볼 수도 있습니다.
data class Amount(val money: Int) {
require(money >= 0)
fun pay(other: Amount): Amount {
val newMoney = (money - other.money).coerceAtLeast(0)
return copy(money = newMoney)
}
}결과적으로 이 함수를 사용할 때 마다 null을 컨트롤 해야한다는게 매우 번거롭다고 느껴지니까요
| is LottoCreationResult.Failure.InvalidCount -> outputView.printErrorMessage(Message.errorInvalidLotto()) | ||
| is LottoCreationResult.Failure.DuplicatedNumbers -> outputView.printErrorMessage(Message.errorInvalidLotto()) | ||
| is LottoCreationResult.Failure.NotSorted -> outputView.printErrorMessage(Message.errorInvalidLotto()) |
There was a problem hiding this comment.
커스텀 예외를 만들었지만 결국 동일한 출력이 발생하는 거라면 오버엔지니어링으로 보이네요!
이럴거면 굳이 타입을 만들 필요가 있었을까요?
runCatching {
val lotto = Lotto.valueOf(...)
}.getOrElse { ... }| val result = | ||
| WinningLotto.create( | ||
| inputView.getWinningLotto().mapNotNull { LottoNumber.createOrNull(it) }, | ||
| LottoNumber.createOrNull(inputView.getBonusNumber()) ?: return getWinningLotto(), | ||
| ) |
There was a problem hiding this comment.
코드 깊이가 너무 들어갔어요.
요구사항에 있듯이 코드 indent가 깊어지지 않도록 정리해보면 좋을 것 같아요
| is WinningLottoCreationResult.Failure.NumberSizeError -> outputView.printErrorMessage(Message.errorInvalidWinningNumbers()) | ||
| is WinningLottoCreationResult.Failure.BonusNumberDuplicated -> outputView.printErrorMessage(Message.errorInvalidBonusNumber()) | ||
| is WinningLottoCreationResult.Failure.DuplicatedNumbers -> outputView.printErrorMessage(Message.errorInvalidWinningNumbers()) |
There was a problem hiding this comment.
또한 출력을 위한 메세지 오브젝트를 만들었는데, 이 오브젝트 내의 값을 컨트롤러가 직접 넣어서 뷰에게 보내는게 괜찮을까요?
왜 괜찮다고 생각하는지 궁금합니다.
Result 타입을 그대로 OutputView로 전달하면 안되는건가요?
There was a problem hiding this comment.
result 타입을 그대로 뷰에 전달하면 뷰가 너무 도메인에 대한 세부 정보를 알게 되는 것이 아닌가..해서 이렇게 작성했습니다!
| private fun sortResultsByOriginalRankOrder(ranks: List<Rank>): List<Pair<Rank, Int>> = | ||
| Rank.entries | ||
| .filter { it != Rank.MISS } | ||
| .map { rank -> rank to ranks.count { it == rank } } | ||
| .reversed() |
There was a problem hiding this comment.
당첨 로또가 몇개인지 계산하는 비즈니스 로직이 왜 컨트롤러에 있을까요?
알고리즘적으로 더 좋은 방법은 없을까요? 지금은 리스트를 몇번 순회하고 있는지 찾아보세요
There was a problem hiding this comment.
33d22ea
전의 코드에서는 Rank를 순회하면서 ranks 순회하면서 매번 맞는 값을 찾는 식이었는데, 새로 고친 코드에서는 ranks 한번만 순회해서 맞는 키값에 1을 더하는 식으로 바꾸었습니다!
| class LottoController( | ||
| val inputView: InputView, | ||
| val outputView: OutputView, | ||
| ) { |
There was a problem hiding this comment.
feat : 입출력 기능 추가
이게 어떻게 봐야 입출력 기능추가인가요..?
There was a problem hiding this comment.
입출력 기능을 실행시키기 위한 컨트롤러도 포함되는 줄 알았습니다. 앞으로는 컨트롤러는 따로 커밋하도록 하겠습니다!
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
객체지향적 사고와 MVC 패턴이 잘 지켜졌는지를 우선시 해서 확인해주세요!
코드 리뷰 커뮤니케이션
📌 GitHub에서 PR에 댓글 남기는 방법
참고 자료
스크린샷