-
Notifications
You must be signed in to change notification settings - Fork 313
Step 3 - Blackjack (Dealer) #835
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
base: krrong
Are you sure you want to change the base?
Conversation
byjo
left a comment
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.
Hi Seokjin, Great work on this step as well!
I've added a few comments for improvement.
Try refactoring your code using those suggestions and the two programming requirements below:
- Implement all features using Test-Driven Development (TDD), with unit tests for all logic except UI
- Maintain indentation depth of 1 or less—do not nest more than two levels.
Hope you wrap up this step smoothly and also enjoy your holiday!
| import Hand | ||
| import card.PlayingCard | ||
|
|
||
| interface State { |
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.
You've finally applied the state pattern to this mission! 👍👍
Great to see you reach your goal!
src/main/kotlin/Casino.kt
Outdated
| if (!response || player.hand.isBust()) return | ||
| player.drawCard(deck.drawCard(1).first()) | ||
| outputView.printPlayerCards(player) | ||
| when (participant) { |
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.
Would it be possible to eliminate this conditional by using polymorphism?
src/main/kotlin/Casino.kt
Outdated
|
|
||
| val winningResult = WinningResult() | ||
| participants.forEach { | ||
| when (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.
How do you think this conditional could be refactored?
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.
I think it's also possible by using polymorphism like above!
|
|
||
| class Dealer(name: String = "Dealer", override var state: State) : Participant(name) { | ||
| override fun showCardFirst(): List<PlayingCard> { | ||
| return listOf(state.hand.cards.first()) |
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.
This line reaches through multiple objects, which slightly violates the Law of Demeter.
You might consider exposing a function to encapsulate this access and make the intention clearer!
This is similar to what I mentioned in a previous comment: #833 (comment)
src/main/kotlin/WinningResult.kt
Outdated
| import state.Bust | ||
| import state.Stay | ||
|
|
||
| class WinningResult { |
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.
Great job extracting the game result logic into WinningResult! 👍
However, I noticed there's no test coverage for this logic yet.
It would be even better to add tests to make sure this logic is well covered.
Hello Brie, I hope you are doing well.
Came back with third step of blackjack and feel free to add some comments!
Actually, I didn't have much time, so I couldn't give it a lot of thought, and it might be a bit disorganized.
I'm looking forward your feedback.
Thanks and have a nice day 😆