From: Brian Candler <b.candler@...>
Date: 2009-07-25T21:58:07+09:00
Subject: Re: Unit Testing and Instance Vars

I'd agree with what David said. But I'd also point out that if you 
really want to test those methods independently, you have the option of 
using instance_variable_set and instance_variable_get - so you can set 
up the object directly into a known state (even if the public API does 
not allow you to do this), then run a method, then check the state 
(ditto).

Alternatively, you can refactor into a more functional style. You got 
part way with this:

  def clean_addresses(raw_addresses) # <----- passed explicitly
    raw_addresses.each do |this_address|
      @addresses << GisTools.normalize_address(this_address)
    end
  end

However you could take this further to:

  def clean_addresses
    @addresses = do_clean_addresses(@raw_addresses)
  end

  def do_clean_addresses(raw_addresses)
    addresses = []
    raw_addresses.each do |this_address|
      addresses << GisTools.normalize_address(this_address)
    end
    addresses
  end

Now do_clean_addresses is very easy to test, and you can also 
drastically simplify it using #map or #collect:

  def do_clean_addresses(raw_addresses)
    raw_addresses.map do |this_address|
      GisTools.normalize_address(this_address)
    end
  end

At which point, you realise that do_clean_addresses probably doesn't 
need a unit test, because it's just applying GisTools.normalize_address 
to a collection.

Now, you've probably spotted the subtle API change I've implemented 
here. Each time clean_addresses is called, @addresses is reset. In your 
original API you just appended to @addresses, so that means if you 
called clean_addresses more than once, @addresses would grow and grow.

It's up to you to decide whether this is what you actually want. If you 
do, you can revert to your original behaviour like this:

  def clean_addresses
    @addresses.concat(do_clean_addresses(@raw_addresses))
  end

Maybe that's irrelevant, since clean_addresses is a private method, and 
only called once at initialization time.

But in that case, it seems pointless to set @addresses and 
@raw_addresses to [] in your initialize method, before appending to them 
later, when load_addresses and clean_addresses could be responsible for 
initializing the instance variables too. Indeed, why not just have:

  def initialize(raw_addresses)
    @raw_addresses = raw_addresses
    clean_addresses(@raw_addresses)
  end

where clean_addresses is responsible for initializing @addresses as 
described above. (Another subtle change: @raw_addresses contains the 
same array as was passed in. Usually this is considered fine, but to get 
the exact same semantics as you had before, you can do @raw_addresses = 
raw_addresses.dup)

Another option to consider is memoisation. If clean_addresses is an 
expensive computation, then you can leave it until when it is needed.

  def initialize(raw_addresses = [])
    @raw_addresses = raw_addresses
    @addresses = nil
  end

  def addresses
    @addresses ||= clean_addresses(@raw_addresses)
  end

So 'addresses' looks like a read accessor for the @addresses variable, 
but it also does the cleaning "on demand" when first called.

Hope this gives you a few ideas.

Regards,

Brian.
-- 
Posted via http://www.ruby-forum.com/.