Skip to content

Conversation

@oyj7677
Copy link

@oyj7677 oyj7677 commented Nov 18, 2023

랜덤 로직을 인터페이스화 하였습니다.
랜덤 로직 구현체를 main에서 LottoGame()생성시 파라미터로 전달해 주는 구조입니다.
LottoGame에서는 LottoNumber의 함수에 전달합니다.
이러한 과정이 클래스들을 너무 의존적이게 만들지 않나 생각이 들었습니다.

램덤 로직을 인터페이스화 하면서 생긴 문제점을 개선하고싶습니다.

당첨을 판단하는 건 winningDomain을 생성하여 역할을 이동 시켰습니다.

로또 번호의 정렬을 자료구조로 해결하기 위해 TreeSet을 유지하였습니다.

Copy link

@catsbi catsbi left a comment

Choose a reason for hiding this comment

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

영재님 어느덧 3단계 미션도 잘 만들어주셨습니다 👍
몇가지 피드백 남겨드렸는데, 전체적인 구조에는 문제가 없는 것으로 판단되고 확장성도 같이 고민해보면서 진행하면 좋을 것 같아. 바로 머지드릴테니 피드백은 다음 단계에서 같이 고민해보는걸로 하시죠.!

}

fun checkLotto(purchaseLotto: Lotto, winningLotto: WinningLotto): LottoRanking {
val intersectNumber = winningLotto.lotto.selectNumbers.intersect(purchaseLotto.selectNumbers)
Copy link

Choose a reason for hiding this comment

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

intersect라는 함수에 도달하기까지 a.b.c.d 이렇게 4번의 그래프 탐색이 이뤄지는데요.
이를 줄여볼 수 없을까요? 로또머신이 winningLotto의 내부의 내부의 내부까지 찾아가야하는 상황은 유지보수성에 유리하지 않을 것 같아요,.

Copy link
Author

Choose a reason for hiding this comment

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

WiningLotto로 intersect의 동작을 옮겼습니다. 로또머신에서는 구매한 로또 정보만 넘겨주만 하면됩니다.
당첨번호화의 일치 로직이 변경되거나 로또 WinningLotto의 구조에 변경이 있더라도 로또 머신에 대한 수정이 이루어질 가능성이 낮아질 것 같습니다.

Comment on lines +29 to +37
return when (intersectNumber.size) {
FirstPlace.matchingNumberCnt -> FirstPlace
SecondPlace.matchingNumberCnt, ThirdPlace.matchingNumberCnt -> {
checkSecondPlace(purchaseLotto, winningLotto.bonusNumber)
}
FourthPlace.matchingNumberCnt -> FourthPlace
FifthPlace.matchingNumberCnt -> FifthPlace
else -> None
}
Copy link

Choose a reason for hiding this comment

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

열거타입을 좀 더 활용해봐도 좋을 것 같아요.

Suggested change
return when (intersectNumber.size) {
FirstPlace.matchingNumberCnt -> FirstPlace
SecondPlace.matchingNumberCnt, ThirdPlace.matchingNumberCnt -> {
checkSecondPlace(purchaseLotto, winningLotto.bonusNumber)
}
FourthPlace.matchingNumberCnt -> FourthPlace
FifthPlace.matchingNumberCnt -> FifthPlace
else -> None
}
return LottoRanking.findByMatchCnt(intersectNumber.size)

Copy link
Author

Choose a reason for hiding this comment

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

WinningLotto에서 일치하는 번호 개수와 보너스 번호 일치 여부에 대한 값을 LottoRanking에 생성된 함수(findLottoRanking)에 넘겨주는 방식으로 수정하였습니다.
아래 리뷰의 보너스 번호에 따른 반환도 LottoRanking에서 처리하게 구성하였습니다.

Comment on lines +46 to +52
private fun checkSecondPlace(purchaseLotto: Lotto, bonusLottoNumber: LottoNumber): LottoRanking {
return if (purchaseLotto.selectNumbers.contains(bonusLottoNumber)) {
SecondPlace
} else {
ThirdPlace
}
}
Copy link

Choose a reason for hiding this comment

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

보너스 넘버에따라 무엇을 반환할지를 서비스 계층에서 직접 판단할 필요가 있을까 고민해보면 좋을 것 같습니다.

Comment on lines +40 to +44
fun createWinningRate(cash: Int, winningStatus: Map<LottoRanking, Int>): Float {
val totalPrice = createTotalWinningPrice(winningStatus)

return totalPrice / cash.toFloat()
}
Copy link

Choose a reason for hiding this comment

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

전략 패턴(strategy pattern)을 활용한다면 다음과 같이 작성할 수 있을 것 같네요.

Suggested change
fun createWinningRate(cash: Int, winningStatus: Map<LottoRanking, Int>): Float {
val totalPrice = createTotalWinningPrice(winningStatus)
return totalPrice / cash.toFloat()
}
fun createWinningRate(cash: Int, winningStatus: Map<LottoRanking, Int>): Float {
return winningRateCalculator.calculate(info);
}

이렇게 작성할 경우 나중에 기획에 따라 나라별로 이율 계산 방식니 달라진다고 해도 쉽게 변경할 수 있어요.

    fun createWinningRate(cash: Int, winningStatus: Map<LottoRanking, Int>): Float {
        return winningRateCalculators.calculate(info= info, country: getCountry());
    }

Copy link
Author

Choose a reason for hiding this comment

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

object LottoCalculator를 생성하여 처리하였습니다.

저는 설계 시 '당첨 번호와 당첨 통계는 로또 머신에서 동작해야된다'라고 생각을 했었습니다.
로또 계산기가 추가됨에 따라 LottoMain에서 로또 머신을 거치지 않고 바로 비율을 구할 수 있게 되었습니다.
현 상황에서 로또 머신을 거쳐서 통계를 가져온 다는게 불필요한 과정을 거친다고 생각을 할 수 있습니다.

질문은 로또 머신을 거치는 불필요한 과정을 제거해도 구조에 문제가 없는가? 입니다.

Copy link

Choose a reason for hiding this comment

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

당첨 번호와 통계에 대한 분석을 로또 머신에서 할 수 있긴 합니다.
그런데 전략패턴을 사용한다고 로또 머신에서 통계등의 작업을 하지 않는게 아니에요.

자동차를 예로 들어볼때 자동차안에서 핸들을 움직인다고 하고 엑셀을 밟고, 브레이크를 밟을때 자동차는 앞/뒤/좌/우로 움직이는데, 실질적으로 책임지는건 핸들이 아니라 핸들부터 각각의 모든 부품들이 각자 위치에서 자기 책임을 다 했을때 최종적으로 우기아 의도한대로 움직이게 되는 것이죠.

그리고 로또 머신이 수익율 계산을 하는게 맞는가? 고민을 하신다면 로또 머신의 Actor에 대해 고민해보면 좋을 것 같아요. 로또 머신이 로또를 발급하고 비교하고 수익율을 계산하는 서비스 계층이라면 적절한 책임이고, 그게 아니라면 과도한 책임이겠죠

Comment on lines +13 to +23
fun buyLotto(cash: Int): List<Lotto> {
val lottoList = mutableListOf<Lotto>()
val times = cash / GAME_COST

repeat(times) {
val lottoNumberCombination = LottoNumber.createRandomLottoNumber(randomLogic)
lottoList.add(LottoMachine.createSelectLotto(lottoNumberCombination))
}

return lottoList
}
Copy link

Choose a reason for hiding this comment

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

이펙티브 시리즈 책의 내용중에는 추상화의 레벨을 맞춰라 라는 말이 있는데요.
이는 다른 함수를 호출해서 결과를 만드는 것과 직접 로직을 작성하는게 혼용되지 말라는 의미를 가집니다.

그리고 도메인에 책임을 위임하는게 아닌 직접 연산이 필요하거나 개념적 분리가 필요한 경우 함수로 분리를 하고는 하는데, 이 경우 이 분리의 수준을 맞추면 좋을 것 같아요.

Suggested change
fun buyLotto(cash: Int): List<Lotto> {
val lottoList = mutableListOf<Lotto>()
val times = cash / GAME_COST
repeat(times) {
val lottoNumberCombination = LottoNumber.createRandomLottoNumber(randomLogic)
lottoList.add(LottoMachine.createSelectLotto(lottoNumberCombination))
}
return lottoList
}
fun buyLotto(cash: Int): List<Lotto> {
val times = getTimes(cash)
return createLotto(times)
}
fun getTimes(cash: Int) {
return cash / GAME_COST // 여기도 전략패턴 반영 가능 하겠네요.
}
fun createLotto(times: Int) {
// ...
}

Copy link
Author

Choose a reason for hiding this comment

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

로또 계산기의 역할이라 생각하여 getTimes를 LottoCalculator에서 구현하였습니다.
getTimes의 파라미터로 enum Country(naem: String, gameCost: Int)와 같은 구성의 클래스가 오면 자연스럽게 나라별 게임 횟수를 구할 수 있을 것 같습니다.

@@ -0,0 +1,3 @@
package lotto.data

data class WinningLotto(val lotto: Lotto, val bonusNumber: LottoNumber)
Copy link

Choose a reason for hiding this comment

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

보너스 번호가 로또번호에 중복되는게 있는 경우에 대한 검증도 필요할 것 같네요.

Copy link
Author

Choose a reason for hiding this comment

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

WinningLotto의 init에 검증 로직 추가하였습니다.

@catsbi catsbi merged commit bb1bb09 into next-step:oyj7677 Nov 19, 2023
@oyj7677 oyj7677 deleted the step3 branch November 20, 2023 11:44
@oyj7677 oyj7677 mentioned this pull request Nov 20, 2023
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.

2 participants