로또 미션 피드백 : [우아한테크코스 5기 AN_베르]
리뷰 1
Rank 의 convertToGrade 메서드는 matchCount 와 bonusMatch 를 입력받아 해당하는 Rank 를 반환한다.
해당 코드에 대한 리뷰
현재 요구사항에서 2, 3등에서만 bonusMatch 가 필요하다. 하지만 요구사항이 추가되어 bonusMatch 가 다른 등수에서도 필요해 진다면 위의 로직에서 if 문이 계속 추가되거나 when 을 사용하여 조건을 추가해 줘야한다.
그렇다면 해당 메서드 로직은 변경하지 않으면서 다른 등수를 추가할 수 있게 만드는 방법은 무엇일까?
해결 방법
내가 사용한 방법은 enum class 추가이다.
각 등수는 bonus 에 대해 3 가지 분기를 갖는다.
1. bonus 가 맞아야한다. (BONUS_MATCH)
2. bonus 가 맞지 않아야 한다. (BONUS_MISMATCH)
3. 상관없다. (ANY)
따라서 BonusResult 라는 enum class 를 추가하였다.
enum class Rank(val matchCount: Int, val bonusResult: BonusResult, val prizeMoney: Int) {
NOTHING(-1, ANY, 0),
FIFTH(3, ANY, 5_000),
FOURTH(4, ANY, 50_000),
THIRD(5, BONUS_MISMATCH, 1_500_000),
SECOND(5, BONUS_MATCH, 30_000_000),
FIRST(6, ANY, 2_000_000_000),
fun convertToGrade(matchCount: Int, bonusResult: BonusResult) = Rank.values()
.find { rank -> rank.matchCount == matchCount && (rank.bonusResult == bonusResult || rank.bonusResult == ANY) }
?: NOTHING
matchCount 가 같을 때 bonusResult 가 ANY 이면 반환
그렇지 않은 경우엔 bonusResult가 일치해야한다.
요구사항이 추가되거나 변경된다면 Rank 의 값만 변경해 주면 가능하다. 위와 같이 한 줄로 일관적인 로직이 가능하다.
리뷰 2
내가 남긴 질문
LottoFactory 의 createLotto function 은 인자를 받으면 수동으로 만들고 인자를 넘겨주지 않으면 랜덤으로 생성한다.
해당 코드에 대한 리뷰
외부(LottoStore)에서는 LottoFactory 를 뜯어보지 않고 알 수 있어야 한다.
따라서 다음과 같이 refactoring 했다.
class LottoFactory {
//수동 로또 생성
fun createManualLotto(numbers: List<Int>): Lotto = createLotto(numbers)
//자동 로또 생성
fun createAutoLotto(): Lotto = createLotto()
//로또를 생성
private fun createLotto(numbers: List<Int> = getRandomNumbers()): Lotto = Lotto(numbers)
private fun getRandomNumbers(): List<Int> = (LOTTO_MINIMUM_NUMBER..LOTTO_MAXIMUM_NUMBER).shuffled().take(LOTTO_SIZE)
}LottoFactory 외부에서는 createManualLotto 와 createAutoLotto 두 가지로 나누어 호출하도록 한다.
내부에서는 인자를 전달하거나 전달하지 않거나로 Lotto를 생성한다.
즉, 외부에서는 내부 로직을 모르게 하면서 기존에 로직을 사용할 수 있도록 변경하였다.
리뷰 3
생성자 추가 vs 테스트를 위한 Fake constructor
문제 상황
원시값 포장이 많아지면서 Test 코드에서 객체를 생성할 때 가독성이 줄어듬
테스트 코드도 리팩터링해야 하듯이 가능한 양질의 코드를 유지해야 한다.
해결 방법 1
첫 번째로 사용한 방법은 생성자 추가이다.
기존엔 LottoNumber 포장을 LottoFactory 에서 하도록 했었다.
하지만 LottoFactory 의 역할은 Lotto를 생성하는 것이지 value 를 LottoNumber로 포장하지 않아도 된다하는 생각에 위 역할을 Lotto class 로 이동시켰다.
위와 같이 변경하면서 테스트 코드가 한줄로 간결해 지는 것을 확인할 수 있다.
해결 방법 2
WinningLotto 는 Lotto 를 변경한다고 해서 해결되지 않았다. 그렇다고 생성자를 추가하기엔 WinningLotto 의 controller 로직까지 변경해 주어야했다.
그래서 fake constructor 사용했다.
class WinningLottoTest {
fun WinningLotto(mainLottoNumbers: List<Int>, bonusLottoNumber: Int) =
WinningLotto(mainLottoNumbers.map { LottoNumber.from(it) }.toSet(), LottoNumber.from(bonusLottoNumber))
Test 코드에서 fake constructor 을 작성할 때 메서드 이지만 대문자로 시작하는 것을 확인할 수 있다.
( 참고. 코틀린에서 val somthingList = List(3){somthing} function 이다.)
생성자처럼 쓰지만 실제 생성자가 아니고 반복되는 포장의 길어짐을 방지하기 위해 사용했다.
마찬가지로 테스트 코드가 간결해 진다.
해결방법 3
fake constructor 로 생성하는 것이 일반적이지 않을 수 있다는 페어의 피드백과 함께 일반적인 factory 메서드로 변경하였다.
private fun lottoNumberOf(vararg lottoNumbers: Int) =
lottoNumbers.map { LottoNumber.from(it) }
@Test
fun `메인 번호는 6개이다`() {
assertThrows<IllegalArgumentException> {
WinningLotto(
lottoNumberOf(3, 45, 34).toSet(),
LottoNumber.from(36),
)
}
}기존의 생성자는 그대로 사용하면서 간결한 테스트 코드를 유지할 수 있다.
나의 결론
해결 방법 1 처럼 테스트가 가능하도록 생성자를 변경해 주는 것이 더 효율적일 것이다.
그러나 내가 건드릴 수 없는 코드이거나 혹은 테스트를 위해 전체적인 구조를 변경해야 한다면 쉽지 않은 문제다.
그럴 때는 두 가지 방법을 생각할 수 있다.
1. 테스트만을 위한 생성자를 추가해 주는 것
(해당 생성자를 로직에서 사용하지 않는 경우이므로 앞의 해결 방법 1과는 다른 얘기이다.)
생성자를 추가하면서 테스트 코드가 확연히 줄어든다면 생성자를 추가할 가치가 있다고 생각한다. 다만 테스트를 위한 코드는 지양하는 것이 좋다.
2. fake constructor 사용
생성자를 추가하지 않으면서 테스트만을 위해 가짜 생성자를 사용하면서 가독성이 좋아질 수 있다. 따라서 테스트만을 위한 생성자를 추가하는 것보다 좋은 방법이라 생각한다.
3. factory method 사용
기존의 생성자를 그대로 사용하면서도 인자로 필요한 값을 만들어 줄 수 있으므로 간결한 테스트 코드를 작성할 수 있다.