Octos Maja Brenda Scrabble #16
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! |
| @letters = { | ||
| "A"=>1, | ||
| "B"=>3, | ||
| "C"=>3, |
There was a problem hiding this comment.
I like this choice of data structure. Since it's not going to change during one run of the program, this is the sort of thing that would be great as a constant.
| else | ||
| array_of_words.length.times do | ||
| @words_scores = {} | ||
| array_of_words.each do |word| |
There was a problem hiding this comment.
You loop over the list of words twice here! I think the outer loop (line 63) could be removed entirely, since you unconditionally return at the end of it (line 86), so it will never run more than once.
| elsif array_of_words.length == 1 | ||
| return array_of_words[0] | ||
| else | ||
| array_of_words.length.times do |
There was a problem hiding this comment.
Ideally this check should be unnecessary, and your loop code should do the right thing if there's only one word.
| if word.length == 7 | ||
| winner = word | ||
| else | ||
| winner = tied_words.min{|word1,word2| word1.length <=> word2.length} |
There was a problem hiding this comment.
On line 83 you're computing the min across all the tied words within the loop of all the tied words. This is probably not what you want - it means you're looking at all the words each time you look at one of the words, an O(n^2) operation. We could rewrite this tiebreaker section as follows:
winner = tied_words.find{ |word| word.length == 7 }
if winner.nil?
# min by length, equivalent to your line 83 but a little more concise
winner = tied_words.min_by{ |word| word.length }
end| if @won == true | ||
| return false | ||
| else | ||
| @plays << word |
There was a problem hiding this comment.
Since the if clause returns from the method, you can leave out the else:
if @won
return false
end
@plays << word
# ...| describe 'won method' do | ||
| it 'returns true if score is > 100' do | ||
| john = Scrabble::Player.new("John") | ||
| john.play("squeeze") |
There was a problem hiding this comment.
I would like to see more precision around the "edge" between winning and not winning. What happens if the player has:
- 99 points
- 100 points
- 101 points
| it 'returns the highest word if there are two words' do | ||
| it 'returns the highest value word if there are two words' do | ||
| words2 = ["cookie", "cake"] | ||
| Scrabble::Scoring.highest_score_from(words2).must_equal "cookie" | ||
| end |
There was a problem hiding this comment.
Like we discussed in class, it would probably be wise to check multiple orderings of these words, that is, both ['dog', 'fish'] and ['fish', 'dog']. You'd be surprised how often this sort of thing turns up a bug.
| end | ||
|
|
||
| it 'if tied, prefer a word with 7 letters' do | ||
| words3 = ["qqqqqx", "aeiould"] | ||
|
|
||
| Scrabble::Scoring.highest_score_from(words3).must_equal "aeiould" |
| it 'returns a collection of random tiles' do | ||
| tile_bag = Scrabble::TileBag.new | ||
| tile_bag.draw_tiles(4).must_be_instance_of Array | ||
| end |
There was a problem hiding this comment.
You should check more than that it gives back an array here. For example:
- Is the array the correct length?
- Are all the members letters?
- Are the letters that were returned the same as the ones that were removed from the bag (how would you test this?)
Expanding your tests in this way would have caught that bug with draw_tiles.
| tile_bag = Scrabble::TileBag.new | ||
| tile_bag.draw_tiles(7) | ||
| tile_bag.default_tiles.values.sum.must_equal 91 | ||
| end |
There was a problem hiding this comment.
I would also like to see tests for draw_tiles that answer the following questions:
- 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
scoremethod in theScoringclass a class method instead of an instance method?Enumerablemixin? If so, where and why was it helpful?Also, we used the delete_if method in the draw_tiles method in the Tilebag file to delete the letter if its quantity was 0. They were very useful to complete and meet the requirements.