Hi --

On Sat, 25 Jul 2009, Greg Willits wrote:

> I find myself often wondering about some design choices that result from
> code which looks similar to the contrived example below.
>
> class Places
>
>  attr_reader :addresses
>
>  def initialize(some_source)
>    @addresses     = []
>    @raw_addresses = []
>
>    load_addresses(some_source)
>  end
>
>  def load_addresses(some_source)
>    some_source.each do |this_source|
>      @raw_addresses << this_source.address
>    end
>    clean_addresses
>  end
>
>  private
>
>  def clean_addresses
>    @raw_addresses.each do |this_address|
>      @addresses << GisTools.normalize_address(this_address)
>    end
>  end
> end
>
> Testing for the final results of @addresses is easy as the instance var
> is exposed through the attr_reader, but there's two problems:
>
> A) Testing clean_addresses is dependent on running load_addresses first
> in order to acquire data.
>
> B) Seeing the intermediate results of @raw_addresses is not possible for
> testing load_addresses independently because @raw_addresses is not
> exposed.
>
> To fix (A), we could insist on passing parameters. So, we would have:
>
>  def load_addresses(some_source)
>    some_source.each do |this_source|
>      @raw_addresses << this_source.address
>    end
>    clean_addresses(@raw_addresses) # <-----  passed explicitly
>  end
>
>  private
>
>  def clean_addresses(raw_addresses) # <----- passed explicitly
>    raw_addresses.each do |this_address|
>      @addresses << GisTools.normalize_address(this_address)
>    end
>  end
>
> That would allow clean_addresses to be tested independently and be fed
> data directly by the test which could be a good thing. Regardless of
> that advantage, I've often wondered if requiring the passing of instance
> vars as parameters really is a 'higher standard' or not. I see a lot of
> code both ways. I tend to believe it's better to be explicit, but admit
> it feels simpler in many cases to just use the hard-coded instance var.
> I find myself flip flopping (always a bad thing).
>
> Either way, I still can't see the results of @raw_addresses. I find
> myself having to expose :raw_addresses so that tests can see the
> results, but there really is no need for :raw_addresses to be exposed,
> and it shouldn't be a part of the interface.
>
> Questions:
> 1) Any consensus on the parameters issue (*always* pass params, or
> *usually*)?
> 2) What's the 'best practice' approach to seeing the results of
> @raw_addresses?

The key word, I think, is "interface", which in your example consists
of initialize and load_addresses. A year or so ago someone asked on
either this list or the Rails list about testing private methods, and
I said, yes, test all your private methods. David Chelimsky chimed in
with the observation that testing private methods is not generally
considered best practice; rather, the private methods are there to
help the class do what it has to do so that its public interface will
act correctly.

So I'm not sure you'd want to test clean_addresses directly. And the
fact that @raw_addresses isn't exposed is fine. Exposing instance
variables is just an implementation technique for attributes anyway.
You should be able to move instance variables around in various ways
without changing your tests -- whereas "attributes" are methods and
therefore part of the public interface.

What you really need to verify, in the large, is that calling
initialize with known input produces the expected array of addresses.
You could get more granular and unit-test load_addresses, since it's
also part of the public API. After that, you sink below the horizon of
implementation detail.

At least, that's my take on it.


David

-- 
David A. Black / Ruby Power and Light, LLC
Ruby/Rails consulting & training: http://www.rubypal.com
Now available: The Well-Grounded Rubyist (http://manning.com/black2)
Training! Intro to Ruby, with Black & Kastner, September 14-17
(More info: http://rubyurl.com/vmzN)