Edges Layla and Dionisia's Adagrams!#16
Conversation
AdagramsWhat We're Looking For
Good work overall. You've done a good job of writing clean, readable code that solves the problem at hand. There are a couple of bugs with your |
| def game_instructions | ||
| puts "Based on these letters, give us a word." | ||
| print "Word: " | ||
| end |
There was a problem hiding this comment.
You shouldn't need to implement these methods - that's what the wave-X-game.rb files attached to this project are for.
| # wave 1 | ||
| def stores_letters | ||
| letters = [ "A", "A", "A", "A", "A", "A", "A", "A", "A", "B", "B", "C", "C", "D", "D", "D", "D", "E", "E", "E", "E", "E", "E", "E", "E", "E", "E", "E", "E", "F", "F", "G", "G", "G", "H", "H", "I", "I", "I", "I", "I", "I", "I", "I", "I", "J", "K", "L", "L", "L", "L", "M", "M", "N", "N", "N", "N", "N", "N", "O", "O", "O", "O", "O", "O", "O", "O", "P", "P", "Q", "R", "R", "R", "R", "R", "R", "S", "S", "S", "S", "T", "T", "T", "T", "T", "T", "U", "U", "U", "U", "V", "V", "W", "W", "X", "Y", "Y", "Z" ] | ||
| end |
There was a problem hiding this comment.
This data structure makes sense for storing the pool of letters. However, laid out like this it's not easy to see the distribution of letters. Something like this might be a bit easier to read:
letters = [
"A", "A", "A", "A", "A", "A", "A", "A", "A",
"B", "B",
"C", "C",
...
]Or, if you were feeling fancy:
letter_distribution = {
A: 9,
B: 2,
C: 2,
...
}
letters = []
letter_distribution.each do |letter, count|
count.times do
letters << letter
end
end| def uses_available_letters?(input, letters_in_hand) | ||
| input = input.upcase.split("") | ||
| input.each do |letter| | ||
| if letters_in_hand.include?(letter) |
There was a problem hiding this comment.
There are two problems with this implementation:
-
You will never return
truefor a word with a repeated letter. For example,hand = ["E", "F", "R", "A", "E", "L", "V", "A", "D", "K"] uses_available_letters?("ADA", hand) # => false
The reason why is that Ruby's
Array#deletemethod will remove all matching elements from an array, not just the first match. -
This method is destructive! Because you call
deleteon the input, the hand will be reduced by the letters in the input. For example:hand = draw_letters puts hand # => ["T", "T", "E", "F", "R", "A", "E", "L", "V", "A"] uses_available_letters?('tte', hand) puts hand # => ["E", "F", "R", "A", "E", "L", "V", "A"]
This is bad because it's unexpected behavior. Nothing about "check whether this word is made up of letters in this hand" suggests that the hand will be changed in the process, but that's exactly what happens.
The way to address this problem would be to make a copy of the hand before removing letters from it.
I will acknowledge that none of the tests checked for either of these behaviors, but that doesn't change the fact that they're bugs.
| score_hash = { | ||
| "A" => 1, | ||
| "E" => 1, | ||
| "I" => 1, |
There was a problem hiding this comment.
I like this data structure. Why not store this data in a constant?
| # if current word has 10 letters and current winner does not, | ||
| # current word becomes new current winner. | ||
| if word.length == 10 && winning_word_and_score[:word].length != 10 | ||
| winning_word_and_score[:word] = word |
There was a problem hiding this comment.
Good work getting this tricky tie-breaking logic sorted out.
3 participants
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerablemixin? If so, where and why was it helpful?