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

Edges - Goeun Maddie - Adagrams #9

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

Conversation

goeunpark
Copy link

Adagrams

Congratulations! You're submitting your assignment.

Comprehension Questions

Feature Feedback
What are the components that make up a method? Most methods in our program take one positional argument but some methods (e.g., draw_letters) takes no argument. Methods have a block of code that returns something.
What are the advantages of using git when collaboratively working on one code base? You can review previous versions and go back to it. You can see who contributed to which code.
What kind of relationship did you and your pair have with the unit tests? When we ignored the tests and tried to pseudocode and code, things didn’t work out that hot! But afterwards when we tried to write code to pass tests, things worked better.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? We used .any, .include?, and .min_by. It was helpful because we didn’t have to create code iterating through the same data repeatedly to extract the data needed. The enumerables were able to do the work in shortened form.
What was one method you and your pair used to debug code? We used binding.pry a lot!
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? We talked about our insecurities about not contributing enough or driving too much given our respective knowledge coming into the project. We also talked about how we communicated expectations of what the completed code would look like.

madaleines and others added 30 commits August 13, 2018 16:37
@droberts-sea
Copy link

Adagrams

What We're Looking For

Feature Feedback
General
Answered comprehension questions yes
Both teammates contributed to the codebase yes
Small commits with meaningful commit messages yes
Code Requirements
draw_letters method
Uses appropriate data structure to store the letter distribution yes
All tests for draw_letters pass yes
uses_available_letters? method
All tests for uses_available_letters? pass yes
score_word method
Uses appropriate data structure to store the letter scores yes
All tests for score_word pass yes
highest_score_from method
Appropriately handles edge cases for tie-breaking logic yes
All tests for highest_score_from pass yes
Overall Great job overall! This code is well-organized and easy to read, and does a good job of solving the problem at hand. I have left a number of nitpicks below for you to review, but in general I am quite happy with this submission. Keep up the hard work!

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 }

SCORE_CHART = { A: 1, B: 3, C: 3, D: 2, E: 1, F: 4, G: 2, H: 4, I: 1, J: 8, K: 5, L: 1, M: 3, N: 1, O: 1, P: 3, Q: 10, R: 1, S: 1, T: 1, U: 1, V: 4, W: 4, X: 8, Y: 4, Z: 10 }

Choose a reason for hiding this comment

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

I like that you have these as constants. They also both feel like good choices of data-structure for this problem. One change I might recommend is to break the letters out onto separate lines.

def uses_available_letters?(input, letters_in_hand)
input = input.chars
(input & letters_in_hand) == input ? true : false
end

Choose a reason for hiding this comment

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

Using the intersection here is a very clever way to solve this problem - good work!


word.each_char do |char|
total_points << SCORE_CHART.fetch(char.upcase.to_sym)
end

Choose a reason for hiding this comment

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

I'm curious why you chose to use fetch here instead of SCORE_CHART[char.upcase.to_sym]


def find_max_words(all_scores)
all_scores.select { |word, score| score == all_scores.values.max }
end

Choose a reason for hiding this comment

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

This works! However!

You call all_scores.values.max in the inside of the select loop, which means you're re-doing this work for each score. That results in an O(n^2) time complexity. You can improve on this as follows:

def find_max_words(all_scores)
  max_score = all_scores.values.max
  all_scores.select { |word, score| score == max_score }
end

This yields an O(n) time complexity.

Choose a reason for hiding this comment

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

Also, you have this method implemented twice.

all_scores = {}

unscored_words.each do |word|
all_scores[word] = score_word(word)

Choose a reason for hiding this comment

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

What happens if there's a duplicate word?

def find_first_ten_word(max_words)
first_ten_word = max_words.select { |word, score| word.length == 10 }.keys[0]
return first_ten_word
end

Choose a reason for hiding this comment

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

Your implementations of find_first_min_word and find_first_ten_word both rely on hash ordering. What I mean by that is, they only work because Ruby keeps track of the order in which key-value pairs are added to a hash.

However, you should not generally rely on hash ordering - it's a bad habit that will get you into trouble once we start JavaScript.

def highest_score_from(unscored_words)
all_scores = find_all_scores(unscored_words)

max_scored_words = find_max_words(all_scores)

Choose a reason for hiding this comment

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

I like the way you've broken this complex method out using helper methods - that makes it much easier to read!

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