-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[로또] 김채원 미션 제출합니다. #81
base: main
Are you sure you want to change the base?
Conversation
class LottoDraw { | ||
fun run(inputView: InputView, outputView: OutputView) { | ||
val price = inputView.getPrice() | ||
|
||
val store = Store() | ||
val lottos = store.buyLotto(price) | ||
|
||
val myLotto = inputView.getMyLotto() | ||
val myBonus = inputView.getBonusNumber(myLotto) | ||
|
||
outputView.purchasedMessage(store.numberOfLottoPurchased) | ||
outputView.purchasedLotto(lottos) | ||
|
||
val ranking = LottoResultChecker.checkWinningStatus(lottos, myLotto, myBonus) | ||
val profitRatio = LottoResultChecker.calculateProfitRate(price, ranking) | ||
outputView.resultStatistics(ranking, profitRatio) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨트롤러임을 명확히 표현하기 위해 class 이름에 Controller를 붙히는것도 좋아 보이네요 !
요 함수가 전체적인 비즈니스 흐름을 컨트롤 한다고 예상되는데
각 플로우마다 별도의 함수로 묶어서 이 함수에서 모두 실행시키는 방법은 어떨까요 ?
문제 요구사항에 함수의 라인이 15라인 이내로 제한하는데 14라인이면 살짝 위태 위태 한 것 같아요 ㅎㅎ
기능별로 별도의 함수로 만드는게 좋을 것 같아요 !
val ranking = MutableList(6) { 0 } | ||
|
||
for (lotto in lottos) { | ||
val (duplicateCount, isBonus) = compareLotto(lotto, myLotto, myBonus) | ||
when (duplicateCount) { | ||
3 -> ranking[5]++ | ||
4 -> ranking[4]++ | ||
5 -> if (isBonus) ranking[2]++ else ranking[3]++ | ||
6 -> ranking[1]++ | ||
} | ||
} | ||
return ranking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 있는 넘버들이 각각의 어떤 의미인지 상수나 enum class로 표현하면 좋을 것 같아요 !
fun checkPrice(input: String, price: Int?) { | ||
require(input != "") { ErrorMessages.NULL_PRICE } | ||
require(price != null) { ErrorMessages.NOT_INT } | ||
require(price >= 1000) { ErrorMessages.MINIMUM_PRICE } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
문제 요구사항에 천원 단위로 입력받아야 했던 것으로 기억하는데
천원 단위가 아닐 때에 대한 에러 처리가 되었을까요 ? 요기만 봐서는 구현이 안된 것 같다고 생각이 들어서요 !
정수형을 입력 받을 때 Int로 표현 가능한 범위가 초과되면 NumberFormatException이 발생합니다
요것두 체크해보시면 좋을 것 같습니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 값의 범위는 생각지 못했네요 알려주셔서 감사합니다 ㅎㅎ
제가 생각했을 땐 어차피 1200원을 내도 한개밖에 못사는데 유하게 처리하자! 라고 생각해서 이렇게 했는데 다음부터는 기능 요구사항을 꼼꼼히 읽어봐야겠네요!
require(numbers.size == 6) { ErrorMessages.INVALID_NUMBER_SIZE } | ||
require(numbers.distinct().size == numbers.size) { ErrorMessages.DUPLICATE_NUMBERS } | ||
require(numbers.all { it in 1..45 }) { ErrorMessages.OUT_OF_RANGE } | ||
require(numbers == numbers.sorted()) { ErrorMessages.NOT_SORTED } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기두 각각의 숫자가 의미하는바를 명확히 표현해 주시면 좋을 것 같습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구현에서 열심히 하신 느낌이 많이 듭니다 고생 많으셨습니다!
|
||
import lotto.model.Lotto | ||
import lotto.model.Store | ||
import lotto.util.ErrorMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotto랑 ErrorMessage를 사용하지 않는데 import가 남아있습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 미처 삭제하지 못했네요 알려주셔서 감사해요!
} | ||
|
||
fun getMyLotto(): Lotto { | ||
println(GET_MY_LOTTO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당첨 번호를 MY_LOTTO로 네이밍 하신 이유가 무엇일까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 사실 이거는 제가 로또 시스템을 잘 이해하지 못해서 그런건데
정답로또가 로또 매점에서 사는거고
입력받는 로또가 제꺼 로또로 혼동해서 my_lotto로 했었는데요
문제의 의미를 파악했으면 네이밍을 더 잘했을텐데 아쉽네요!
import lotto.model.Lotto | ||
|
||
object InputValidate { | ||
fun checkPrice(input: String, price: Int?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 오브젝트가 input에 관련한 예외처리라서 input만 받아서 해당 함수에서 toIntOrNull()을 사용해도 될것 같은데 input과 price를 둘다 매개변수로 받는 이유가 궁금합니다!
validate인데 parser의 역할을 한다고 생각해서 분리하신거면 parser를 따로 만드는것도 좋을것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate인데 여기서 분리하는것도 굳이 라는 생각이 들어서 그냥 매개변수로 둘 다 들고왔는데 그것도 좋은 생각인 것 같아요!
"${4}개 일치 (50,000원) - ${ranking[4]}개\n" + | ||
"${5}개 일치 (1,500,000원) - ${ranking[3]}개\n" + | ||
"${5}개 일치, 보너스 볼 일치 (30,000,000원) - ${ranking[2]}개\n" + | ||
"${6}개 일치 (2,000,000,000원) - ${ranking[1]}개\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 숫자가 하드코딩 되어 있는데 왜 ${}를 사용 하신지 궁금합니다!
그냥 숫자를 넣어도 될것 같아서 질문 드립니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 이렇게 보니까 간과했던 부분이 하나씩 보이네요,, 하하 지적 감사합니다!
|
||
val lottoNumber = input.split(",").map { it.trim().toIntOrNull() } | ||
val notNullLottoNumber: List<Int> = lottoNumber.filterNotNull().sorted() | ||
require(lottoNumber.all { it != null }) { ErrorMessages.NOT_INT } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toIntOrNull() 구현에서 null 일 경우에 예외로 처리하면 아래 로직을 실행할 필요 없이 예외를 처리하고 다시 실행할 수 있을 것 같습니다!
requireNotNull()이 null이 아닐 경우에 매개변수 결과를 반환해서
코드도 짧아지고 null일 경우에 예외를 바로 호출해서 불필요한 로직은 실행 안 되고 반복할 수 있어서 학습해 보시고 적용해 보시면 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견 감사합니다 :)
|
||
fun getNumbers(): List<Int> { | ||
return numbers | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numbers가 private로 되어있고 함수로 반환하는 이유는 List와 같은 객체는 외부에서 변경 할 수 있는 가능성이 있어서 라고 생각해서 numbers를 다른 방식으로 반환 시키는 형태가 좋을것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번에 피드백 보니까 확실히 그런 것 같더라구요 객체는 객체답게 하는 것을 명심해야겠어요!
테스트의 장점 중 본인이 가장 공감하는 작성 이유를 작성해 본다. | ||
- [x] 처음부터 큰 단위 테스트를 만들지 않는다. → 테스트 주도 개발을 하자 | ||
- [x] 작은 단위 테스트 : 무작위 값이 4 이상이면 자동차가 전진한다. | ||
- [x] 큰 단위 테스트 : 자동차 경주 게임을 시작하여, 사용자가 이름과 진행횟수를 입력하고, 게임을진행한 후 결과를 확인한다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
문서 정리 정말 잘 하신거 같습니다!
제가 많이 부족한 부분이라 보고 참고하려고 하는데 괜찮을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 편하게 참고하세요 피드백 감사합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
늦었지만 리뷰남깁니다!
다른 분들이 남겨주신 리뷰와 비슷한게 많아서 짧게 남겼어요.
메서드를 적절히 분배해서 이해하기 편한 코드였습니다!
남은 미션도 화이팅~!!
var numbers = Randoms.pickUniqueNumbersInRange(1, 45, 6) | ||
while (numbers.distinct().size != numbers.size) { | ||
numbers = Randoms.pickUniqueNumbersInRange(1, 45, 6) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Randoms.pickUniqueNumbersInRange(1, 45, 6)
자체가 1~45에 중복되지 않는 6개의 숫자를 뽑는 함수여서 while
에서 중복체크를 하지않아도 될 것 같습니다!
const val GET_MY_LOTTO = "당첨 번호를 입력해 주세요." | ||
const val GET_BONUS_NUMBER = "보너스 번호를 입력해 주세요." | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 글-입력-체크-반환
이 while
과 try-catch
로 중복되는 것 같아 while
, try-catch
을 입력을 반환하는 함수로 만들어 사용해도 될 것 같네요!
|
||
@Test | ||
fun `run 함수가 `() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 파일에 커밋이 안된 것 같아요
기능 구현 목록
1. 입력
로또 구입 금액을 입력받는다.
당첨 번호를 입력받는다.
보너스 번호를 입력받는다.
사용자가 잘못된 값을 입력할 경우
IllegalArgumentException
을 발생시킨다.Exception
이 아닌IllegalArgumentException
,IllegalStateException
등과 같은 명확한 유형을 처리한다.2. 로또 발행
로또를 발행한다.
로또가 알맞은 형식인지 검사한다.
3. 당첨 내역 저장
당첨 여부를 비교한다.
수익률을 구한다.
4. 출력
프로그래밍 요구 사항 1
프로그래밍 요구 사항 2
indent(인덴트, 들여쓰기) depth를 3이 넘지 않도록 구현한다. 2까지만 허용한다.
함수(또는 메서드)가 한 가지 일만 하도록 최대한 작게 만들어라.
JUnit 5와 AssertJ를 이용하여 정리한 기능 목록이 정상적으로 작동하는지 테스트 코드로 확인한다.
프로그래밍 요구 사항 3
피드백
테스트의 장점 중 본인이 가장 공감하는 작성 이유를 작성해 본다.