Orange Tree simulator

Posted on

Problem

I’m practicing SOLID principles and the concept of making a clear distinction between an object’s `private` and `public` interfaces.

I made the conscious choice to extract complex logic into `private` methods to hide the implementation details from the `public` interface. I did this because I heard that it’s best for `public` methods to clearly indicate what they’re doing so that others have an easier time figuring out what the program is doing.

I watched a talk by Ben Orenstein, during which he revealed that he does not test private methods very often. I don’t know if this is a good idea or not, but I trust his practices. How do you all feel about this? For those of you who agree with him, under what circumstances DO you test your private methods? How do you decide it’s important enough to test?

The specs:

``````require_relative '../lib/oranges'
require 'spec_helper'

describe OrangeTree do
let(:orange_tree) { OrangeTree.new }

it 'starts with an age of 0' do
expect(orange_tree.age).to eq 0
end

it 'starts with an height of 0' do
expect(orange_tree.height).to eq 0
end

it 'increases age by one when we call age!' do
orange_tree.age!

expect(orange_tree.age).to eq 1
end

it 'can be aged twice' do
age_by_years(2)

expect(orange_tree.age).to eq 2
end

it 'grows 2 feet each year' do
orange_tree.age!

expect(orange_tree.height).to eq 2
end

it 'dies after it reaches its lifespan' do
age_by_years(15)

end

it 'only has oranges' do
expect(orange_tree.oranges).to eq []
end

it 'only grows oranges after reaches 3 years old' do
assert_emptiness_when_young
orange_tree.age!

all_oranges?(orange_tree)
end

it 'can be picked' do
age_by_years(3)
orange_tree.oranges.pick!

expect(orange_tree.oranges.count).to eq 2
end

private

def all_oranges?(orange_tree)
orange_tree.oranges.each do |orange|
expect(orange).to be_an_instance_of(Orange)
end
end

def assert_emptiness_when_young
orange_tree.age!

expect(orange_tree.oranges).to eq []

orange_tree.age!

expect(orange_tree.oranges).to eq []
end

def age_by_years(number_of_years)
number_of_years.times { orange_tree.age! }
end
end
``````

The main program:

``````class Orange
end

class OrangeTree

LIFESPAN = 15
FRUIT_BEARING_AGE = 3

def initialize
@age = 0
@height = 0
@oranges = []
end

def age!
increase_age_by_one_year
increase_height_by_two_feet
end

reached_age?(LIFESPAN)
end

private

def reached_age?(requirement)
age >= requirement
end

def increase_age_by_one_year
@age += 1
end

def increase_height_by_two_feet
@height += 2
end

3.times { oranges << Orange.new }
end
end

class Array
def pick!
pop
end
end
``````

Any suggestions?

Solution

In some sense, the question of whether or not to test private methods might be a better fit for Programmers.SE. I’ll try to answer, but first a few notes on your code (since this is Code Review):

• It seems the tree keeps growing and keeps adding oranges after its death, which doesn’t seem quite right (though I’m sure the State of Florida would find it interesting!). A failing test for this could be:

``````it "stops growing once it's reached its lifespan" do
age_by_years(15)

expect {
orange_tree.age!
}.to_not change { orange_tree.height }
end
``````

You should probably add a similar one to check that the number of oranges stops increasing too.
You might also ask, if you never pick any oranges, do they just stick around from year to year? I mean, there’s ripe and then there’s withered on the branch ðŸ™‚

• Incidentally, the `expect {...}.to change {...}` test could be useful in a few places. If you want to check how much a value changed, you can add a `by` at the end, e.g.

``````it 'increases age by one when we call age!' do
expect {
orange_tree.age!
}.to change { orange_tree.age }.by 1
end
``````
• I’m not a fan of monkey patching `Array` in this case. While it seems neat to call `orange_tree.oranges.pick!`, extending a core class to do so is going too far (especially as it’s not even new functionality, just an alias). The semantics only make sense as longs as we’re talking about an array of oranges, but an array is a generic object that can be used for anything.
You might have an array of `OrangeTree` objects – does it make sense that you can call `orange_trees.pick!`? Maybe, yeah, if you got a bunch of oranges back. But this is just a cutesy alias for `pop`.
I might just add a `pick` method to `OrangeTree` instead. Maybe give it an argument to say how many to pick.

