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

Fixing the Tree #64

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

Fixing the Tree #64

wants to merge 2 commits into from

Conversation

spudfkc
Copy link

@spudfkc spudfkc commented Mar 5, 2018

Copy link
Member

@jaybobo jaybobo left a comment

Choose a reason for hiding this comment

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

Looks good. I'd recommend reviewing better specs. It'll help with constructing readable, more easily maintained test.

http://www.betterspecs.org/


def initialize
attr_reader :height, :age, :apples
Copy link
Member

Choose a reason for hiding this comment

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

nice! not everything needs to be settable.



# Simple check if a tree has become too old.
if @age >= 10 && age + @prng.rand(0..10) > 20
Copy link
Member

Choose a reason for hiding this comment

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

nice use of the getter here.

class Apple < Fruit
attr_reader :color, :diameter

@@colors = ['red', 'yellow', 'green']
Copy link
Member

Choose a reason for hiding this comment

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

class variables arent recommended.

https://stackoverflow.com/questions/40819068/why-is-using-a-class-variable-in-ruby-considered-a-code-smell/40819118

instead you might use a constant or encapsulate them in a class method.

COLORS = %w(red yellow green).freeze
  def self.colors
    ['red', 'yellow', 'green']
  end

relatedly the cool thing about class << self is that you can use it to organize your private class methods in the same way you would handle instance methods.

class << self
  def foo
     bar
  end
  
  private

  def bar
    :baz
  end
end

12.times { tree.age! }
end

def new_test_tree
Copy link
Member

Choose a reason for hiding this comment

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

subject is a better option depending on what you're trying to convey.
you could also use let or let!` to accomplish this as well.

@@ -1,14 +1,137 @@
require 'rspec'
require 'tree'

def kill_tree(tree)
# The 12 here is based off of the rand_seed the below
12.times { tree.age! }
Copy link
Member

Choose a reason for hiding this comment

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

this works but its preferred to either accomplish this with a helper method or let. https://relishapp.com/rspec/rspec-rails/v/3-7/docs/helper-specs/helper-spec


describe '#any_apples?' do
it 'should return true when there are apples' do
tree = new_test_tree
Copy link
Member

Choose a reason for hiding this comment

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

the test tree can be setup as a variable placed in an outer context.

describe Tree do
  let(:tree) { described_class.new(...) }

  describe "#kill" do
    context "when tree is alive" do
      it "kills tree"
         tree.kill
         expect(tree.dead?).to eq true
      end
    end
  end
end

describe 'Fruit' do
describe Fruit do
it 'should have seeds when created' do
f = Fruit.new
Copy link
Member

Choose a reason for hiding this comment

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

127..128 could also be written with subject or described_class

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