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: Zheng Cao & Nicoleta Brandolini #24

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

Conversation

nbrandolini
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 allows all members to share their codes by pushing to repo and access updated version of code base by pulling from repo. It is a very efficient way to collaborate remotely, which allows team members to work on problems individually on different computers and share their work after.
Why do you think we chose to make the score method in the Scoring class a class method instead of an instance method? It’s only tied to the class and not to any particular instance, so it made sense to make it a class method. Any player instance should be able to use it, without having to create a new score instance every time they play a word.
Describe an example of a test you wrote for a nominal case. For TileBag#draw_tiles, “it ‘Returns a collection of random tiles’ ” is a nominal case. The data type, length, and element of the returned array was checked.
Describe an example of a test you wrote for an edge case. For TileBag#draw_tiles, “it ‘Raises an ArgumentError if user inputs an invalid number’ ” is an edge case. Several invalid inputs (string, negative num, too large number) were tested. If the player draws more tiles than there are in the bag, the program will raise an Argument Error and also if the player tries to enter a negative number or not an integer.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? Yes, we used ‘reduce’ in Player#total_score to calculate the total score of words played. With this method, we can implement total_score with only one line of code.
What was one method you and your pair used to debug code? For most of bugs we encountered, we were able to find the cause by reading Error message. If this didn’t work, we would use “binding.pry” to debug.
Is there a specific piece of code you'd like feedback on? 1) Scoring.highest_score_from looks cumbersome to us. We would like to see a better way to implement this method. 2) Also, if we can get some feedback on the edge-cases we implemented, it will be helpful.
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) NB - Working with Zheng was very helpful for me, as I learned from her way of structuring the project first and then take it step-by-step. While she guided me through the project she was very patient. Her knowledge of the material is way above my knowledge at the moment. We took turns typing and working on different computers. 2) ZC - Sometimes, we got confused about nominal cases and edge cases for a method. The fact that we were able to utilize most of the new techniques (ternary conditional, post-fix conditional, git push and pull, debugging with Stack Traces, CONSTANTS, Enumerable’s built-in methods, and handling Exceptions) in our project made us feel really proud. :)

2020dream and others added 29 commits February 20, 2018 14:36
@CheezItMan
Copy link

Scrabble

What We're Looking For

Feature Feedback
General
Answered comprehension questions Check, I hope my solution code helped a bit, there are other solutions as well.
Both Teammates contributed to the codebase Both team member have commits
Regular Commits with meaningful commit messages Good number of commits and good commit messages
Scoring class
score method Check, but see my in-line note
Uses appropriate data structure to store the letter score Check
Appropriately handles edge cases Check
highest_score_from method Check
Appropriately handles edge cases Check
Test implementations for highest_score_from Check
Player class
Uses an array to keep track of words played Check
Uses existing code to create highest_scoring_word and highest_word_score Check
Returns true or false from won? method Check
Tests edge cases on the play(word) Check, but some issues with total_score
TileBag class
Uses a data structure to keep the set of default tiles in the bag Check, but see my notes on it.
draw_tiles method uses the parameter to draw a set of random tiles It pick random tiles, but doesn't remove them from the structure.
tiles_remaining method returns a count Check
Tests draw_tiles edge cases No edge-cases tested.
Summary Not bad, you had some trouble, specifically with TileBag. You also need to look a bit more closely for edge-cases. Let me know where you have questions.

@@ -1,5 +1,6 @@
*.gem
*.rbc
.vscode/

Choose a reason for hiding this comment

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

Interesting, you're using VS Code

module Scrabble
class Scoring

SCORE_CHART = {

Choose a reason for hiding this comment

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

Good way to store the letter scores, and nice use of a CONSTANT.

score = 0
word.each_char do |c|
return nil if !SCORE_CHART.keys.include?(c.upcase)
SCORE_CHART.each do |key, value|

Choose a reason for hiding this comment

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

You actually don't need this loop, you can instead just do:

score += SCORE_CHART[c.upcase]

Take advantage of the fact that this is a hash.

end

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

Choose a reason for hiding this comment

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

Nice, you considered the order!


# Returns the `highest_scoring_word` score
def highest_word_score
return Scrabble::Scoring.score(highest_scoring_word)

Choose a reason for hiding this comment

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

Well done

end
end

describe 'highest_scoring_word' do

Choose a reason for hiding this comment

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

What's the result of this method if nothing has been played.

end

describe 'draw_tiles' do
it "Fills tiles array until it has 7 letters from the given tile bag" do

Choose a reason for hiding this comment

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

You should also probably remove some tiles from the player's hand (like 2) and then call draw_tiles and see if it draws the right number of tiles (like 5).

"U" => 4, "V" => 2, "W" => 2, "X" => 1, "Y" => 2, "Z" => 1 }

def initialize
@tiles = []

Choose a reason for hiding this comment

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

This sets @tiles to an empty array and doesn't put any tiles in it.


attr_reader :tiles, :number_of_tiles

LETTER_QUANTITIES = {

Choose a reason for hiding this comment

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

This is great for the letter quantities, but you're using this hash to store the tiles in the bag. This is problematic for a couple of reasons.

  1. All instances of TileBag are sharing this constant. So if you remove a "z" from one TileBag instance, you've removed it from them all.
  2. This structure makes any letter equally likely to be drawn from the bag. However there is only 1 "z" and 12 "E"'s.

end
end

describe 'draw_tiles' do

Choose a reason for hiding this comment

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

What happens when you draw all the tiles?

Also you need to check that the number of each drawn tile has gone down.

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