• The same way you have `#dead?` as a shortcut for calling `#reached_age?` with a certain value, I’d add a `#fruit_bearing?` method that works the same way. I don’t know if I’d bother with the `#reached_age?`method, though, as it’s so simple. It’s actually shorter to write `age >= ...`

• Your `#increase_age_by_one_year`, `#increase_height_by_two_feet`, and `#add_three_oranges_to_tree` methods are too specific in their naming. Sure, the names are accurate descriptions, but it’s like having magic numbers in your method names. What if you decide that it should grow 3 feet per year? Or that the number of oranges to add is derived from the tree’s age? Then the method names are no longer appropriate. I’d stick with just `#increase_age`, `#increase_height`, and `#add_oranges`.
As long as the increments are constant, though, it’d probably be good to use, well, constants, rather than hardcoded numbers.
The `#add_three_oranges_to_tree` method is also sort of redundantly named: What would it add oranges to, if not the tree?

• Speaking of, while you can pick oranges, and thus decrease their number, the height can’t decrease the same way. So there’s no need to keep track of it in an instance variable. Instead, you could just do:

``````def height
age * 2 # or, better yet, multiply by a named constant
end
``````

Of course, there’s still the question of what happens to the age and height, after the tree’s death. You might say that a 100-year-old tree is indeed 100 years old in terms of age – it’s just been dead for 85 years. But of course it shouldn’t then be 200 feet tall.

• Really minor thing, but if you intend for `Orange` to just be an empty class, you can make the extra clear by writing it in 1 line: `class Orange; end`

• As for your tests: What is `#assert_emptiness_when_young` doing as a method? It’s a test like all the others, not a helper. And you only use it in one place, so I don’t see why it’s separate. It’s also potentially misleading: You create a tree, and call `age!` once. But what if the fruit-bearing age was 5? Ageing it once would no longer be a good test. I’d simply write:

``````it "has no oranges when it's young" do
age_by_years(2)
expect(orange_tree.oranges).to be_empty
end
``````

Incidentally, it’d be nice to change that `#age_by_years` method. Age what by years? It’d be simpler to just have a method called `tree_aged` or `tree_with_age` that simply returns a tree with the given age.
That would let you simplify many of your tests, like the one above, to just:

``````  expect(tree_aged(2).oranges).to be_empty
``````

Usually, no, I wouldn’t test private methods. Tests are just code, and much like any other code that’ll use your class, it shouldn’t have to call private methods. That’s why they’re private. What’s important is that the public interface works as expected. And if it does, that indicates that whatever the class is doing in private must work as expected too.

Same as how you don’t need to poke around in a car’s engine as long as it starts and gets you to your destination just fine.

However, if your tests of the public interface are failing, and you want to go debugging, you might add a test of a private method to help you fix it. Again, the car analogy: If the car doesn’t work, your mechanic will want to look around in the engine.

Another case in which you might want to test private methods, is if your public interface is very simple but also very flexible and accepts all sorts of arguments (say, a great big hash of options). Checking every combination of arguments would take forever, and you still might miss some edge cases. But if the private methods each handle just one of those arguments, you can perhaps test those by themselves, and thus be reasonably satisfied that things will work with any combination of arguments. It’s not a common situation though, since you’ll generally want to avoid such complex “black box” constructions anyway.

So again, there isn’t a great need to test private methods. You can mess around with the class’ private methods, and re-run the public method tests, and it’ll tell you if things work as expect. Because again, if the public interface (which is what other code will be using) works, who cares about how the private methods work? Even if you’ve used a private method test to help you fix a problem, the real payoff should still be that the public method tests now act as intended. So if anything, you should add a few more of those, and scrap the private one.

Take the proposed test above that verifies that the tree doesn’t keep growing after its death. The test is still only using the public `#height` method/accessor. Since it’s failing, you’ll want to do something. You can add whatever logic you want to the public or private methods; when the tests pass, you know you’ve fixed that particular issue. No need to specifically test the private methods.

There’s also just basic decoupling. Since private methods aren’t meant for consumption, and should be able to change radically, as long as the public interface stays intact. For instance, since you’re not testing private methods right now, you could rewrite the class as below, and your current tests would still pass just fine.

``````class OrangeTree
LIFESPAN = 15
FRUIT_BEARING_AGE = 3
ORANGES_PER_YEAR = 3

def initialize
@age = 0
@oranges = []
end

def age!
@age += 1
end

age >= LIFESPAN
end

def fruit_bearing?
age >= FRUIT_BEARING_AGE
end

def height
age * 2
end

private