Conversation
chore: test클래스 패키지 이동, 코드 컨벤션
Refactor: 당첨 통계 역할 이동 (LottoMachine -> LottoCalculator)
Refactor: 로또 번호 조힙 List<Int> -> NumberCombination 변경
Refactor: 구매 금액 반환값 String -> Int변경
# Conflicts: # README.md # src/main/kotlin/lotto/data/LottoRanking.kt # src/main/kotlin/lotto/data/WinningLotto.kt # src/main/kotlin/lotto/domain/LottoCalculator.kt # src/main/kotlin/lotto/domain/LottoMachine.kt # src/main/kotlin/lotto/service/LottoGame.kt # src/test/kotlin/lotto/data/WinningLottoTest.kt # src/test/kotlin/lotto/domain/LottoCalculatorTest.kt
catsbi
left a comment
catsbi
left a comment
There was a problem hiding this comment.
영재님 지금 테스트가 실패하는게 있네요.
(구매 로또와 당첨 로또를 받았다면, 당첨 확인을 요청했을 때, 당첨 등수를 반환한다)
그 외엔 크게 문제될 부분은 없고 몇 가지 구조적인 피드백남겨드렸어요. 고민해보시면 좋을 것 같습니다. :😄
| val numberCombinationList = InputView.inputNumberCombination(manualGameTimes) | ||
|
|
||
| val winningNumberList = InputView.inputWinningNumber() | ||
| val purchaseLottoList = lottoGame.buyLotto(gameTimes, numberCombinationList) |
There was a problem hiding this comment.
좀 더 심화로 넘어간 키워드를 드리자면, 현재 Main은 컨트롤러와 UI가 섞여있다고 볼 수 있습니다.
그러다보니 설계를 하다보면 뭔가 섞이는 것 같다는 애매한 느낌을 받을 수 있습니다.
(저는 이 부분에서 많은 고민을 했었죠. )
웹서비스를 보면 프론트 영역에서 ajax로 요청해야하는 부분도 컨트롤러에 있는 것 같고, 응용SW로 보면 CLI의 영역을 컨트롤러가 같이 가지고 있는 것으로 보이죠.
그래서 이를 한번 더 분리해서 사용자와 입출력을 담당하는 cli계층과 말 그대로 하나의 요청에 대한 응답을 해주는 컨트롤러 영역을 분리해보는것도 좋은 경험이 될 것 같아요,.
|
|
||
| private fun determineRankingWithBonusNumber(isContainBonusNumber: Boolean): LottoRanking { | ||
| return if (isContainBonusNumber) { | ||
| return if (matchingNumberCnt == SecondPlace.matchingNumberCnt && hasBonusNumber) { |
There was a problem hiding this comment.
이 부분이 많이 헷갈리시는것 같아요. 이쪽에서 제가 드렸던 피드백의 핵심은 상위 객체(LottoRanking)는 하위 객체가 무엇으로 결정되는지에 대해서 상세 조건을 알고싶어하지 않아야 한다는거죠.
지금은 그냥 매치카운트와 보너스번호 딱 두가지의 조건뿐이기에 한줄로도 정리가 되고 if-else로도 정리가 됩니다.
하지만 조건들이 수십개 이상이 된다면 이런 로직이 유지될 수 없겠죠.
반면, 상위객체에선 전달받은 객체만 하위 객체에 전달해서 맞는지에 대한 유무만 판단하도록한다면 로직이 추가되던 값이 바뀌던 뭐가 어떻게 되던 로직이 바뀔일이 없겠죠.
return values().find { it.support(matchingNumberCnt, hasBonusNumber) } ?: NONE아 그리고 열거타입의 네이밍 컨벤션은 대문자 스네이크 케이스입니다 ㅎㅎ
| @@ -0,0 +1,3 @@ | |||
| package lotto.data | |||
|
|
|||
| class NumberCombination(val numberCombination: List<Int>) | |||
There was a problem hiding this comment.
이건 오버 엔지니어링으로 보이네요 🤔
이 객체가 존재해야 할 이유가 명확하지 않은 것 같아요 ㅠㅠ
|
|
||
| private fun validateDuplicationBonusNumber() { | ||
| require(!lotto.selectNumbers.contains(bonusNumber)) { "당첨 번호 구성과 보너스 번호가 중복됩니다." } | ||
| require(!lotto.selectNumbers.contains(bonusNumber)) { ERR_MSG_DUPLICATION_BONUS_NUMBER } |
There was a problem hiding this comment.
좀 더 객체 지향적으로 설계가 되었다면 다음과 같이 작성될 수 있을꺼에요.
| require(!lotto.selectNumbers.contains(bonusNumber)) { ERR_MSG_DUPLICATION_BONUS_NUMBER } | |
| require(!lotto.containNumber(bonusNumber)) { ERR_MSG_DUPLICATION_BONUS_NUMBER } |
| import java.util.EnumMap | ||
|
|
||
| class LottoGame(private val randomLogic: RandomLogicInterface) { | ||
| class LottoGame(private val randomLogic: RandomLogicInterface, private val gameCost: Int = DEFAULT_COST) { |
There was a problem hiding this comment.
private val gameCost: Int = DEFAULT_COST
해당 값은 stateful한 설계로 보이는데 서비스는 stateless를 지향해서 적절한 선언은 아닙니다 ㅎ
2 participants
리뷰 주신 것들 최대한 적용하고 수동 기능 구현했습니다.
이전 리뷰 중 정적 팩토리 함수 관련한 리뷰은 방법을 찾지 못했습니다.
수동 번호들의 모음(?) List<List>를 방지하기 위해 NumberCombination을 추가하였습니다.
수동과 자동 로또를 생성하기 위해 LottoGame의 buyLotto를 수정하였습니다.
게임 가격은 LottoGame이 가지고 있어야 한다고 생각하여 디폴트 파라미터로 추가하였습니다.
한솔님의 꼼꼼한 리뷰 덕분에 코딩을 하며 많은 생각을 할 수 있었습니다. 감사합니다.