- 
                Notifications
    You must be signed in to change notification settings 
- Fork 29
Sockets - Amy and Evelynn #13
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
 | 
| @@ -1,0 +1,125 @@ | |||
| def draw_letters | |||
| letter_distribution = { | |||
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.
In the future consider defining something like letter_distribution outside of the method as a constant.
LETTER_DISTRIBUTION = { 
  # ...
}
def draw_lettersUsing a constant isn't always going to be the right call but it's worth thinking about. For example, it's fine here because this data is just from a hash but if it was loaded from a file or something it might be worth only creating it once.
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 format you used here, though. I think having the letter to go its number of occurrences is really readable.
| letter_pool = [] | ||
| letter_distribution.each do |str, n| | ||
| n.times do | ||
| letter_pool << str.downcase | 
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.
When you compute something like this in a method you should think about whether or not it should be a constant. Here it's perfectly reasonable to write the method this way but it's worth keeping in mind.
If it's expensive to compute (like, if it needs to read from a file) then you may want to store it once instead of generating it multiple times.
| def uses_available_letters?(input, letters_in_hand) | ||
| if input.length > 10 | ||
| puts "Too many letters! Try again: " | ||
| input = gets.chomp.downcase | 
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.
You shouldn't be handing input from the user here.  The wave-N-game should handle that for you.
The reason for this is that it's tricky to test code that asks for user input so it's nice to have that all in one place.
| word.each_char do |letter| | ||
| total_points += (score_chart[letter]) | ||
| end | ||
| return total_points | 
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 how concise this method is. 😃
| if word.length == 10 && !(word.length == winning_word[:word].length) | ||
| winning_word[:word] = word.upcase | ||
| elsif word.length == 10 && (word.length == winning_word[:word]) | ||
| winning_word[:word].downcase = winning_word[:word].downcase | 
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 think you meant:
| winning_word[:word].downcase = winning_word[:word].downcase | |
| winning_word[:word] = winning_word[:word].downcase | 
The code you have produces a NoMethodError.
| puts "Too many letters! Try again: " | ||
| input = gets.chomp.downcase | ||
| end | ||
| if (input.chars.all? { |char| letters_in_hand.include? char }) | 
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.
Nice use of .all?!
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerablemixin? If so, where and why was it helpful?