Skip to content

Blackjack: Implements Panda, Tiger & Eagle Levels #26

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rastreus
Copy link

@rastreus rastreus commented Mar 4, 2013

I tested to see if I achieved the desired results by "playing" the game using irb.
I was able to change a few of the specs to actually unit test for the Panda Level, but I wasn't sure the best ways to test for the Tiger and Eagle Levels.
It's alright, but I know that there's a more elegant, idiomatic "Ruby Way" to accomplish the same results.
I definitely feel this way for the Eagle Level. I feel like adding conditional statements into #status wasn't the best way to get it done. But it worked. Anyway, if there are any hints or what you would do differently then I would love to hear it.
Thanks.

end

def bust?
if value > 21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should write this method as

def bust?
  value > 21
end

The reason --- if you're doing an "if" that returns true/false, your condition is already what you want

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Thanks for pointing this out. There's no reason for the if. Exactly.

@jwo
Copy link
Member

jwo commented Mar 5, 2013

I left some idiomatic comments on the "ruby way" to organize and name your code.

Overall though, your code was very good, and your tests look nice.

@rastreus
Copy link
Author

rastreus commented Mar 6, 2013

@jwo I made some of the changes that you suggested and just wanted to see what you thought. I don't want to dwell on the first assignment too long, but I wanted to get it to a good level before moving on to the next episode.

@jwo
Copy link
Member

jwo commented Mar 6, 2013

Looks as good as I've seen for the blackjack Eagle.

If you ever get interested in some advanced topics, we can revisit this. Right now, we have a lot of presentation logic in the Hand class, like hiding or showing the cards depending on the game state. Instead, it'd be kind of cool to have a DealerHandRenderer and a PlayerRenderer and have those do all the rendering.

For now though, looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants