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

Ana Lisa & Jamila: Octos-C9 #21

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

Conversation

The-Beez-Kneez
Copy link

@The-Beez-Kneez The-Beez-Kneez commented Feb 25, 2018

Scrabble

Congratulations! You're submitting your assignment.

Comprehension Questions

Feature Feedback
What are the advantages of using git when collaboratively working on one code base? It is easy for people to consider the problem separately on different screens.
Why do you think we chose to make the score method in the Scoring class a class method instead of an instance method? So that the overall module Scrabble would be able to access Scoring Class elsewhere in the program
Describe an example of a test you wrote for a nominal case. We tested whether or not the plays method could return an array.
Describe an example of a test you wrote for an edge case. We had some drafted up for won but did not get the chance to implement them.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? We used inject to receive the total_score of our player's word scores.
What was one method you and your pair used to debug code? We utilized pry pretty heavily and relied on our tests for the majority of the project.
Is there a specific piece of code you'd like feedback on? Any and all is welcome.
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 agreed that we worked well together and felt that we were able to discuss ideas and plans on an equal footing. However two individual points held us back, we should have asked for help early on and agreed in the future that if we are stuck, instead of toughing it out, we should ask for help after 20 minutes have gone by.

@droberts-sea
Copy link

Scrabble

What We're Looking For

Feature Feedback
General
Answered comprehension questions yes
Both Teammates contributed to the codebase yes
Regular Commits with meaningful commit messages yes
Scoring class
score method yes
Uses appropriate data structure to store the letter score yes
Appropriately handles edge cases yes
highest_score_from method yes
Appropriately handles edge cases tie breaking doesn't work - see inline
Test implementations for highest_score_from see inline
Player class
Uses an array to keep track of words played yes
Uses existing code to create highest_scoring_word and highest_word_score some - methods have othr problems
Returns true or false from won? method yes
Tests edge cases on the play(word) some - see inline
TileBag class
Uses a data structure to keep the set of default tiles in the bag N/A
draw_tiles method uses the parameter to draw a set of random tiles N/A
tiles_remaining method returns a count N/A
Tests draw_tiles edge cases N/A

The big learning goals I'm looking to see demonstrated on this project are:

  • Building complex logic (tie-breaking logic in highest_score_from)
  • Test coverage
  • Object composition (interaction between Player and TileBag)

You've made a good start on all of these, but there are substantial pieces missing for each. Please review my inline comments, and try and focus on these learning goals for Ride Share and next week's Hotel project. I'll be excited to see what you come up with. I appreciate that both of you have been putting in the hours recently - keep up the hard work and dedication!

LETTER_VALUES =
{
:A => 1,
:E => 1,

Choose a reason for hiding this comment

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

I like this choice of data structure!

# iterate over each letter
user_word.each do |letter|
letter = letter.to_sym
if LETTER_VALUES.has_key?(letter)

Choose a reason for hiding this comment

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

This check on line 52 is redundant! Above (line 36) you do a regex match, and here you check whether the LETTER_VALUES hash contains the letter. One or the other would suffice. I think I prefer the check here, since that makes it easy if you ever need to change the letters (maybe our version of scrabble starts allowing words with apostrophes).

# RETURNS WORD FROM ARRAY LENGTH 1
elsif array_of_words.length == 1
return array_of_words[0]
else

Choose a reason for hiding this comment

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

Since both the if and elsif branches of this conditional have a return, you can omit the else entirely. It's not a huge difference, but it saves you some indentation.

end

it 'if tied, prefer a word with 7 letters' do
array_of_words = ['dog', 'course', 'lungers']
Scrabble::Scoring.highest_score_from(array_of_words).must_equal 'lungers'
end

Choose a reason for hiding this comment

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

Since lungers gets the 7-letter bonus, I don't think you actually have a tie here. There isn't a pair of English words that allow you to test this logic, but zzzzzz and oratory will do.

"Why bother" you ask? Well, if Miriam-Webster adds zzzzzz as an onomatopoeia for snoring, we need to be prepared, or you could envision a scrabble app that works across languages or that allows users to add their own words. Moreover, maybe this is the mathematician in me speaking, but I believe there's beauty and joy to be found in writing code that isn't just "good enough", but which truly solves the problem.

end

it 'returns the highest word if there are two words' do
array_of_words = ['cat', 'zoo']
Scrabble::Scoring.highest_score_from(array_of_words).must_equal 'zoo'

Choose a reason for hiding this comment

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

It might be wise to reverse the order of this array and check that you get the same result. You would be surprised at how often this sort of thing turns up a bug. This applies to all the tiebreaker tests in this section.

# if word_scores.include?(nil)
# binding.pry
# end
return word_scores.inject(:+)

Choose a reason for hiding this comment

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

Beautiful!

end

def highest_scoring_word
Scrabble::Scoring.highest_score_from(plays)

Choose a reason for hiding this comment

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

You're missing an end here - highest_scoring_word is defined inside of won. Turns out this is valid Ruby syntax, but it'd probably not what you wanted.

if total_score > 100
return true
else
return false

Choose a reason for hiding this comment

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

Since > will always return true or false, this could be shortened to:

def won?
  return total_score > 100
end

describe 'won?' do
it 'if player has > 100 point score, return true' do
word = "crazy"
word_1 = 'janky'

Choose a reason for hiding this comment

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

Here are some other questions I would like to see answered by these tests:

  • What if the player has not played any words at all?
  • What if the player has < 100 points?
  • What if the player has exactly 100 points?


# Assert
player.total_score.must_be Integer

Choose a reason for hiding this comment

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

You should also calculate the score and check the result.

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