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

Lindsay + Katricia - Edges - Adagrams #24

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

Conversation

krsmith7
Copy link

Adagrams

Congratulations! You're submitting your assignment.

Comprehension Questions

Feature Feedback
What are the components that make up a method? A method signature, parameters, method body with implicit or explicit return, and end.
What are the advantages of using git when collaboratively working on one code base? Logs of all changes by all users, ability to edit at the same time, ability to merge changes and manage conflicts.
What kind of relationship did you and your pair have with the unit tests? They provided us guidance on next steps and ways to fix our code.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? We used enumerables in transforming the array and hash data more efficiently.
What was one method you and your pair used to debug code? We used pry.
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 discussed ways to step outside of "guess culture" tendencies when giving feedback. We also discussed if the driver-navigator framework worked for us.

@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 some - I would like to see both more frequent commits, and more descriptive commit messages. Each message should give the reader a sense of what changed and why, without having to dive into the code or the project specs.
Code Requirements
draw_letters method
Uses appropriate data structure to store the letter distribution yes
All tests for draw_letters pass no - You changed the tests! It seems like there was a requirements miss here - see inline.
uses_available_letters? method
All tests for uses_available_letters? pass yes
score_word method
Uses appropriate data structure to store the letter scores sort of - see inline
All tests for score_word pass yes
highest_score_from method
Appropriately handles edge cases for tie-breaking logic almost - see inline
All tests for highest_score_from pass almost - see inline
Overall

I ended up having a lot to say about this one. In general your product code (lib/adagrams.rb) does a good job of solving the problem. There are a few places where it is incorrect or could be cleaned up, but in general I feel pretty good about it.

I am more concerned by what I see as trouble with the process. In particular, it seems like you struggled with the idea of writing code to match the tests. The tests (and in this case the driver code) should give you an idea of what your code should look like from the outside, how it should be used. Being able to read the tests and quickly turn this into a high-level code structure is a skill that will continue to be important as we move through the curriculum. You should be running tests on a regular basis, and if you must comment some out, uncomment them an make sure they pass before submitting.

In general I think you are on a good trajectory. Going forward, it will be important to make sure you have a strong understanding of what is expected and how you are supposed to write your code. This will be particularly relevant on Grocery Store, which has a similar structure to Adagrams.

Let me know if you have any questions about that, and keep up the hard work!

@@ -0,0 +1,136 @@
require 'pry'

Choose a reason for hiding this comment

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

You've included this file twice, once as adagrams.rb, and once as lib/adagrams.rb.

drawn_letters = draw_letters
#Arrange
drawn_letters = ["A", "Q", "L", "J", "P", "E", "K", "I", "A", "Z"]
# Assert

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 made this change. This test is supposed to be testing the draw_letters method, but with this change that method is never called in this test. All you do now is create an array by hand and then check that it has 10 letters.

# describe 'highest_score_from method' do
# it 'returns a hash that contains the word and score of best word in an array' do
# words = ['X', 'XX', 'XXX', 'XXXX']
# best_word = highest_score_from words

Choose a reason for hiding this comment

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

Why are these tests commented out? Getting these to run is part of the assignment.

if drawn_letters.include?letter
is_valid = true
puts "letter valid " + "#{is_valid}"
letter_index = drawn_letters.index(letter)

Choose a reason for hiding this comment

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

Please remove debugging output before submitting projects.

#create a method to return a hash with the highest scoring words
def highest_score_from(words)
all_words_scores = []

Choose a reason for hiding this comment

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

This implementation is close, but the tie-breaking logic isn't quite right. The problem is that you don't respect the order in which the words were given.

The reason this happens is the sorting you do on line 112. This will re-order the words by their scores, so that the input order is now gone.

As it turns out, you don't need to sort the words at all to find the max. If we change line 112 to word_rank = all_words_scores, the rest of your logic works perfectly and all the (uncommented) tests pass.

if best_word[:word].length != 10
if word_rank[index][:word].length == 10
best_word = word_rank[index]
elsif word_rank[index][:word].length < best_word[:word].length

Choose a reason for hiding this comment

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

Good work getting this tricky tie-breaking logic figured out.

all_letters.uniq.each do |letter|

case letter
when "A","E", "I", "O", "U", "L", "N", "R", "S", "T"

Choose a reason for hiding this comment

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

The idea of generating this data structure dynamically with a loop and a case statement is an interesting one. However, I think I would argue that the resulting code is a little less readable than just hardcoding it:

LETTER_SCORES = {
  "A" => 1,
  "B" => 3,
  # ...
}

return user_letters
end

DRAWN_LETTERS = draw_letters(all_letters)

Choose a reason for hiding this comment

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

I think there was some confusion about what was expected here. The idea is that draw_letters should be called by the driver code (wave-X-game.rb) or by the specs in order to create a hand of letters. Your code should not ever need to call draw_letters.



# Fill pool of letters array with letters
letters.each do |letter, freq|

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 chosen to store the letter frequencies in a hash and then dynamically turn them into an array. I would say that this is much more readable than a giant hard-coded array with 9 As, 2 Bs, 2 Cs, etc.

def uses_available_letters? (input, drawn_letters)
is_valid = "default"
word_letters = make_word_array(input)
word_letters.each do |letter|

Choose a reason for hiding this comment

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

This method is destructive! Because you call delete_at on the drawn_letters directly, the hand will be reduced by the letters in the input. For example:

hand = draw_letters
puts hand
# => ["T", "T", "E", "F", "R", "A", "E", "L", "V", "A"]
uses_available_letters?('tte', hand)
puts hand
# => ["F", "R", "A", "E", "L", "V", "A"]

While none of the tests we provided check for destroying the hand, this is bad because it's unexpected behavior. Nothing about "check whether this word is made up of letters in this hand" suggests that the hand will be changed in the process, but that's exactly what happens.

The way to address this problem would be to make a copy of the hand before removing letters from it.

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