- 
                Notifications
    You must be signed in to change notification settings 
- Fork 29
Ports - Alex & Ngoc #16
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
 Well done on this project, you two! Your code handles the logic well and accurately, and I like the use of  I have a few comments, but in general, I think your code looks great. Good work! | 
| 9.times do | ||
| letter_pool << "A" | ||
| letter_pool << "I" | ||
| end | 
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 this strategy of using times to populate the letter_pool
| letter_pool << "O" | ||
| end | ||
|  | ||
| letter_pool.sort! | 
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 that you sorted, and you did it with the ! which is great! Technically, it shouldn't actually matter if you sort it though, since it's all random anyway
| return rand_letter | ||
| end | ||
|  | ||
| letters_in_hand = draw_letters | 
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.
Since this is library code, you'll want to delete this line, since it doesn't get used or run anywhere (or at least, doesn't get used by the tests!)
|  | ||
| letters_in_hand = draw_letters | ||
|  | ||
| user_input = gets.chomp | 
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.
Same as above: this line doesn't ever get executed, so it's okay to remove this line
| return true | ||
| else | ||
| return false | ||
| end | 
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! Very clean logic
| n = scores.length | ||
| winning_word_array = [] | ||
|  | ||
| (0..n - 1).each do |n| | 
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.
Outside of this loop, n means the total number of scores/words, and inside, n means the number. It might help to have a more descriptive name for n outside the loop
| winning_word = word | ||
| break | ||
| else | ||
| winning_word = winning_word_array.min_by { |word| word.length } | 
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 your code as is. However, if you wanted to do some extreme optimization: Let's say that winning_word_array was an array of 5 items. Let's say that all 5 items a length of less than 10 (so they would end up in this else block.) winning_word ends up being re-assigned all 5 times because this each loop gets us here, and the value of winning_word never changes because of this winning_word_array.min_by call... so it's a little redundant. A possible refactoring to get this super fancy would be to change the logic so it doesn't do that. :)
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerablemixin? If so, where and why was it helpful?