Naheed & Sigrid - Edges - Adagrams #5
Conversation
AdagramsWhat We're Looking For
This code is for the most part well-organized and easy to read, and does a good job of solving the problem at hand. The place where I see room for improvement is around your choice of data structures. For both That being said, I am generally pretty happy with what I see. Keep up the hard work! |
| hand = [] | ||
|
|
||
| initial_array = [["A", 9], ["B", 2], ["C", 2], ["D", 4], ["E", 12], ["F", 2], ["G", 3], ["H", 2], ["I", 9], ["J", 1], ["K", 1], ["L", 4], ["M", 2], ["N", 6], ["O", 8], ["P", 2], ["Q", 1], ["R", 6], ["S", 4], ["T", 6], ["U", 4], ["V", 2], ["W", 2], ["X", 1], ["Y", 2], ["Z", 1]] | ||
|
|
There was a problem hiding this comment.
I like the idea of this data structure, but I think you could simplify it a bit by using a hash, where the keys are letters and the values are counts. Something like:
{
"A" => 9,
"B" => 2,
# ...
}| word = input.chars | ||
| word.each do |letter| | ||
| if letters_in_hand.include?(letter) | ||
| letters_in_hand.delete_at(letters_in_hand.index(letter)) |
There was a problem hiding this comment.
This isn't something we checked for in the tests, but it's worth pointing out that this method is destructive. By calling it, you modify the array letters_in_hand. To me this feels like an unexpected behavior - it's not obvious that "check whether this word is made up of letters in this hand" is a process that will change the hand itself. Unexpected behaviors are often where bugs can creep in, especially when there's more than one person working on something.
| if %w[A E I O U L N R S T].include?(letter) | ||
| score += 1 | ||
| elsif %w[D G].include?(letter) | ||
| score += 2 |
There was a problem hiding this comment.
While using a big if statement like this works just fine, the information about letter scores is trapped in this code. If you wanted to use that information elsewhere (for example, to tell the user what the letters in their hand are worth) you would have to repeat it.
Instead, it might work well to store the data in a hash, something like this:
LETTER_SCORES = {
"A" => 1
"B" => 3,
"C" => 3,
"D" => 2,
# ...
}Then to get the score for a letter, you can say LETTER_SCORES[letter]
| winning_word = "" | ||
| max_values = [] | ||
| min_length = 10 | ||
| hash = {} |
There was a problem hiding this comment.
The variable names array and hash here could use some improvement. While technically correct, they don't give the reader any information about how they're used or what sort of data they store. Instead you might use words_with_scores and best_word_and_score.
| def is_in_english_dict?(input) | ||
| check = false | ||
| dictionary = CSV.open('assets/dictionary-english.csv') | ||
| word = input.downcase |
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerablemixin? If so, where and why was it helpful?