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

Dikla Rikovitch Scrabble Octo #18

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

Conversation

diklaaharoni
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? you can work remotely with your partner
Why do you think we chose to make the score method in the Scoring class a class method instead of an instance method? because there is no need to create an instance of score
Describe an example of a test you wrote for a nominal case.
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? no
What was one method you and your pair used to debug code? we used pry
Is there a specific piece of code you'd like feedback on? all
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?

@droberts-sea
Copy link

Scrabble

What We're Looking For

Feature Feedback
General
Answered comprehension questions Several of these answers are missing, and the ones you do have are pretty sparse. In future pair projects, please set aside some time with your partner to go over these questions together before the project is due.
Both Teammates contributed to the codebase yes
Regular Commits with meaningful commit messages I see a lot of commit messages that say something like "no changes" or "nothing to commit", but which are actually big commits full of work. In the future, please try to provide commit messages that describe how and why the code changed.

This submission is currently full of git merge-conflict markers! These need to be removed before you're really finished merging. To grade it, I rolled back to a previous version before all those came in.
Scoring class
score method yes
Uses appropriate data structure to store the letter score see inline
Appropriately handles edge cases yes
highest_score_from method yes
Appropriately handles edge cases yes - good work getting the tie-breaking logic
Test implementations for highest_score_from mostly - 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 yes
Returns true or false from won? method yes
Tests missing some - see inline
TileBag class
Uses a data structure to keep the set of default tiles in the bag yes
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 missing some - see inline

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 the first and the third of these. Your test coverage could use some work - several of your methods are missing tests entirely, or are missing interesting cases. I've left some inline comments for you to review, please do so, and keep up the hard work!

case letter.downcase
when "a", "e", "i", "o", "u", "l", "n", "r", "s", "t"
total_score += 1
when "d", "g"

Choose a reason for hiding this comment

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

Storing these as options in a case statement certainly works, but it is a single-use solution. If you ever needed to use letter values somewhere else, you'd have to copy paste. An alternative would be to build a data structure stored in a variable that maps letters to scores, something like this:

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

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

end

it 'returns the highest word if there are two words' do
Scrabble::Scoring.highest_score_from(["apple", "pear"]).must_equal "apple"

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 ['apple', 'pear'] and ['pear', 'apple']. You'd be surprised how often this sort of thing turns up a bug.


def plays
return @player_array
end

Choose a reason for hiding this comment

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

If you named the array @plays, you could make this method an attr_reader.

def won?
if @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


describe 'play' do
it 'Adds the input word to the plays Array' do

Choose a reason for hiding this comment

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

What happens if the player has already won? Is the word recorded? What does it return?

end

describe 'won?' do
it 'Returns true if the player has 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 be interested in investigating a little closer around 100 points, to more clearly define the "edge" of the behavior. In particular, what happens when the player has:

  • 99 points
  • 100 points
  • 101 points

@tiles = []
DEFAULT_TILES.each do |letter, count|
count.times {@tiles << letter}
end

Choose a reason for hiding this comment

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

I love this strategy for getting the initial tiles into the tile bag! Keeping them in an array is much simpler than trying to manage them in a hash, for example.

tile_bag.draw_tiles(5)
after_count = tile_bag.tiles_remaining
after_count.must_equal 93
end

Choose a reason for hiding this comment

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

Other questions I would like your tests to answer:

  • Am I able to draw the last tile from the bag?
  • What happens if I try to draw more tiles than are currently in the bag?
  • What if I try to draw 0 or -3 tiles?

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