Closed aksnell closed 4 years ago
https://github.com/aksnell/sdg/tree/master/lessons/four/blackjack Simple, minimal, viable, etc.
Hi Alex, I'll check this out in detail later and provide code feedback.
You are creating all of these projects in a single repository. Generally, we don't recommend you do this.
If you do wish to continue in this mode I would ask that you ensure that you are at least using the recommended sdg-console
template since it will generate a proper .gitignore
for each project. You can say "No" to create a git repository for each project and it will still create a proper .gitignore
. Your handcrafted .gitignore
in your home directory is not properly ignoring binary build assets and thus you are committing things into your repository that you should not be. For instance your blackjack has an "obj" and "bin" directory that the sdg-console
generated .gitignore
would have prevented.
Alex! This is very good work! You clearly understand the problem and have done a good job implementing it.
I'm giving you deep feedback on the assignment. Please consider each of these suggestions and consider implementing them into your assignment and see how each of these impact the code.
The top thing that stands out is that I believe you are missing the concept of a Game
class and that omission is revealing some extra implementation and details in your classes that I believe will be simplified by the implementation of this class.
I'd love you to take the time this weekend to reflect on and implement these suggestions. Consider this the type of feedback you are likely to get on a code review on a feature before it is merged into the main codebase. Try revising and resubmitting by letting me know, via comment, you have pushed new code.
Looking at Program.cs
it seems we are missing an abstraction. You are essentially adding a player to a dealer. This seems like the wrong level of abstraction. I believe you are missing a Game
class.
Thus instead of:
Dealer d = new Dealer{};
Player p = new Player{};
d.JoinGame(p);
d.Play();
you would have
Dealer dealer = new Dealer{};
Player player = new Player{};
Game game = new Game();
game.JoinGame(dealer);
game.JoinGame(player);
game.Play();
or
Dealer dealer = new Dealer{};
Player player = new Player{};
Game game = new Game(dealer);
game.JoinGame(player);
game.Play();
This even reads more like what you might expect.
Also, notice I am rejecting single variable names. I will soon stop accepting assignments that use this variable naming style.
No single letter variable names: https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L14
This method name implies that it does work (DisplayCard
) when in fact it returns a representation of a card. So a better name would be Description
or similar: https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L20
Grip
- This is an interesting idea, an intermediate object between the deck and the player. However, I think it has some areas to improve. It does seem like the idea of a Hand
which would be a more common name from the program description.
The constructor takes a deck and keeps a reference. This tightly couples these classes and implementations. You could implement interface and pass that instead, but you are still binding these implementations together. I would propose that the introduction of a Game
class would allow it to be the interaction coordinator between the Deck
and the Player
(or Grip
or Hand
) -- The Game
would hold the Deck
and the various things that receive cards. Thus it could control the interactions. What if we need to extend the program to keep a log of the card's dealt, or there needs to be some other action. In this system the implementation would have to be in the Deck or the Grip. By having a Game we could concentrate the various business/logic rules there.
No single character variable names: https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L54 -- this could be foreach (var card in Cards) {
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L61 - This method name could be improved: Description
or AllCardDescriptions
, etc. We know it is a Get
because it takes no args and returns something, we know it is a Str
since the return type is a string
-- so the method name adds no context and repeats things that the language already implies.
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L71 - local variables should be in camelCase
so handValue
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L89 - as a reader of this code it is not immediately obvious why this loop starts at 1
and ends at 14
and why it is called face
-- as a ready I wondered why face
goes to 14
-- Wouldn't this also add a face of 1
? -- This seems like an optimization that would be better as an explicit list of strings. "2", "3", ... "J", "Q", "K", "A"
-- then you could still have your switch
statement cases.
I've typed the following without checking, but something like this is more intent revealing and any performance difference is negligible unless we are building a lot of decks per second.
for (var face in new [] {"2", "3", "4", "5", "6", "7", "8", "9", "10", "J", "K", "A"})
{
case "J":
Cards.Add(new Card(suit, face, 10));
break;
case "Q":
Cards.Add(new Card(suit, face, 10));
break;
case "K":
Cards.Add(new Card(suit, face, 10));
break;
case "A":
Cards.Add(new Card(suit, face, 11));
break;
default:
Cards.Add(new Card(suit, face, int.Parse(face)));
break;
}
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Deck.cs#L113 - This shuffles the deck each time? That is not necessary, just shuffle it once at creation time. This is another argument for the idea of a Game
class as it would then be in charge of shuffling the deck. It would allow the game to control that logic and tell the deck to shuffle when needed.
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Player.cs#L10 - Indent this line. I think some Visual Studio code setting is not on that should be auto-formatting your code. Check with Jordan or me.
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Player.cs#L19 - this should be in its own file named Dealer
if we are following conventions
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Player.cs#L32 - single-letter variable names.
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Player.cs#L58 and the other "SomethingPlayer" methods likely don't need the word Player
in the method name since it is clear that this involves a Player
by its argument.
https://github.com/aksnell/sdg/blob/master/lessons/four/blackjack/Player.cs#L144 - This logic is creating a deep nesting of the stack. Every time you Reset
you are calling down into Play
which will eventually call Reset
which will call Play
which will (etc.) -- Eventually you will run out of stack. Again this is another reason we should likely have a Game
class that is running some sort of loop that does interactions with the Deck
, Dealer
, and Player
without nesting the prompting/resetting into these classes.
Hi Alex, I'm going to mark this assignment as complete. However, I pose to you a challenge to review my comments above and attempt to implement the ones you see fit.
You clearly have a good intuition and experience with writing code. I'd like to get you to focus on writing in a more clear and more structured way. Take this time to not just focus on language features and structural approaches (e.g. your initial want to use message or function passing) but on improving your style. One of the best things a programmer can do at the beginning of their formal career is to build a good, clean, coding style. I think you can do it, so I'm giving you pointers to that path. Please do take the time here to work on this.
Good work!
Your homework 01 - 04 - Blackjack was marked: Meets Expectations
“Well done!” — via Gavin Stark
In this project, you are creating a playable game of Blackjack. If you haven't played this game ever, or in a while, grab a deck of cards and play a few games.
Objectives
Requirements
Create a single-player blackjack game that plays against the house, i.e., a human player and computer dealer. You are free to create the user interface however you want, but keep it simple for Explorer Mode.
Explorer Mode
General Rules:
[x] The game should be played with a standard deck of playing cards (52).
[x] The house should be dealt two cards, hidden from the player until the house reveals its hand.
[x] The player should be dealt two cards, visible to the player.
[x] The player should have a chance to hit (i.e. be dealt another card) until they decide to stop or they bust (i.e. their total is over 21). At which point they lose regardless of the dealer's hand.
[x] When the player stands, the house reveals its hand and hits (i.e. draw cards) until they have 17 or more.
[x] If dealer goes over 21 the dealer loses.
[x] The player should have two choices: "Hit" and "Stand."
[x] Consider Aces to be worth 11, never 1.
[x] The app should display the winner. For this mode, the winner is who is closer to a blackjack (21) without going over.
[x] There should be an option to play again; this should reset the hands and reshuffle the deck.
Adventure Mode
Epic Mode