dave astels

A friend of mine, Misko Hevery, has written a very cool opensource tool for analyzing Java projects and scoring them in terms of how testable they are. His plans are to have it point out what’s wrong, and make suggestions as to what you can do to improve the situation.

Check it out: Testability Explorer

Well worth looking at.

I’m an OO bigot, plain & simple. One of my soapboxes/sacred-cows/hot-buttons/whatever is encapsulation… or more to the point, the lack of it.

I was writing some code recently and thought it might make a nice example.

I was just starting with a rough idea: I have a Form object that has FormPages. Pages can be moved around, added, deleted, etc. But they need to be ordered… pages of a form get filled out in some logical sequence.

So here’s my initial spec:

context "A form" do

  setup do
    @form = Form.new
    @form.form_pages << FormPage.new(:name => ‘one’, :number => 3)
    @form.form_pages << FormPage.new(:name => ‘three’, :number => 2)
    @form.form_pages << FormPage.new(:name => ‘two’, :number => 1)
  end

  specify “should be able to order its pages” do
    @form.ordered_pages.collect {|page| page.number}.should_eql [1, 2, 3]
  end
end

Some code to make this work:

class Form < ActiveRecord::Base
  has_many :form_pages

  def ordered_pages
    form_pages.sort
  end
end

class FormPage < ActiveRecord::Base
  belongs_to :form
  has_many :form_fields

  def <=>(other_page)
    number <=> other_page.number
  end
end

And that works just fine. BUT I’m totally violating any semblance of encapsulation. Look at FormPage for starters. We’re reaching into other_page and pulling out its number. Not so bad maybe.. it’s an instance of FormPage too. But it’s still breaking encapsulation. It can be done cleaner with a bit of double dispatch fun. Try this:

class FormPage < ActiveRecord::Base
  belongs_to :form
  has_many :form_fields

  def <=>(other_page)
    other_page.compare_number_to(number)
  end

  def compare_number_to(other_page_number)
    other_page_number <=> number
  end
end

That still works, and things are kept nicely encapsulated.

Now let’s look at the spec:

specify "should be able to order its pages" do
  @form.ordered_pages.collect {|page| page.number}.should_eql [1, 2, 3]
end

That collect is leaving a bloody trail in its wake as it hacks its way through the pages, ripping out numbers like the organ collectors in Monty Python’s “The Meaning of Life”. What are the alternatives? Well.. to support sorting we just wrote FormPage#compare_number_to(other_page_number). Let’s see if we can’t use that:

specify "should be able to order its pages" do
  @form.ordered_pages.each_with_index do |page, index|
    page.compare_number_to(index + 1).should_eql 0
  end
end

That does it. Notice that this works because we had that method to use, and the FormPages in the fixture were number sequentially.. we could clean it up even more by numbering the pages starting with 0 (which is much more ruby-esque anyway):

context "A form" do

  setup do
    @form = Form.new
    @form.form_pages << FormPage.new(:name => ‘one’, :number => 2)
    @form.form_pages << FormPage.new(:name => ‘three’, :number => 1)
    @form.form_pages << FormPage.new(:name => ‘two’, :number => 0)
  end

  specify “should be able to order its pages” do
    @form.ordered_pages.each_with_index do |page, index|
      page.compare_number_to(index).should_eql 0
    end
  end
end

Nice. We can go further… I don’t really care for the exposure of the array of pages. I’d feel better with an addPage method:

class Form < ActiveRecord::Base
  has_many :form_pages

  def add_page(aPage)
    form_pages << aPage
  end

  def ordered_pages
    form_pages.sort
  end
end

That lets us have our context setup be like this:

setup do
  @form = Form.new
  @form.add_page(FormPage.new(:name => 'one', :number => 2))
  @form.add_page(FormPage.new(:name => 'three', :number => 1))
  @form.add_page(FormPage.new(:name => 'two', :number => 0))
end

That’s much nicer from a pure OO perspective. “But,” you say, “that’s not overly Rails-like!” So… it IS very OO. It’s up to you where you draw the line. But remember… just because we can ignore encapsulation doesn’t mean we should. Doing so couples our code to the schema, and the goal of OO is to manage and minimize the coupling in our code. For a one-off, throw-away, 15 minute hack.. who cares.. but a mission critical application needs more thought & care. And it’s amazing how many one-off, throw-away, 15 minute hacks turn into mission critical apps.

David Chelimsky suggested making one more step and removing the reliance on the comparison method. After all, that is an implementation detail and relying on it from the spec makes it hard to change… just like any dependancy. So:

context "A form" do

  setup do
    @form = Form.new
    @pages_in_order = [
      FormPage.new(:id => 10, :name => 'two',  :number => 0),
      FormPage.new(:id => 5,  :name => 'one',  :number => 1),
      FormPage.new(:id => 13, :name => 'zero', :number => 2)
    ]
    @form.add_page(@pages_in_order[2])
    @form.add_page(@pages_in_order[1])
    @form.add_page(@pages_in_order[0])
  end

  specify "should be able to order its pages" do
    @form.ordered_pages.each_with_index do |page, index|
     page.should_equal @pages_in_order[index]
   end
  end
end

This final version is much better. I’ll blame my not seeing it on being sick at the time.

