TreeTDD exercise dhatten #58
Conversation
| end | ||
|
|
||
| def age! | ||
| @age+=1 | ||
| @alive = false unless @age < 9 |
There was a problem hiding this comment.
@alive = false if @age > 9 is probably a bit clearer. if is generally clearer than unless, except when it isn't.
There was a problem hiding this comment.
* if is clearer than unless, unless it isn't. 😁
| @age+=1 | ||
| @alive = false unless @age < 9 | ||
| @height+=rand(20) | ||
| self.add_apples |
There was a problem hiding this comment.
The self shouldn't be necessary here - self is the default receiver for every command in ruby.
There was a problem hiding this comment.
Joe is right. The only time self. is needed is calling a setter method, like self.foo = "bar". Ruby will assume foo = "bar" is local variable assignment instead of calling the foo= method.
|
|
||
| def initialize | ||
| @height = 0 | ||
| @age = 0 | ||
| @apples = 0 |
There was a problem hiding this comment.
This makes it sound like @apples will be an array of apples. I'd say either switch the value or change the name to make clear that it's a count.
There was a problem hiding this comment.
Rupert ended up doing the array design, it works out much better overall
| diameter_sum = 0 | ||
|
|
||
| basket.each do |apple| | ||
| diameter_sum += apple.diameter | ||
| end | ||
|
|
||
| avg_diameter = # It's up to you to calculate the average diameter for this harvest. | ||
| avg_diameter = diameter_sum / basket.count |
There was a problem hiding this comment.
This seems to assume that users of the Apple class will always initialize it with a floating point diameter, as you did above.
| it 'should be a Class' do | ||
| expect(described_class.is_a? 'Class').to be_true | ||
| describe AppleTree do | ||
| context 'when AppleTree exists' do |
There was a problem hiding this comment.
I'm not sure adding this context helps to clarify anything.
| subject(:tree) { AppleTree.new } | ||
| it 'should be a Class' do | ||
| expect(described_class.is_a? Class).to be true | ||
| end |
There was a problem hiding this comment.
I know this is provided for you, but I really dislike this kind of test 😄 .
|
|
||
| it 'should have no apples' do | ||
| expect(tree.any_apples?).to eq false | ||
| end |
There was a problem hiding this comment.
For these two tests: if it's part of the required setup for these guys that this be a brand-new tree, then the setup should go with the test. The options are either to do the necessary setup inside the it block itself, or else to use a context/describe block.
(and some other tests below too)
|
|
||
| it 'should not be able to have an apple picked' do | ||
| expect{tree.pick_an_apple!}.to raise_error NoApplesError | ||
| end |
| tree.age! | ||
| } | ||
| expect(tree.dead?).to eq true | ||
| end |
There was a problem hiding this comment.
Not that you need to add it, but think about testing the flip side here too: this only shows that tens years of aging kills the tree - but maybe 9 or 8 years would also kill it.
|
|
||
| it 'should have a diameter' do | ||
| expect(apple.diameter).to eq 5 | ||
| end |
No description provided.