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

Ana Lisa & Jamila: Octos-C9 #21

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions lib/player.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# require 'pry'
require_relative 'scoring'
module Scrabble
class Player
attr_reader :name, :played_words
def initialize(player_name)
@name = player_name
@played_words = []
end

def plays
return @played_words
end

Choose a reason for hiding this comment

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

Another way to approach this method would be to name the instance variable @plays and use an attr_reader.


def play(word)
@played_words << word

if won
return false
else
return Scrabble::Scoring.score(word)

end
end
# calculates the score of all the words
def total_score
word_scores = @played_words.map do |word|
Scrabble::Scoring.score(word)
end
# this will allow me to see if something inside a loop is not what I expect
# if word_scores.include?(nil)
# binding.pry
# end
return word_scores.inject(:+)

Choose a reason for hiding this comment

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

Beautiful!

end

def won
if total_score > 100
return true
else
return false

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


end

def highest_scoring_word
Scrabble::Scoring.highest_score_from(plays)

Choose a reason for hiding this comment

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

You're missing an end here - highest_scoring_word is defined inside of won. Turns out this is valid Ruby syntax, but it'd probably not what you wanted.


end


end
end
end
91 changes: 91 additions & 0 deletions lib/scoring.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,100 @@
require 'pry'
module Scrabble
class Scoring
LETTER_VALUES =
{
:A => 1,
:E => 1,

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!

:I => 1,
:O => 1,
:U => 1,
:L => 1,
:N => 1,
:R => 1,
:S => 1,
:T => 1,
:D => 2,
:G => 2,
:B => 3,
:C => 3,
:M => 3,
:P => 3,
:F => 4,
:H => 4,
:V => 4,
:W => 4,
:Y => 4,
:K => 5,
:J => 8,
:X => 8,
:Q => 10,
:Z => 10
}

