-
Notifications
You must be signed in to change notification settings - Fork 29
Sockets: Jansen & Kate #22
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
base: master
Are you sure you want to change the base?
Conversation
AdagramsWhat We're Looking For
Good job overall! This code is clear and well-organized. I've left some inline comments below about things that could be cleaned up, but in general I'm pretty happy with what I see here. Keep up the hard work! |
| # WAVE 1 | ||
| LETTER_POOL = {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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of a constant here. This data structure makes it very easy to tell how many there are of each letter.
| letter_draw = LETTER_POOL.map do |key, value| | ||
| (key.to_s * value).split("") | ||
| end | ||
| letter_draw.flatten! |
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.
Could you use more descriptive variable names here than key and value?
| hand_count = letters_in_hand.count(letter) | ||
| if user_count > hand_count | ||
| uses_available_letters = false | ||
| break |
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.
Rather than keeping this extra variable uses_available_letters here, you could return false here, and return true at the bottom.
| SCORE_CHART = {1 => ["a", "e", "i", "o", "u", "l", "n", "r", "s", "t"], | ||
| 2 => ["d", "g"], | ||
| 3 => ["b", "c", "m", "p"], | ||
| 4 => ["f", "h", "v", "w", "y"], |
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 like the idea of using a hash for this, but I think I would switch the keys and the values. I would go with something like:
LETTER_SCORES = {
"A" => 1
"B" => 3,
"C" => 3,
"D" => 2,
# ...
}Then to get the score for a letter, you can say LETTER_SCORES[letter].
| def tie_breaker(word, current_best, list) | ||
| new_best = current_best | ||
| if word.length == current_best.length | ||
| if list.index("#{word}") < list.index("#{current_best}") |
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 that you broke this complex logic out into a separate method. Good organization!
| if word.length == 10 | ||
| best_word = word | ||
| best_score = score | ||
| break |
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.
This will do the wrong thing if there's a short word that scores higher than a 10-letter word. For example:
➜ pry -r ./lib/adagrams.rb
[1] pry(main)> word_a = 'ZZZZZZZZZ'
=> "ZZZZZZZZZ"
[2] pry(main)> word_b = 'AAAAAAAAAA'
=> "AAAAAAAAAA"
[3] pry(main)> score_word(word_a)
=> 98
[4] pry(main)> score_word(word_b)
=> 18
[5] pry(main)> highest_score_from([word_a, word_b])
=> {:word=>"AAAAAAAAAA", :score=>18}Paying attention to word length 10 only matters for breaking ties.
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerablemixin? If so, where and why was it helpful?