-
Notifications
You must be signed in to change notification settings - Fork 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
Dikla Rikovitch Scrabble Octo #18
base: master
Are you sure you want to change the base?
Conversation
ScrabbleWhat We're Looking For
Good job overall! The big learning goals I'm looking to see demonstrated on this project are:
I feel that you've done a good job of demonstrating the first and the third of these. Your test coverage could use some work - several of your methods are missing tests entirely, or are missing interesting cases. I've left some inline comments for you to review, please do so, and keep up the hard work! |
case letter.downcase | ||
when "a", "e", "i", "o", "u", "l", "n", "r", "s", "t" | ||
total_score += 1 | ||
when "d", "g" |
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.
Storing these as options in a case
statement certainly works, but it is a single-use solution. If you ever needed to use letter values somewhere else, you'd have to copy paste. An alternative would be to build a data structure stored in a variable that maps letters to scores, something like this:
LETTERS = {
"A" => 1
"B" => 3,
"C" => 3,
"D" => 2,
# ...
}
Then to get the score for a letter, you can say LETTERS[letter]
.
end | ||
|
||
it 'returns the highest word if there are two words' do | ||
Scrabble::Scoring.highest_score_from(["apple", "pear"]).must_equal "apple" |
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.
Like we discussed in class, it would probably be wise to check multiple orderings of these words, that is, both ['apple', 'pear']
and ['pear', 'apple']
. You'd be surprised how often this sort of thing turns up a bug.
|
||
def plays | ||
return @player_array | ||
end |
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.
If you named the array @plays
, you could make this method an attr_reader
.
def won? | ||
if @total_score > 100 | ||
return true | ||
else |
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.
Since >
will always return true
or false
, this could be shortened to:
def won?
return total_score > 100
end
|
||
describe 'play' do | ||
it 'Adds the input word to the plays Array' do | ||
|
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.
What happens if the player has already won? Is the word recorded? What does it return?
end | ||
|
||
describe 'won?' do | ||
it 'Returns true if the player has over 100 points' do |
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 would be interested in investigating a little closer around 100 points, to more clearly define the "edge" of the behavior. In particular, what happens when the player has:
- 99 points
- 100 points
- 101 points
@tiles = [] | ||
DEFAULT_TILES.each do |letter, count| | ||
count.times {@tiles << letter} | ||
end |
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 love this strategy for getting the initial tiles into the tile bag! Keeping them in an array is much simpler than trying to manage them in a hash, for example.
tile_bag.draw_tiles(5) | ||
after_count = tile_bag.tiles_remaining | ||
after_count.must_equal 93 | ||
end |
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.
Other questions I would like your tests to answer:
- Am I able to draw the last tile from the bag?
- What happens if I try to draw more tiles than are currently in the bag?
- What if I try to draw 0 or -3 tiles?
Scrabble
Congratulations! You're submitting your assignment.
Comprehension Questions
score
method in theScoring
class a class method instead of an instance method?Enumerable
mixin? If so, where and why was it helpful?