-
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
[로또] 임선미 미션 제출합니다. #90
base: main
Are you sure you want to change the base?
Conversation
단위 테스트를 통해 소숫점 반올림 자릿수가 잘못되었음을 발견 -> 양식 수정
예외가 발생할 때 재입력을 받을 수 있는 반복문 함수 callRetriableInput()를 만들어 적용
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.
3주차도 고생 많으셨습니다!
입력 에러확인과 재입력 코드가 깔끔하고 좋아요!
좋은 정보 배워갑니다~
마지막까지 화이팅입니다~!
src/main/kotlin/lotto/Lotto.kt
Outdated
import lotto.utils.ValidationUtils | ||
|
||
class Lotto( | ||
val numbers: List<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.
프로그래밍 요구 사항에서 Lotto class
에 numbers
접근제어자 private
는 변경 불가능이었던 것으로 알고 있습니다.
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 calculateEarningsRate(earningMoney: Long, investMoney: Int): Number { | ||
val earningsRate = if (investMoney == EMPTY_INVEST_MONEY) { | ||
ZERO_EARNINGS | ||
} else { | ||
(earningMoney.toDouble() / investMoney) * RATE_PERCENT | ||
} | ||
return earningsRate | ||
} |
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 calculateEarningsRate(earningMoney: Long, investMoney: Int): Number { | |
val earningsRate = if (investMoney == EMPTY_INVEST_MONEY) { | |
ZERO_EARNINGS | |
} else { | |
(earningMoney.toDouble() / investMoney) * RATE_PERCENT | |
} | |
return earningsRate | |
} | |
fun calculateEarningsRate(earningMoney: Long, investMoney: Int): Number { | |
if (investMoney == EMPTY_INVEST_MONEY) return ZERO_EARNINGS | |
return (earningMoney.toDouble() / investMoney) * RATE_PERCENT | |
} |
프로그래밍 요구 사항에서 else
사용을 지양하고, if 조건절
에서 값을 return
하는 방식을 추천한다고 해서 이런 방식은 어떨지 제안합니다!
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 input = Console.readLine() | ||
return@callRetriableInput input.toPurchaseMoney() | ||
} | ||
} |
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.
라벨환산은 가독성때문에 넣은거고 굳이 "return@callRetriableInput" 명시 안해도 해당 코드에서는 람다의 마지막줄이 리턴 되긴 합니다!
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.
람다함수를 능숙하게 잘 쓰시는 것 같네요. 보면서 많이 배웠습니다. 이번주차 고생하셨어요
return this.toInt() | ||
} | ||
|
||
private const val INPUT_WINNING_NUMBERS_TITLE = "당첨 번호를 입력해 주세요." |
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 input = Console.readLine() | ||
return@callRetriableInput input.toPurchaseMoney() | ||
} | ||
} |
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.
저도 입력값을 다시 받는 부분이 어려웠는데 이런식으로도 간단히 처리할 수 있다는걸 몰랐네요! 배워갑니다
package lotto.utils | ||
|
||
object ValidationUtils { | ||
|
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.
여기서 try-catch 사용하면 에러 던지고 에러 처리한걸로 간주되어서 프로그램이 끝나버리지 않나요? 제가 코드 짤 때는 그랬어서 잘 몰라서 여쭤봅니다.
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.
ValidationUtils을 설계할 때 해당 유틸 클래스의 목적은 유효성 검사에 대한 역할만 할뿐 프로그램이 끝나고 안끝나고는 결정하지 않는다 였습니다. 프로그램 종료와 관련된 컨트롤은 이 클래스의 책임이 아니라 판단했어요.
그래서 프로그램이 예외가 발생해도 종료되지 않고, input을 계속 입력받을 수 있게 하는 것은 Input관련 클래스 역할이라고 생각했고,
해당 클래스에 예외가 발생해도 종료되지 않고 다시 재입력을 받는 확장함수 fun callRetriableInput()을 만들어서 호출하도록
했습니다!
src/test/kotlin/lotto/LottoTest.kt
Outdated
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.
기능테스트와 예외테스트를 나누어서 처리한게 가독성이 더 향상되네요! 좋은 것 같아요
PLACE_1st(2000000000L, "6개 일치 " ), | ||
PLACE_2nd(30000000L, "5개 일치, 보너스 볼 일치 "), | ||
PLACE_3rd(1500000L, "5개 일치 "), | ||
PLACE_4th(50000L, "4개 일치 "), | ||
PLACE_5th(5000L, "3개 일치 "), | ||
PLACE_Fail(0L, ""); |
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.
case type을 일관적으로 지키는 것이 좋을 것 같아요. 현재는 screaming snake case에 소문자가 섞여있습니다.
PLACE_5th(5000L, "3개 일치 "), | ||
PLACE_Fail(0L, ""); | ||
|
||
fun str(): String { |
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.
해당 메소드는 이름으로 어떠한 역할을 하는지 알기 어려운 것 같습니다.
private const val RATE_PERCENT = 100 | ||
private const val EMPTY_INVEST_MONEY = 0 | ||
private const val ZERO_EARNINGS = 0 | ||
fun calculateEarningsRate(earningMoney: Long, investMoney: Int): 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.
해당 메소드의 경우 외부에서 호출이 되지 않으므로 private로 접근 제어자를 변경하는 것이 어떨까요?
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.
해당 함수를 private으로 하지 않은 것은 유틸 함수이기도하고 나중을 생각했을 때 외부에서도 호출할 가능성이 있지 않을까해서 접근 제어자를 명시하지 않았습니다!
require(validMoney <= 1000000) { | ||
"[ERROR] 한번에 구입 가능한 최대 금액은 1,000,000원을 넘길 수 없습니다." | ||
} | ||
} catch (e: Exception) { |
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.
Exception을 catch하게 되면 위의 try 블럭에서 발생한 예외들이 모두 잡히게 되어 상황에 맞는 에러 메시지가 아니라 항상 같은 메시지가 예외에 담기게 됩니다.
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.
너무 좋은 조언입니다! 제가 미쳐 생각하지 못했던 부분이었습니다.
예외 테스트 통과해서 문제 없을 것이라 생각했는데 테스트 방법 자체도 예외가 발생하는지만 검사했더라구요.
덕분에 놓쳤던 많은 부분들을 짚고 넘어갑니다.
어떻게 수정해야 정상적으로 작동하면서 가독성 좋은 코드가 될지 고민해서 반영해볼게요!!
정말 감사합니다(__)
예외 테스트할 때 예외 메시지 확인하는 테스트로 수정 예외 메시지 상수화를 통해 테스트 하기 용이하게 리팩토링
구현 기능 목록
예외 처리
실행 결과