-
Notifications
You must be signed in to change notification settings - Fork 74
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
. #75
base: master
Are you sure you want to change the base?
. #75
Conversation
lib/tree.rb
Outdated
end | ||
|
||
def add_apples | ||
rand(5..15).times do @apples.push(Apple.new()) end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple ruby style points in here:
- For blocks that fit on a single line, the preferred syntax is
{ @apples.push(Apple.new()) }
.do
-end
is preferred for multi-line blocks. - It's standard to omit the option
()
for method calls with no arguments.
@@ -61,14 +68,13 @@ def tree_data | |||
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.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is doing integer division. Is that what is intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant to do. Am I doing it incorrectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose you a had basket with two apples in it, with diameters of 7 and 8, respectively. What would you expect the average diameter to be? What does this code say the average diameter is?
expect(tree.age).to eq(1); | ||
expect(tree.apples).to eq(0); | ||
expect(tree.alive).to eq(true); | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice test
describe Fruit do | ||
it 'should be a Class' do | ||
expect(described_class.is_a? Class).to eq true | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of test is quite possibly not worth having in a project. If Fruit
isn't a class, then all kinds of stuff should be blowing up in your tests.
it 'apple should be a an instance of Apple and subclass of Fruit' do | ||
apple = Apple.new() | ||
expect(apple).to be_a_kind_of(Fruit) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you find yourself putting 'and' in the description of your test, then you should consider writing two tests instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to be two tests, one for Apple < Fruit, and one for apple to be_an_instance_of(Apple)
:-) |
No description provided.