tree.rb runs, added a few minor tests. Need to work on testing#57
tree.rb runs, added a few minor tests. Need to work on testing#57Joehimes wants to merge 1 commit intopaircolumbus:masterfrom Joehimes:master
Conversation
sjreich
left a comment
sjreich
left a comment
There was a problem hiding this comment.
Looks pretty solid. I made some suggestions, especially on the tests.
| end | ||
|
|
||
| it 'should have height' do | ||
| tree = Tree.new |
There was a problem hiding this comment.
The idiomatic rspec way would be to use a let statement for tree here. Generally speaking, let statements are preferred over local variables.
There was a problem hiding this comment.
I kinda like local variables. ¯\_(ツ)_/¯
Extracting the repetition of tree = Tree.new is nice, but the clarity of the local variable has some benefit too.
| tree = Tree.new | ||
| tree.add_apples | ||
| expect(tree.apples).to_not eq [] | ||
| end |
There was a problem hiding this comment.
For a lot of these tests, you might do some thinking about what the subject of the test really is. Here, for instance, the subject is really the add_apples method, rather than the Tree class itself. So using a nested describe block would be a good way to represent that.
Also, think about whether this is the right test. Sometimes you might want to call add_apples on a tree that already had apples, in which case the fact that tree.apples is not empty is pretty meaningless.
| end | ||
|
|
||
| def any_apples? | ||
| @apples != [] |
There was a problem hiding this comment.
@apples.empty? would work too, and it a bit more readable IMO.
3 participants
I would love any and all feedback! Thanks!