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

Ampers - Abinnet + Lily - Scrabble #4

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

Conversation

lillers1122
Copy link

@lillers1122 lillers1122 commented Feb 23, 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? Git helps facilitate collaboration on one code base by making it possible for all collaborators to work on their own computers and forces version control for the overall progress.
Why do you think we chose to make the score method in the Scoring class a class method instead of an instance method? Because you want to be able to compare scores across different instances. Also centralizing scoring in one class is more efficient because we do not over-process data when we call on it from other classes.
Describe an example of a test you wrote for a nominal case. Player test: it 'returns the score of the word' do / player_1 = Scrabble::Player.new("lily") / player_1.play("ada").must_equal 4 /end / is a nominal test case because it tests that a method works the way it should for a reasonable input. Describe an example of a test you wrote for an edge case.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? In our tile_bag.rb file we used the enumerable ".sum" in this method: /def tiles_remaining / @tile_bag.values.sum / end / It was helpful because it made our code shorter and more efficient. And, it was easier to troubleshoot.
What was one method you and your pair used to debug code? We used awesome_print a lot. We also white boarded before we started.
Is there a specific piece of code you'd like feedback on? We chose to put our letter and their values as arrays in the scoring.rb file. Should we have used hashes? We weren't sure which to do. Also, we got a little bogged down by how our classes communicate with each other and use each others' methods.
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? 1. Communication style established at start; also assumed positive intent with all feedback 2. Coded together with the assumption we were not pursing the BEST path, but were nevertheless pursing valid paths.

@CheezItMan
Copy link

Scrabble

What We're Looking For

Feature Feedback
General
Answered comprehension questions Check, Over-processing data, really isn't a thing, at least now how you're describing it. I'm glad you worked together well.
Both Teammates contributed to the codebase Both teammate have commits!
Regular Commits with meaningful commit messages Check, Ok number of commits, but you really need to focus on the functionality you added in the commit messages, not vague messages about tests or waves.
Scoring class
score method Check
Uses appropriate data structure to store the letter score You could do a more efficient job tracking letter scores. Yours however works.
Appropriately handles edge cases Score handles the edge cases.
highest_score_from method DOES NOT WORK You are only returning the 1st element from a tie, and your tests always assume the 1st element is the winner. You did NOT look at the conditions for the winning word. Read the question more carefully.
Appropriately handles edge cases NO See above and in inline notes.
Test implementations for highest_score_from Some good tests, but this method is not throughly tested. See my inline notes.
Player class
Uses an array to keep track of words played Check
Uses existing code to create highest_scoring_word and highest_word_score Your highest_word_score only works if the prior method was called first. That's a seriously dangerous assumption.
Returns true or false from won? method Check
Tests edge cases on the play(word) No see my inline notes.
TileBag class
Uses a data structure to keep the set of default tiles in the bag You have a data structure, but the TileBag gives each letter an equal chance to be drawn even though for example vowels are present in much greater numbers. You also have it set up to allow the TileBag to have a negative number of a specific letter! See my inline notes.
draw_tiles method uses the parameter to draw a set of random tiles Check, but see the notes.
tiles_remaining method returns a count Check
Tests draw_tiles edge cases No edge-cases tested.
Summary You both need to seriously look at your tests and make sure they are through. If you had you would have spotted the errors in the highest_score_from method. TileBag and Player bugs are harder to spot, but your testing was very thin. Try to test all possibilities, at this stage writing too many tests is better than too few! You hit a lot of the learning goals, except for the TDD requirement. You need to be testing all the requirements. This is something I'd like you both to focus on this week.

lib/scoring.rb Outdated
end

def self.highest_score_from(array_of_words)
max_points = 0

Choose a reason for hiding this comment

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

This method breaks when given two words, both with the same score but the 2nd one has 7 letters. Instead of returning the 7-letter-word it returns the 1st word in case of a tie, every time.

end

it 'if tied, prefer a word with 7 letters' do
Scrabble::Scoring.highest_score_from(["aaaaaad", "qqqqqj"]).must_equal "aaaaaad"

Choose a reason for hiding this comment

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

This only tests that the 7-letter word wins, if it comes first in the array.

To test it adequately you'd need to also test:

Scrabble::Scoring.highest_score_from(["qqqqqj", "aaaaaad"]).must_equal "aaaaaad"

end

it 'if tied and no word has 7 letters, prefers the word with fewer letters' do
Scrabble::Scoring.highest_score_from(["qqqqqq", "zxd"]).must_equal "qqqqqq"

Choose a reason for hiding this comment

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

Look at the description here! The word with more letters is winning!

Like above you should also scramble the order.

end

it 'returns the first word of a tie with same letter count' do
Scrabble::Scoring.highest_score_from(["adbf", "egch"]).must_equal "adbf"

Choose a reason for hiding this comment

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

Again swapping around the order is needed.

def self.score(word)
@word_array = word.upcase.split("")

Choose a reason for hiding this comment

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

This works, but can you think of a more efficient data structure to hold the point values?

player_1 = Scrabble::Player.new("Isabel")
player_1.tiles.must_equal []
player_1.draw_tiles(game_1)
player_1.tiles_hand.length.must_equal 7

Choose a reason for hiding this comment

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

You should also have some way to test that the correct letters were removed from the tilebag and placed in the player's hand.


describe 'draw_tiles' do
it 'can fill hand to 7 tiles' do
game_1 = Scrabble::TileBag.new

Choose a reason for hiding this comment

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

This test hasn't changed from above.

describe 'initialize' do
it 'correctly initializes a bag of tiles' do
game_1 = Scrabble::TileBag.new
game_1.tile_bag["C"].must_equal 2

Choose a reason for hiding this comment

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

More testing required.

end

it 'removes random tiles from tile_bag' do
game_1 = Scrabble::TileBag.new

Choose a reason for hiding this comment

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

Good

# end

describe 'tiles_remaining' do
it 'returns number of tiles remaining in bag' do

Choose a reason for hiding this comment

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

It would be better to set sum_control = game_1.tiles_remaining then draw and then compare the difference.

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