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

Leaves - Mariya & Yasmin #20

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

Conversation

YasminM11
Copy link

Adagrams

Congratulations! You're submitting your assignment.

Comprehension Questions

Feature Feedback
What are the components that make up a method? method signature line & the parameters that that method will take; and also all the directions/stuff to do inside the method, and so far our methods have return values.
What are the advantages of using git when collaboratively working on one code base? we could see the updated version & changes at every step in our coding process.
What kind of relationship did you and your pair have with the unit tests? We liked them! It helped us feel more secure in our code's functionality & stability. Seeing green/zero failures & errors also provided positive emotional reinforcement. We really liked using the tests available to us.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? We used the enumerable 'all' on line 40.
What was one method you and your pair used to debug code? We put different puts statements inside of loops & conditionals to see if our code was hitting those cases and performing as expected.
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 both worked well together b/c we prefer direct communication. We did differ in how much support vs. self-discovery we engaged in and had to figure out how to balance each other out and compromise (both in coding and in personal interactions). Overall, it was a very positive experience. WE HAD FUN!!!

@beccaelenzil
Copy link

Adagrams

What We're Looking For

Feature Feedback
General
Answered comprehension questions 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 see code review
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 Hey y'all! Great work on this project. Your code fulfills all of the requirements. Your code for the highest_score_from(words) method is particularly clear and elegant. I've made a few comments on this assignment about some suggestions on how to refactor. Pay particular attention to the comments on the uses_available_letters? method for some ideas on how to make this method more efficient and/or add comments to increase the readability of your code.

players_letters = []
alphabet = ("A".."Z").to_a

until players_letters.length == 10

Choose a reason for hiding this comment

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

This is clever. Consider this other method to draw_letters that uses the pool_of_letters hash to create an array of letters. You could then shuffle this array of letters and pop off a letter 10 times.

letters = []
pool_of_letters.each do |letter, count|
  count.times do
    letters << letter
  end
end


#Wave_3
def score_word(word)
one_point_letter = %w[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.

While this if/elif control structure works, it means that the information about which letter has which score is locked into this piece of code, and can't easily be used elsewhere. For example, if you wanted to display the value of each letter in a hand, you would need to repeat this work.

An alternative approach would be to store the letter scores in a hash, something like this:

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

Then to get the score for a letter, you can say LETTER_SCORES[letter].

user_word = input.upcase.chars
word_hash = {}
user_word.each do |letter|
if word_hash["#{letter}"].nil?

Choose a reason for hiding this comment

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

You do not need to use interpolation. letter is already a string.


full_pile = {}
letters_in_hand.each do |letter|
if full_pile["#{letter}"].nil?

Choose a reason for hiding this comment

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

letters_in_hand_hash would be a more meaningful name then full_pile and would follow the convention introduced in the previous loop.

full_pile["#{letter}"] += 1
end
end
return word_hash.all? do |letter, count|

Choose a reason for hiding this comment

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

This is a lot to put into a return statement. It would be good to do something like the code below. Notice the use of a comment to explain your logic. This would be useful for the two loops above as there is a lot of logic packed into this method.

# return true if every letter of the word is in the letters_in_hand_hash and they're used no more times than they appear in the letters_in_hand array

uses_available_letters_boolean = word_hash.all? do |letter, count|
  full_pile.has_key?(letter) && (full_pile[letter] >= word_hash[letter])
end
return uses_available_letters_boolean

end

#wave_4
def tie_breaker(old_word, new_word)

Choose a reason for hiding this comment

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

This is a great use of a helper method and is elegant code/logic.

return winner
end

def highest_score_from(words)

Choose a reason for hiding this comment

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

Again, the logic in this method is clear, elegant, and efficient.

end

# wave2 code
def uses_available_letters?(input, letters_in_hand)

Choose a reason for hiding this comment

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

Review this implementation of this method that does not require the creation of new hashes:

def uses_available_letters?(input, letters_in_hand)
  input_array = input.split('')
  letters_in_hand_copy = letters_in_hand.shuffle

  input_array.each do |letter_in_word|
    if letters_in_hand_copy.include?(letter_in_word)
      letters_in_hand_copy.delete(letter_in_word)
    else
      return false
    end
  end
  
  return true
end

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.

2 participants