One of the core tenets of object-oriented programming is encapsulation. It’s one of OO’s most powerful ideas. More powerful than inheritance. Unfortunately it’s also one of the most ignored.

In “OO in One Sentence: Keep It DRY, Shy, and Tell The Other Guy” Andy Hunt & Dave Thomas (The Pragmatic Programmers) talk about good OO code being shy:

“The best code is very shy. Like a four-year old hiding behind a mother’s skirt, code shouldn’t reveal too much of itself and shouldn’t be too nosy into others affairs.

But you might find that your shy code grows up too fast, shedding its demure shyness in favor of wild promiscuity. When code isn’t shy, you’ll get unwanted coupling.”

Something I see all the time, on every team I’ve been involved with, is code like the following (classes are generalized from examples):

MyThing[] things = thingManager.getThingList();
for (int i = 0; i < things.length; i++) {
  MyThing thing = things[i];
  if (thing.getName().equals(thingName)) {
    return thingManager.delete(thing);
  }
}

This code is tightly coupled to the implementation of myThing in that it gets the name property, knows that it’s a string, etc. This is a classic approach from the old days or procedural programming. It is NOT OO.

How about:

MyThing[] things = thingManager.getThingList();
for (int i = 0; i < things.length; i++) {
  MyThing thing = things[i];
  if (thing.isNamed(thingName)) {
    return thingManager.delete(thing);
  }
}

This code still knows the details of myThing. Why should it? All it should know is how to ask a thing manager to delete a thing given its name:

return thingManager.deleteThingNamed(thingName);

thingManager is closer to MyThing, in fact they’re likely packaged together, so it’s not as bad for it to have somewhat more intimate knowledge of MyThing.

This is just one example of the approach of treating objects like new age data structures: Ask an object for information about its state, process that, make decisions based on that, then do something to/with the object.

In the above example, the code

• asks the ThingManager for its list of Things
• iterates through those Things
• asks each one for its name
• checks if the name is what it is looking for
• if so it asks the ThingManager to delete that thing

The proper approach is to ask the thingManager “Delete a thing named this if you have one. Let me know if you were able to do that.”

As a side note, writing code this way makes it much more understandable.

Do you write code like the first snippet? If so, you’re not doing OO! Stop telling people that you are. Or better, start doing OO. Better still, get your whole team doing OO.

A great article by Nat Pryce on encapsulation and information hiding.

http://nat.truemesh.com/archives/000498.html

When refactoring some code recently, my pair and I came across yet another reason why state based testing is bad.. and even more.. why testing using access to private state is incredibly bad.

We were doing some serious refactoring…. tackling a class that had become a catch-all… thousands of lines and several dozen methods… and splitting it up into multiple classes, each with a single responsibility.

We did this by creating new classes and moving groups of methods and associated instance variables to the new class, and leaving delegating methods in the original class. These we will clean up on the next pass.

We did this for one responsibility and suddenly tests that had been written long ago started to fail. This perplexed us since the original methods remainded in place in the original class, simply delegating to the real methods in the new class.

A quick exploration of the failing test revealed what was happening. The test in question was using reflection to get access to private fields in the class being tested. This went both ways… private fields were being set with fake values for purposes of the test, and assertions were being made on the values of private fields.

This is evidence of a serious misunderstanding of how you should be writing tests: they should be based on behaviour and function.. not state. An object’s state is simply an implementation detail, and should not be exposed to, or relied upon by, other objects. Guess what.. that means no getters & setters, either. But that’s a discussion for another time.

The problem that we ran into with accessing private fields in a test is that it makes the tests brittle, throwing a monkey wrench into the process of refactoring. When we were refactoring we made a change that did not effect the behaviour. That should NOT cause tests to break. But it did.

Why? Because those tests were accessing a private field of the class via reflection.. and we had moved that field to a new class.

Example:

Let’s start with the example from the first paper in the references section below:

class FieldTest {
  public String publicString = "Foobar";
  private String privateString = "Hello, World!";
}

We’ll pull in the test from that article as well (even if it is contrived):

...

FieldTest f = new FieldTest();
String s = (String)PrivateAccessor.getPrivateField(f, "privateString");
assertEquals("Hello, World!", s);

Now, say we add some behaviour that uses the private field:

public String makeHeader() {
  return "<h1>" + privateString + "</h1>";
}

so now we see some behaviour that doesn’t belong in the class (just go with me on this).. creating the header.. so we can extract that and the variable that only it uses to a new class:

class HeaderCreator {
  private String privateString = "Hello, World!";
  public String makeHeader() {
    return "<h1>" + privateString + "</h1>";
  }
}

class FieldTest {
  public String publicString = "Foobar";
  public String makeHeader() {
    return new HeaderCreator().makeHeader();
  }
}

Now the test that relies on accessing privateField will fail since we’ve, quite legitimately, moved it.

So, it’s a bad idea to write tests based on the state of an object, especially when that state is only available in private fields. That being said, don’t do it..and if you are.. stop doing it… and fix where you did do it.

References

1. Subverting Java Access Protection for Unit Testing by Ross Burton (NOTE: this article presents what NOT to do)
2. State vs Interaction Based Testing by Nat Pryce
3. Mocks Aren’t Stubs by Martin Fowler