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

Karinna & Angelica - Scrabble - Octos #9

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

Conversation

amcejamorales
Copy link

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's easier to switch between driving and navigating while using your own computer.
Why do you think we chose to make the score method in the Scoring class a class method instead of an instance method? Because we never create a new instance of the Scoring class. The purpose of this class is similar to modules. We used it as a namespace to hold functionality.
Describe an example of a test you wrote for a nominal case. For play(word) in the Player class, we wrote a test that passes in a valid string of characters and it returns the accurate score for that string.
Describe an example of a test you wrote for an edge case. We created a test to check if draw_tiles(num) raises an argument error if the argument is greater than the amount of tiles remaining.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? Our highest_score_from method in the Scoring class uses several methods. This was helpful because it helped us eliminate several .each methods and clarified the purpose of each iteration.
What was one method you and your pair used to debug code? We used binding.pry on several occasions. For example, we used it to figure out that a constant we declared inside of our TileBag class was being altered by individual instances of that class.
Is there a specific piece of code you'd like feedback on? In our highest_score_from method inside our Scoring class we used several helper methods. We wonder whether it would have been better to write the loops inside just one bigger method. How fine-grained should we get when keeping the one-responsibility principle in mind.
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 having a more even distribution of time spent navigating/driving. Also, it was useful to talk about what we wanted to improve on from the beginning, because we were better able to help each other.

@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 yes
Test implementations for highest_score_from
Player class
Uses an array to keep track of words played yes
Uses existing code to create highest_scoring_word and highest_word_score no - see inline
Returns true or false from won? method yes
Tests some missing - see inline
TileBag class
Uses a data structure to keep the set of default tiles in the bag see inline
draw_tiles method uses the parameter to draw a set of random tiles yes
tiles_remaining method returns a count yes
Tests draw_tiles edge cases yes

Good job overall! 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)

I feel that you've done a good job of demonstrating all three of these. Your test coverage could use some work - several of your methods are missing interesting cases - but it seems like you're on the right track. I've left some inline comments for you to review, please do so, and keep up the hard work!


LETTER_SCORES = { "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,

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.

LETTER_SCORES[letter]
end

points = score_array.inject(points,:+)

Choose a reason for hiding this comment

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

Excellent use of map and inject in this method.

hashes_array = self.get_hashes(array_of_words)
winner_score = self.get_winner_score(hashes_array)
hashes_array = self.get_winner_hashes(hashes_array, winner_score)
seven_letters = self.seven_letters(hashes_array)

Choose a reason for hiding this comment

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

I love the idea of breaking the pieces of this out into separate methods. However, in this case I think I agree that you may have gone a little too far, in that each of the components is so far removed from the others that it's hard to see the pattern. This would make it difficult to refactor if, for example, you wanted to switch to the inline tie-breaking style we did in class.

That being said, I definitely don't want to discourage functional decomposition. This is still way more readable than one enormous method. Figuring out where to draw the line is a bit of an art form.

return false
elsif score.nil?
return 0
else

Choose a reason for hiding this comment

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

Good job catching this behavior.

def won?
if self.total_score > 100
return true
else

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


def draw_tiles(num)

available = @total_tiles.select do |tile, quant|

Choose a reason for hiding this comment

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

By this logic, you're as likely to draw an e as you are a z! It also means that you won't ever get more than one of the same letter, which could be bad if you need to draw the last two tiles from the bag and they're both i. However, the way you've set up your data structures, it would be difficult to do it any other way.

Instead, you might keep all the letters in the bag in an array. You could seed the array with code like:

def initialize
  letters = {
    # the hash you had before
  }
  @tiles = []
  letters.each do |letter, count|
    count.times do
      @tiles << letter
    end
  end
end

Then you could pull a random tile with sample and get a nice even distribution.


describe '#won?' do

it "returns false if the player does not have over 100 points" do

Choose a reason for hiding this comment

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

I would like to see more precision around the "edge" between winning and not winning. What happens if the player has:

  • 99 points
  • 100 points
  • 101 points

end

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

Choose a reason for hiding this comment

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

Like we discussed in class, it would probably be wise to check multiple orderings of these words, that is, both ['dog', 'fish'] and ['fish', 'dog']. You'd be surprised how often this sort of thing turns up a bug.

it "Sets up instance with a collection of tiles" do
new_pool = Scrabble::TileBag.new
quantities = { "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 }
new_pool.total_tiles.must_equal quantities

Choose a reason for hiding this comment

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

I don't know that I like this test - seems like you're just copying the data that you have in tile_bag.rb. That means it's not likely to find a bug, and if that data ever changes (Scrabble rules change or you do find a bug) you'll have to make the update in more than one place.

Instead, it might be better in this case to test less. Maybe check that you get the right number of tiles and leave it at that.


it "raises an argument error if the user tries to take more tiles than are available" do
new_bag = Scrabble::TileBag.new
result = proc { new_bag.draw_tiles(100) }

Choose a reason for hiding this comment

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

I like this test! Good work!

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