def self.score(word)
# Check word for invalid characters
if !/[^a-zA-Z]+/.match(word)
# Split the word into its indvidual letters and store in array
user_word = word.upcase.split(//)
# Adds 50 points for 7 letter words
word_total = 0
# checks if word length is equal to seven and adds 50 pts
if user_word.length == 7
word_total += 50
elsif
# checks if word length is greater then 7, and returns nil
user_word.length > 7 || user_word.length == 0
return nil
end
# iterate over each letter
user_word.each do |letter|
letter = letter.to_sym
if LETTER_VALUES.has_key?(letter)

Choose a reason for hiding this comment

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

This check on line 52 is redundant! Above (line 36) you do a regex match, and here you check whether the LETTER_VALUES hash contains the letter. One or the other would suffice. I think I prefer the check here, since that makes it easy if you ever need to change the letters (maybe our version of scrabble starts allowing words with apostrophes).

word_total += LETTER_VALUES[letter]
end
end
return word_total
else
return nil
end
end


def self.highest_score_from(array_of_words)
# RETURNS NIL FOR EMPTY ARRAY
if array_of_words.length == 0
return nil
# RETURNS WORD FROM ARRAY LENGTH 1
elsif array_of_words.length == 1
return array_of_words[0]
else

Choose a reason for hiding this comment

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

Since both the if and elsif branches of this conditional have a return, you can omit the else entirely. It's not a huge difference, but it saves you some indentation.

scores = []
array_of_words.each do |word|
scores << Scrabble::Scoring.score(word)
end
highest_score = scores.max
highest_words = []
longest_word = nil
array_of_words.each do |word|
if Scrabble::Scoring.score(word) == highest_score
highest_words << word
end
# binding.pry
# sorts through words depending on length
longest_word = highest_words.sort_by do |word| word.length
if word.length == 7
longest_word = word
return longest_word

Choose a reason for hiding this comment

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

This loop does not select the best word. I think the core of the problem comes from the return statements - a return here doesn't return from the block, but bails out of the whole method! That means that as soon as you encounter a tie, you'll return (from the method) the first word. To demonstrate, here is a copy-paste from a pry session testing your code:

[9] pry(main)> Scrabble::Scoring.score('ada')
=> 4
[10] pry(main)> Scrabble::Scoring.score('ap')
=> 4
[11] pry(main)> # OK they're tied
[12] pry(main)> Scrabble::Scoring.highest_score_from(['ada', 'ap'])
=> "ada"
[13] pry(main)> Scrabble::Scoring.highest_score_from(['ap', 'ada'])
=> "ap"

Your implementation returns whichever word came first, when it should always prefer ap.

# if length is not 7 returns the shortest word
elsif word.length < 7
longest_word = word
return longest_word
end
end
end
return scores
total_score = scores.inject(0, :+)
end
end
end
end
158 changes: 158 additions & 0 deletions specs/player_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
require 'minitest/autorun'
require 'minitest/reporters'
require 'minitest/skip_dsl'

require_relative '../lib/player'



Minitest::Reporters.use! Minitest::Reporters::SpecReporter.new


describe 'Player' do
describe 'name' do
it 'can return a name' do
#Arrange
name = 'Gordon'
#Act/Assert
Scrabble::Player.new(name).name.must_equal 'Gordon'
end

# xit '' do
#
# end
end

describe 'plays' do
it 'it can return an array' do
player = Scrabble::Player.new('')
player.plays.must_be_instance_of Array
end

# xit '' do
#
# end
end

describe 'play(word)' do
# Feedback mentions testing edge cases for this method
it 'takes word and adds it to the plays array' do
# arrange
word = 'batshit'
player = Scrabble::Player.new('')

# Act
player.play(word)

# Assert
player.plays.must_include 'batshit'
end

it 'returns false if player has already won' do
# Arrange
word = "crazy"
word_1 = 'janky'
word_2 = 'coding'
word_3 = 'dancing'

Choose a reason for hiding this comment

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

Good words.

word_4 = 'hah'
player = Scrabble::Player.new('')

# Act
player.play(word)
player.play(word_1)
player.play(word_2)
player.play(word_3)

Choose a reason for hiding this comment

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

This code would be a little more clean and DRY if you stored the words in an array instead of a bunch of instance variables.

Also, I would argue that in this test, playing the first 4 words is part of the Arrange step, since it's only the last word that you're interested in.


# Assert
player.play(word_4).must_equal false

end

it 'returns score of the word' do
word = "guano"
Scrabble::Player.new('').play(word)

Scrabble::Scoring.score(word).must_equal 6

# Scrabble::Player.new('').play(word).score(word).must_equal 7
end
end

xdescribe 'total_score' do
xit 'Returns the sum of scores of played words' do
# Arrange
player = Scrabble::Player.new('')
word = "crazy"
word_1 = 'janky'
word_2 = 'coding'
word_3 = 'dancing'
word_4 = 'hah'

# Act
player.play(word)
player.play(word_1)
player.play(word_2)
player.play(word_3)
player.plays
player.total_score

# Assert
player.total_score.must_be Integer

Choose a reason for hiding this comment

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

You should also calculate the score and check the result.


end

xit '' do

end
end

describe 'won?' do
it 'if player has > 100 point score, return true' do
word = "crazy"
word_1 = 'janky'

Choose a reason for hiding this comment

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

Here are some other questions I would like to see answered by these tests:

  • What if the player has not played any words at all?
  • What if the player has < 100 points?
  • What if the player has exactly 100 points?

word_2 = 'coding'
word_3 = 'dancing'
word_4 = 'hah'
player = Scrabble::Player.new('')

# act
player.play(word)
player.play(word_1)
player.play(word_2)
player.play(word_3)
player.total_score

# Assert
player.won.must_equal true
end

xit '' do

end
end

describe 'highest_scoring_word' do
it 'Returns highest scoring played_word' do
word = "crazy"
word_1 = 'janky'
# word_2 = 'coding'
word_3 = 'dancing'
# word_4 = 'hah'
player = Scrabble::Player.new('')

player.play(word)
player.play(word_1)
# player.play(word_2)
player.play(word_3)

player.highest_scoring_word.must_equal 'dancing'


end

xit '' do

end
end
end
14 changes: 14 additions & 0 deletions specs/scoring_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,35 @@

describe 'highest_score_from' do
it 'returns nil if no words were passed' do
Scrabble::Scoring.highest_score_from('').must_be_nil
end

it 'returns the only word in a length-1 array' do
array_of_words = ['far']
Scrabble::Scoring.highest_score_from(array_of_words).must_equal 'far'
end

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

Choose a reason for hiding this comment

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

It might be wise to reverse the order of this array and check that you get the same result. You would be surprised at how often this sort of thing turns up a bug. This applies to all the tiebreaker tests in this section.

end

it 'if tied, prefer a word with 7 letters' do
array_of_words = ['dog', 'course', 'lungers']
Scrabble::Scoring.highest_score_from(array_of_words).must_equal 'lungers'
end

Choose a reason for hiding this comment

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

Since lungers gets the 7-letter bonus, I don't think you actually have a tie here. There isn't a pair of English words that allow you to test this logic, but zzzzzz and oratory will do.

"Why bother" you ask? Well, if Miriam-Webster adds zzzzzz as an onomatopoeia for snoring, we need to be prepared, or you could envision a scrabble app that works across languages or that allows users to add their own words. Moreover, maybe this is the mathematician in me speaking, but I believe there's beauty and joy to be found in writing code that isn't just "good enough", but which truly solves the problem.


it 'if tied and no word has 7 letters, prefers the word with fewer letters' do
# arrange
array_of_words = ['jot','ploys','cat']
# act/assert
Scrabble::Scoring.highest_score_from(array_of_words).must_equal 'jot'

end

it 'returns the first word of a tie with same letter count' do
array_of_words = ['jot','jet','ploy']
Scrabble::Scoring.highest_score_from(array_of_words).must_equal 'jot'
end
end
end