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

Samantha and Kiera - Scrabble - Octos #22

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

Conversation

samanthaberk
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 ensures that our code remains consistent even when working on different machines
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 we could use it across classes
Describe an example of a test you wrote for a nominal case. We made sure the Scrabble game could initialize a new player
Describe an example of a test you wrote for an edge case. We did not have time to write edge cases, but possible cases could be returning the full tile bag if a player draws 0, raising an error if they draw a negative number or more than 7.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? We used .max_by for scoring to find the highest scoring word. It was helpful because it simplified the logic rather using .each
What was one method you and your pair used to debug code? We used pry and minitests
Is there a specific piece of code you'd like feedback on? We would like feedback on the modifications to player using tilebag. We were unclear on what the instructions were asking for (i.e. what is the user story?)
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 talked about learning from each other and managing our time/knowing when to change strategy and seek help early when necessary to improve productivity

samanthaberk and others added 28 commits February 20, 2018 14:45
…method to calculate total score, and cleaned up the ternary statement to add 50 points to a 7 letter word
@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 - great work!
Scoring class
score method yes
Uses appropriate data structure to store the letter score see inline
Appropriately handles edge cases mostly - see inline
highest_score_from method yes
Appropriately handles edge cases yes
Test implementations for highest_score_from yes
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 mostly good - 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 Would like to see much more here, but recognize that time was short.

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)

It doesn't look like you got an opportunity to exercise the third, since Player#draw_tiles is incomplete, but I am quite pleased with the first two. Given the aggressive nature of our schedule so far, I'm not too worried that wave 3 didn't get finished. I've left a bunch of inline comments for you to review below, and keep up the hard work!

# LETTERS = {
# 1 => ["A", "E", "I", "O", "U", "L", "N", "R", "S", "T"],
# 2 => ["D", "G"],
# 3 => ["B", "C", "M", "P"],

Choose a reason for hiding this comment

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

I like the idea of using a hash for this, but I think I would switch the keys and the values. I would go with something like:

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

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

case letter
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.

Using a case statement certainly solves this problem, but it's a one-off solution. If you ever need to use letter scores anywhere else, you'll have to recreate this data. Keeping it in a data structure makes it much more amenable to change.

# }
def self.score(word) # 'dog'
word = word.upcase
word_array = word.split('') #['D', 'O', 'G']

Choose a reason for hiding this comment

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

I like that you're using comments here to keep track of what format the word will be in at each stage. This also makes the code easy to understand when you're reading it for the first time.


if word_array.length > 7 || word_array.length < 1
return total_score = nil
else

Choose a reason for hiding this comment

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

On line 20, you don't need to set total_score, you can say return nil.

total_score += 10
else
total_score = nil
end # case letter

Choose a reason for hiding this comment

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

On line 46, this will cause problems if there's a bad letter in the middle of a word, like 't3st'. Once you've set total_score = nil, trying to add to it will result in an error. Instead you should return nil here, bail out of the method entirely.

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

num.times do
random_num = rand(26) # Choose a letter at random
@letter_array[random_num]
@tile_bag << @letter_array[random_num][0] #Add letter to tile bag

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! Maybe this is fine? 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.

@letter_array[random_num][1] -= 1 # Reduce number of tiles by 1
if @letter_array[random_num][1] == 0 # Delete letter from list if 0 tiles left
@letter_array.delete_at(random_num)
end

Choose a reason for hiding this comment

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

There's a bug here! You remove items from @letter_array, but always do rand(26) to figure out what index to use, which means that once the bag is getting low on letters you're likely to go past the end of what you have left.

end

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

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.

describe "#won?" do
it'if player has 100 or more points return true' do

player = Scrabble::Player.new("player")

Choose a reason for hiding this comment

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

Good job catching both sides of this condition. I would be interested in looking at 100 points specifically: what does your program do at 99 points, at 100, and at 101? Where is the cutoff? This is the sort of question tests are well-situated to answer.

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