Robby on Rails

Hi Robby

Would it be possible to extract the country codes out and use a method that determines whether or not a region code record wants invoices emailed right away or by third party? E.g.

if division.needs_invoices_processing?

But perhaps it’s a bad suggestion if you won’t be adding any more region codes or if all region codes need different behaviour from the rest….

What about using ActiveSupport::StringInquirer to achieve something like “division.name.australia?” or “division.name.usa?”?

First thought as I skimmed the article … often when I see ‘if’ statements I wonder about refactoring to move the behaviour to the objects … so you can just call division.process_invoices.

Hello Robby, What about using the “type” magic column and processing the custom behaviours in subclasses ?

If you’ve got such a short list and mostly use it for branching in your code, perhaps a few instance methods would go a long way towards (even more) readable code?

if division.canada?
# canada specific stuff
end

Would that work with case statements?

Maybe you have other reasons to keep doing things this way, but it sounds like case where Tell, Don’t Ask would improve your code. In this situation, I’d probably give a Division an after_initialize to include a country-specific module of code.

Or at the least, write ‘if division.invoice_by_email? ...’ because that’s the important question you’re asking that location is only a proxy for.

Hey guys,

Nice article. I had a question about performance though. How often are those constant definitions called? Since in a production environment the classes are cached, would it be executed once then live until the server is restarted?

Thanks, Chris

Why do you find yourself looking over the code to compare divisions? Could you write a tool to extract the ‘process_invoice’ someone suggested to display them all side-by-side?

I just really don’t like spreading logic out from the object into other scripts. Every time I’ve done it I’ve later realized that my code is clunky or hard to test or flaky because half the code for an object lives outside that object, making it harder to test and reason about.

In any case, this issue is tangential to your post, which is quite nice, and I don’t mean to be the “ur doin it rong!” guy. Thanks for sharing it.

Without diving into patternitis, this sounds like a case for the Strategy pattern

class Division 
...

  def process_invoice
    invoice_strategy.perform(self)
  end

...
end

1. a simple attr_accessor method which contains an InvoicingStrategy instance that was chosen when you initialised the division constant, or
2. a method which uses logic to decide which kind of invoicing strategy is required for that specific division (and/or even that request).

This way the code for processing invoices by email is kept as one (testable) class while the code for processing invoices by rube-goldberg machine is kept separately as one (testable) class, etc.

    %w[foo bar foz].each{|name| define_method(name+'?'){ region == name }}

We are in similar situation here. In our application, we have to support several countries (US, CA, UK) and in many places, the behavior is slightly different. When those are initially implemented, we used if Locale.current.canada? to encapsulate the logic. Later I created a LocaleAware class and LogicForDefault, LogicForCanada classes. So I could use if LocaleAware.support_cpni? and have all canada specific logic in one place to look and change. This has worked well in many cases. There are still some cases we feel it is probably better to leave the “if country” there though.

I’m actually going to comment on the specific technique you talk about here, as opposed to the overall design. :)

I’ve been burned in the past by defining top level constants in models that do ActiveRecord finds. The reason is that those finds get executed whenever the class gets loaded, instead of later on when the constant is actually needed. This means that your app ends up calling the database at times when you don’t expect it – for example when executing any rake task that happens to end up referring to Division.

This can also bite you if you ever want to decouple your app more from the database, in order to use something like UnitRecord for example. Oh, it also means its really hard to stub or mock those values in tests as well (hello Object.const_set nastiness as opposed to Division.stubs(:usa) )

I usually go right to class methods in approaches like this. Its just as clear, and its much more flexible – you can memoize it (or not), you can stub/mock like any other methods, and they don’t get loaded at class eval time.

I share Rob Sanheim’s concerns about loading models when the class gets loaded. Would be cleaner to just set the constants as the codes, like so:

class Division < ActiveRecord::Base
  # country codes
  AFRICA = 'XYU238'
  ASIA = 'XIUHR73'
  ...
  # rules for who gets what feed
  WANTS_XML_FEED = [USA, CANADA]
  WANTS_CSV_FEED = [AUSTRALIA, NEW_ZEALAND]

...and then instance methods can check against these codes:

def wants_xml_feed?
  WANTS_XML_FEED.include? code
end

def wants_csv_feed?
  WANTS_CSV_FEED.include? code
end

Controller methods then don’t have to contain rules about who gets what feed type, they just need to deliver as appropriate:

def feed
  ...
  if division.wants_xml_feed?
    # deliver xml
  elsif divison.wants_csv_feed?
    # deliver csv
  else
   # send email
  end
end

This code, to me, is absolutely crying out for a little polymorphism. If there are only ten of these divisions, why even keep them in the database? Do their attributes ever need to be changed at run-time?

Mightn’t it be easier to just have POROs like

Division::Canada Division::UK Division::Africa

etc and put the specific behaviour into each concrete class?

I know what you mean about being able to see all the behaviour of each different division side-by-side, but this kind of conditional logic is soooooo prone to errors. See http://www.antiifcampaign.com/ if you want to learn more.

Just be aware that this method won’t work with transactional fixtures unless you have them preloaded—ActiveSupport::TestCase loads model class when you make a call to “fixture”, and doesn’t populate the tables until right before running a test case.

Furthermore, although they are also used by the user to keep feet warm between discount ugg cardy boots waves catch should not be used to get into the waves. In fact, Ugg boots should be worn to be in other conditions of humidity, mud, rain, ice or mud. discount ugg boots To avoid unexpected weather damage Ugg, you should apply repellent thoroughly with the frequency prescribed. This will seal the sheep’s clothing, dirt and stains well. wholesale ugg boots The best product for this is Australia sheepskin UGG water and stain.

I can always get much useful information here. I hope I can know more information here. Many thanks.

Nice post. Hope to see more fresh ideas here. Thanks a lot.

thanks a lot for sharing and hope more

nike shox ghd hair straightener china wholesale

nice post

I love this post.

ed hardy clothing

wholesale ed hardy

ed hardy boots