-
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
[로또] 정남진 미션 제출합니다. #106
base: main
Are you sure you want to change the base?
[로또] 정남진 미션 제출합니다. #106
Conversation
프로젝트 개요와 기능 요구 사항, 입출력 요구 사항을 작성
금액 입력 예외 사항과 예외 처리, 수익률에 관란 요구 사항 추가
NoSuchElementException의 경우 실제 프로그램 실행시 일어나지 않고 테스트 코드에서는 해당 예외가 테스트가 성공적으로 종료되기 위해 필요함
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.
제가 이제껏 써보지 않은 Kotlin의 기능들을 쓰셔서 많이 배워갈 수 있었습니다!
4주차도 화이팅입니다 ㅎㅎ
import lotto.domain.model.Lotto.Companion.VALID_LOTTO_NUMBER_LENGTH | ||
import lotto.domain.model.Lotto.Companion.VALID_LOTTO_NUMBER_RANGE | ||
|
||
object ExceptionMessages { |
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.
이 object를 domain 패키지에 넣어둔 이유가 궁금하네요!
뭔가 common이나 util의 성격이 강하다는 생각이 들기도 해서요..!
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.
네 말씀하신 것 처럼 common의 성격이 더 강한 것 같습니다.
원래는 해당 object는 에러 메시지를 담고 있어 성격상 ui 패키지에 넣을까 생각하다가 그렇게 되면 domain에서 ui로 의존성이 생길까 하다가 domain에 넣었던 것 같네요.
val lottoes = lottoFactory.createLottoes(amount) | ||
outputView.displayLottoes(lottoes) | ||
|
||
val winningNumbers = keepCallingWithDefaultOnFailure(inputView::requestWinningNumbers) |
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.
습관적으로 참조를 사용했네요.
Jetpack Compose에서 람다를 생성해서 넘겨주게 되면 recomposition이 필요하지 않은 상황에서도 발생하는 경우가 있어서 습관적으로 Compose에서 사용할 때 처럼 사용했습니다. trailing lambda를 이용하여 호출하여도 괜찮을 거 같습니다.
import lotto.ui.console.ConsoleInputView | ||
import lotto.ui.console.ConsoleOutputView | ||
|
||
class Ui : OutputView by ConsoleOutputView(), InputView by ConsoleInputView() |
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.
원래 해당 클래스에서 입출력을 모두 처리하다가 클래스가 커지는 것 같아서 InputView, OutputView로 나누었습니다. 기존의 main 함수 코드에 영향을 주지 않기 위해서 해당 클래스에 delegation을 이용해서 각각의 인터페이스를 구현하도록 했습니다.
그런데 나중에 프로그램 전반을 나타낼 LottoApp을 생성하게 되면서 해당 클래스가 쓸모가 없어져서 삭제해도 될거 같네요.
@JvmStatic | ||
fun provideParametersForCalculateWinPlace(): Stream<Arguments> = Stream.of( | ||
Arguments.of(listOf(1, 2, 3, 4, 5, 6), 7, LottoWinPlace.FIRST_PLACE), | ||
Arguments.of(listOf(1, 2, 3, 4, 5, 45), 6, LottoWinPlace.SECOND_PLACE), | ||
Arguments.of(listOf(1, 2, 3, 4, 5, 45), 44, LottoWinPlace.THIRD_PLACE), | ||
Arguments.of(listOf(1, 2, 3, 4, 44, 45), 7, LottoWinPlace.FOURTH_PLACE), | ||
Arguments.of(listOf(1, 2, 3, 43, 44, 34), 7, LottoWinPlace.FIFTH_PLACE), | ||
Arguments.of(listOf(1, 2, 42, 43, 44, 45), 7, 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.
test에서 @ParameterizedTest와 함께 이렇게도 사용될 수 있군여.. 새롭게 배워갑니다!
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.VALID_LOTTO_NUMBER_RANGE.first, | ||
Lotto.VALID_LOTTO_NUMBER_RANGE.last, | ||
Lotto.VALID_LOTTO_NUMBER_LENGTH | ||
) |
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.
data 레이어는 원격 및 로컬에 접근, 관리,처리등을 하는 레이어로 알고 있는데,
로컬이나 원격 데이터가 없는데 데이터 레이어에 해당 구현을 하신 이유가 궁금합니다!
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 the UI layer contains UI-related state and UI logic, the data layer contains application data and business logic. The business logic is what gives value to your app—it's made of real-world business rules that determine how application data must be created, stored, and changed.
https://developer.android.com/topic/architecture/data-layer
데이터 레이어의 경우 원격 및 로컬에서 데이터를 가져오는 것 뿐만이 아니라 애플리케이션에서 사용될 데이터에 관해서 담당하는 레이어입니다.
로또 앱의 경우 랜덤한 데이터를 가져와서 로또 생성에 사용하게 되는데 이에 가장 적합한 레이어는 데이터 레이어라 생각하여 데이터 레이어에서 구현하였습니다.
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 interface LottoNumberGenerator { | ||
fun pickLottoNumbers(): 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.
impl이랑 다른 레이어에 있는 이유가 궁금합니다!
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.
랜덤값을 이용하여 로또를 생성하는 것은 로또앱의 비즈니스 로직으로 domain 레이어에 속하지만 랜덤값을 어떻게 가져오는지에 대해서 세부사항에 대해서는 감추어서 랜덤값 라이브러리 변경에 대해서 다른 코드에 영향을 미치지 않게 하기 위해서입니다.
https://blog.cleancoder.com/uncle-bob/2012/08/13/the-clean-architecture.html
위 글의 Crossing boundaries 부분을 읽어보시면 될 거 같습니다.
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 PERCENT = 100 | ||
|
||
fun calculateProfitRate(budget: Int, lottoWinPlaces: Map<LottoWinPlace, Int>): Float { | ||
val totalPrize = lottoWinPlaces.map { (lottoWinPlace, count) -> lottoWinPlace.prize * count }.sum().toFloat() |
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.
sum을 사용한 시점에서 큰 값이 될 것 같습니다!
toFloat()은 소수 계산 때문에만 사용하신 걸까요?
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.
네 소수 계산 때문에 사용했습니다. 그리고 말씀하신 것 처럼 큰 값이 되어 overflow가 발생할 여지에 대해서는 제가 마지막 날에 급하게 구현하느라 놓친점 같네요.
|
||
override fun requestWinningNumbers(): Result<List<Int>> = runCatching { | ||
displayEnterWinningNumbers() | ||
val userInput = readIntListSplitByComma().also { validateWinningNumbers(it) } |
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.
각 레이어를 나누신 이유가 무엇일까요??
레이어 끼리 책임을 분리하고 의존성을 줄이기 위해서라고 저는 알고 있어서,
domain에 validate 함수가 전체 로직에서 제한 없이 접근할 수 있는 부분과
view에서 domain의 validate 함수에 바로 접근해서 사용하는 것이 의문입니다!
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.
domain 레이어의 경우 세개의 레이어 중 가장 높은 레벨의 레이어로 다른 레이어에서 의존성을 가져도 괜찮습니다.
하지만 반대로 domain 레이어가 data 레이어나 ui 레이어에 의존성을 가지게 되면 비즈니스 로직의 변경이 아닌 구현 세부 사항에 의해 코드가 변경될 여지가 있으므로 지양해야합니다.
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.
아 이해 했습니다!
domain 레이어의 경우 다른 레이어에서 직접적으로 의존성을 가져도 되지만
domain 레이어가 다른 레이어들을 조회할때는 인터페이스를 통해서 접근해야 하는거군요!
domain 레이어를 따로 안써봤더니 정확히 이해하기가 어려웠네요!
덕분에 이제까지 해본것도 다시 확인해보고 수정 할 수 있을것 같습니다 감사합니다!
return@runCatching userInput | ||
} | ||
|
||
override fun requestWinningNumbers(): Result<List<Int>> = runCatching { |
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.
runCatching으로 예외를 처리하고 내부값을 Result타입으로 받아서 처리하는 방법이 있네요 새로운 구현 알아가네요 ㅎㅎ
|
||
stringBuilder.appendLine(PROFIT_FORMAT.format(profit)) | ||
println(stringBuilder.toString()) | ||
} |
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.
StringBuilder에 모든 내용을 담아서 출력하시는 이유가 무엇일까요??
StringBuilder에 추가 되는 부분들을 다 해석해야 어떤 출력이 나올지 이해가 될꺼 같아서
코드를 이해 하기가 어려울거 같습니다!
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.
반복적인 IO 작업은 성능을 저하시킬 수 있기 때문에 StringBuilder를 이용하여 출력할 문자열을 build하고 한번에 출력시키는 방식으로 하였습니다.
실제로 백준에서 문제를 풀 때 반복적으로 IO를 하게 되면 시간 초과하게 되는 경우가 있습니다.
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.
아 성능을 위해서 이렇게 작성하신 거군요!
백준을 풀어볼 때는 IO를 많이 호출해서보다는 문제를 잘못 풀어서 시간이 초과한다고 생각을 많이 해서 생각을 못 해봤습니다!
생각을 못 해본 부분이었던 것 같습니다. 설명 감사합니다!
No description provided.