Orange Tree simulator

By 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)

    expect(orange_tree.send(:dead?)).to eq true
  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
  attr_reader :age, :height, :oranges

  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
    add_three_oranges_to_tree if reached_age?(FRUIT_BEARING_AGE)
  end

  def dead?
    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

  def add_three_oranges_to_tree
    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

Anyway, back to your questions:

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

  attr_reader :age, :height, :oranges

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

  def age!
    @age += 1
    add_oranges if fruit_bearing?
  end

  def dead?
    age >= LIFESPAN
  end

  def fruit_bearing?
    age >= FRUIT_BEARING_AGE
  end

  def height
    age * 2
  end

  private

  def add_oranges
    ORANGES_PER_YEAR.times { oranges << Orange.new }
  end
end

(note that this implementation still has the same issues as yours: It keeps growing and adding oranges even if it’s dead.)

Leave a Reply

Your email address will not be published. Required fields are marked *