Skip to content
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

adagrams #20

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

adagrams #20

wants to merge 16 commits into from

Conversation

amandaungco
Copy link

Adagrams

Congratulations! You're submitting your assignment.

Comprehension Questions

Feature Feedback
What are the components that make up a method? method signature, parameters, method block
What are the advantages of using git when collaboratively working on one code base? ability to track changes and work off different machines.
What kind of relationship did you and your pair have with the unit tests? lower engagement than expected, but they were very helpful guiding us - good to see how they were structured to help guide thought process for creating methods.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? select, reduce, min_by- they were helpful to parse through sets of information and return a single value. they helped dry up our code.
What was one method you and your pair used to debug code? we used pry - to look at what variables/data types to see if they matched our plans/expectations
What are two discussion points that you and your pair discussed when giving/receiving feedback from each other that you would be willing to share? How our code hygiene has decreased since we've learned more code - we wished we did more whiteboarding and psuedocode instead of going straight to writing code.

@dHelmgren
Copy link

Adagrams

What We're Looking For

Feature Feedback
General
Answered comprehension questions Good answers
Both teammates contributed to the codebase YEAH!
Small commits with meaningful commit messages Good, small, well spoken commits!
Code Requirements
draw_letters method Besides the issue below, well structured and readable.
Uses appropriate data structure to store the letter distribution Your hash actually doesn't do the appropriate task!
All tests for draw_letters pass Pass!
uses_available_letters? method It works, though you've created a little more work for yourself and for the computer than you needed to. I don't think that converting the arrays to hashes actually helps performance or clarity.
All tests for uses_available_letters? pass P-P-P-Pass!
score_word method See comments. This works, but gosh it's not very clear what's happening.
Uses appropriate data structure to store the letter scores I do think that using a fixed value hash for the scoring scheme is a great idea!
All tests for score_word pass Pass!
highest_score_from method Oooh, this is full of spiffy one liners! Do be aware of using enumerables too much though. Each enumerable hides a loop from you.
Appropriately handles edge cases for tie-breaking logic This was a very clever and readable solution.
All tests for highest_score_from pass Pass!
Overall Your code all works correctly, however, it seems like you tried to use the same tool to solve most problems. Hashes are really great for some of the things you are trying to do, but arrays are a better option in as many cases. Let me know if you want some help understanding why some of these situations called for different data structures.

def draw_letters
letter_bank = Array.new
10.times do
random_letter_picked = LETTERS_TO_QUANTITY.keys.sample

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, you have a probability problem here. When you sample from the keys, the odds are not weighted by the number of each tile in the letter_bank. Your results give a 1/26 chance to draw any letter, but that's not accurate is it?


def uses_available_letters?(user_input_word, letter_bank)
array_of_input_letters = user_input_word.split('')
letter_bank_hash = letters_to_hash(letter_bank)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason you couldn't have done this with 2 arrays?

user_input_letters_hash = letters_to_hash(array_of_input_letters)
output = true
user_input_letters_hash.each do |key,value|
if !letter_bank_hash.keys.include?(key) # look at looping of outputs, see what happens if use

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as one of these is false, do you need to check the whole hash still?

user_input_word_hash.each do |word_key, word_value|
SCORE_TO_LETTERS.each do |score_key, score_value|
if score_value.include?(word_key)
score = score_key*word_value

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a minute to see why this works. Again, using a hash here is actually more work for you, and hindering clarity.

end
end
end
total_score = array_of_letter_scores.reduce(:+)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reduce is hiding a loop from you, though you could have used reduce to solve this problem by replacing an earlier .each.


def highest_score_from(played_words)
collection_of_played_words_and_scores = []#array of hashes
played_words.each do |word|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! I love this, very clear!

played_word_to_score[:score] = score_word(word)
collection_of_played_words_and_scores.push(played_word_to_score)
end
top_score = collection_of_played_words_and_scores.reduce(0){ |memo, h| h[:score] > memo ? h[:score] : memo } # ==> {score: n}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yum, this is also great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants