Step 2 - Lotto (Auto) #1147
Step 2 - Lotto (Auto) #1147
Conversation
There was a problem hiding this comment.
Hello again, @arkadiishyndhse !
Great job on this step, too! 👏
I've left some comments suggesting changes to the application's structure.
It would be best to resolve them before the next step.
Your feedback is more than welcome!
|
|
||
| fun main() { | ||
| LottoController.run() | ||
| } No newline at end of file |
There was a problem hiding this comment.
Please activate the configuration to prevent EOF alerts on GitHub. 🙂
Editor -> General -> Ensure every saved file ends with a line break
There was a problem hiding this comment.
This is a great practice to help you set your standard for the test scenarios. 👍
It's best to include the requirements you listed into your commit messages and test scenarios.
| class LottoController { | ||
|
|
||
| companion object { |
There was a problem hiding this comment.
Class with only one companion object could be considered to be declared as object.
(Also for the InputView, ResultView)
| @@ -0,0 +1,30 @@ | |||
| package lotto.model | |||
|
|
|||
| const val MINIMUM_MATCHES = 3 | |||
There was a problem hiding this comment.
How about moving MINIMUM_MATCHES into the companion object of LottoResult
https://kotlinlang.org/docs/object-declarations.html#companion-objects
Note: class members can access private members of their corresponding companion object. So MINIMUM_MATCHES does not have to be public in this case. 🙂
| class LottoResult(private val winningNumbers: List<Int>, private val tickets: List<TicketModel>) { | ||
| fun calculateResult(): Map<Int, Int> { | ||
| return tickets.groupingBy { ticket -> | ||
| ticket.numbers.count { it in winningNumbers } | ||
| } | ||
| .eachCount() | ||
| .filterKeys { it >= MINIMUM_MATCHES } | ||
| } |
There was a problem hiding this comment.
If we work on this Lotto Application with other developers, will they be able to understand what Map<Int, Int> means?
| fun totalPrize(): Int{ | ||
| val prizeMap = mapOf( | ||
| 3 to 5_000, | ||
| 4 to 50_000, | ||
| 5 to 1_500_000, | ||
| 6 to 2_000_000_000 | ||
| ) |
There was a problem hiding this comment.
It's not necessary to create Map instance every time the totalPrize() is called!
How about refactoring them using enum classes?
https://kotlinlang.org/docs/enum-classes.html
| fun totalPrize(): Int{ | ||
| val prizeMap = mapOf( | ||
| 3 to 5_000, | ||
| 4 to 50_000, | ||
| 5 to 1_500_000, | ||
| 6 to 2_000_000_000 | ||
| ) |
There was a problem hiding this comment.
Many functions, including totalPrize(), are only referenced in test packages, so they do not need to be opened as public functions.
Private functions separated only for readability reasons can also be considered verifiable by the public. Since the public function uses the private function, it can be naturally included in the test range.
However, if they are needed more than just improving readability, you might want to think about whether it's time to create another object that performs that role to the testable implementation.
| class PurchasedTickets(private val tickets: List<TicketModel>) { | ||
| fun getTickets(): List<TicketModel> = tickets |
There was a problem hiding this comment.
In Kotlin, the getter is already included in the property! So we don't need to declare a function that just returns property value 🙂
| class PurchasedTickets(private val tickets: List<TicketModel>) { | |
| fun getTickets(): List<TicketModel> = tickets | |
| class PurchasedTickets(val tickets: List<TicketModel>) { |
| val prizeMap = mapOf( | ||
| 3 to 5_000L, | ||
| 4 to 50_000L, | ||
| 5 to 1_500_000L, | ||
| 6 to 2_000_000_000L | ||
| ) | ||
|
|
||
| prizeMap.forEach { (matchCount, prize) -> | ||
| val count = stats.getOrDefault(matchCount, 0) | ||
| println("$matchCount Matches ($prize KRW) - $count tickets") | ||
| } |
There was a problem hiding this comment.
Since this prizeMap is already in LottoResult, how can we improve code duplicates? 🤔
| import org.junit.jupiter.api.assertThrows | ||
|
|
||
| class PurchasedTicketsTest { | ||
| private val testGenerator = TestTicketNumberGenerator(listOf(1, 2, 3, 4, 5, 6)) |
There was a problem hiding this comment.
I see you design your code to be testable 👍
This is also called the Strategy Pattern.
https://refactoring.guru/design-patterns/strategy
Strategy is a behavioral design pattern that lets you define a family of algorithms, put each of them into a separate class, and make their objects interchangeable.
There was a problem hiding this comment.
(Sorry, I intended to request changes 😅 )
Hello again, @arkadiishyndhse !
Great job on this step, too! 👏
I've left some comments suggesting changes to the application's structure.
It would be best to resolve them before the next step.
Your feedback is more than welcome!
2 participants
No description provided.