-
Couldn't load subscription status.
- Fork 29
Ports - Heather & Laneia #12
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
Hi Heather and Laneia! Great work on this project! In general, I think you all really rocked this project in terms of finding elegant, beautiful, logical, clean, concise solutions. They are very aligned with thinking like a programmer: being comfortable transforming, filtering, and finding data in ways that work for you. I think sometimes this sacrifices readability, so I put a few comments where I thought that might be true. However, I won't tell you two to STOP writing code like this, because it's beautiful, but please keep readability in mind in the future ;) Also, great work on the optional wave! Again, great work overall. |
|
|
||
| def draw_letters | ||
| avail_letters = %w[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] | ||
| avail_letters.sample(10) |
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.
Yes! I love this syntax! This implementation is so short! :) Fantastic.
|
|
||
| def draw_letters | ||
| avail_letters = %w[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] | ||
| avail_letters.sample(10) |
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 general, your code takes advantage of Ruby's implicit return statements, but if you ever felt like going back to explicit return statements, I wouldn't be mad...
| end | ||
|
|
||
| sample = draw_letters | ||
| # print sample |
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 code will not be used when running tests, feel free to delete these lines before project submission
| else | ||
| false | ||
| end | ||
| 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.
VERY interesting and good job of implementing your own .all? logic! I love it! In general, I think that understanding the implicit return of this method might be confusing, so I might recommend rewriting this code to be something like, and take advantage of variables storing what the .all? block turns into, and using the explicit return
I have an example of maybe what I would do:
is_valid = input.each_char.all? do |char|
if letters_in_hand.include?(char)
letters_in_hand.delete_at(letters_in_hand.index(char))
true
else
false
end
end
return is_valid| score_chart.each_with_index do |letter_bank, letter_value| | ||
| word.each_char do |char| | ||
| if letter_bank.include?(char) | ||
| score << (letter_value + 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.
Ha! Interesting relationship between the index of the letter_bank inside of score_chart and its point value. Because this relationship isn't very trustworthy, I might suggest using a different data structure with a more obvious relationship between letter and point value, but I applaud this Sherlock Holmes sleuthing ;)
| score << 8 | ||
| end | ||
|
|
||
| return score.sum |
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 it's really fascinating that you all did an array of scores and then summed it in the end! I think most people would have opted to just add them, but I think this is also valid and interesting and cool and is very compatible with thinking like a programmer :D
|
|
||
| high_score = scored_words.max_by { |point| | ||
| point[:each_score] | ||
| }[:each_score] |
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.
Fantastic use of max_by (this is one of my favorite solutions to this part of the project!)
| semi_finalist[:each_score] == high_score | ||
| }.map { |semi_finalist| | ||
| semi_finalist[:each_word] | ||
| } |
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 that using select and map are the right ways to go about this. However, you're starting to chain a lot of methods together, which is valid, but leads to unreadable code. Breaking up these two method calls (select and map) into two different lines doesn't detract from your elegant code style, in my opinion!
| } | ||
|
|
||
| if highest_scored_words.length == 1 | ||
| return ultimate_winner = {word: highest_scored_words[0], score: high_score} |
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 the return statement ends the method, you're probably okay with skipping the variable assignment: return {word: highest_scored_words[0], score: high_score}
| if highest_scored_words.length == 1 | ||
| return ultimate_winner = {word: highest_scored_words[0], score: high_score} | ||
| elsif highest_scored_words.length > 1 | ||
| winner = highest_scored_words.find { |ten| ten.length == 10 } |
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! This works with knowing that .find will find the FIRST instance of true, which meets the requirements. ten is probably not my favorite variable name though.
| end | ||
|
|
||
| def is_in_english_dict?(user_input_word) | ||
| require "csv" |
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'll want to put all require statements at the top of the file no matter what!
| dictionary_array << row | ||
| end | ||
| dictionary_array.flatten! | ||
| return dictionary_array.include?(user_input_word) ? true : false |
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.
Consider why this is the same thing as above: return dictionary_array.include?(user_input_word)
| end | ||
| dictionary_array.flatten! | ||
| return dictionary_array.include?(user_input_word) ? true : 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 work on this optional! :D ... Where are the tests to verify that it works? ;)
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerablemixin? If so, where and why was it helpful?