From: Bill Guindon <agorilla@...>
Date: 2005-04-09T04:42:05+09:00
Subject: Re: How should this really be coded in the Ruby style?

On Apr 8, 2005 3:12 PM, Glenn Smith <glenn.ruby@gmail.com> wrote:
> I'm now in the lucky position of finding myself coding in Ruby all
> day!  I've spent the last 4 or 5 days developing a 'rails' app which
> I'm hoping to demo to the customer on Thursday!
> 
> However, I'm still relatively new at Ruby, though have been a
> programmer for 20+ years.
> 
> The thing that is most worrying me at the moment is that when I write
> Ruby code, at the back of my mind I'm wondering if I'm writing it in
> the style of, say, a 'VB' programmer (which I have been for about 10
> years), when there is a better 'Ruby' style which I've not yet picked
> up.
> 
> Today I was writing a long bit of code and the longer it got the more
> worried that I was writing it incorrectly.  That is, it would be
> perfectly good style in the more traditional-style languages I use -
> C, VB, PL/SQL etc.  But everything I've read about Ruby suggests short
> functions, 'DRY', and an OO style.
> 
> Below is an example of code I've written today.  I'm not asking
> anybody to debug it (I know it has a problem or two in there
> somewhere) , solve problems, etc.  What I'm wondering is, can somebody
> glance at this code and say "yes you are writing ruby in the correct
> form' or 'no, you should really restructure this way or that way'.
> It's just one (private) method cut from a Rails controller class.
> 
> In particular, similar code sort of repeats itself, which makes me
> wonder if it could be better structured (using a 'proc' maybe?).
> 
> Anyway, over to the guru's.
> 
> G

I'm hardly a guru, but I'll toss in my $.02
I'm not really adding much, mostly use of 'unless' and 'empty?'
But by using them, you can make your code more like simple english.
 
>         def save_activitydates(change_sent, change_received, org, year)
>                 return true if !(change_sent or change_received)

return true unless change_sent or change_received
 
>                 begin
>                         activitydates = @params['activitydates']
> 
>                         if change_sent
>                                 if !defined?(activitydates['sent']) or
> activitydates['sent'].strip.length == 0

unless defined?(activitydates['sent']) || 
   activitydates['sent'].strip.empty?

had to change the 'or' to '||' to avoid the method missing error if it's nil.

>                                         date_sent = Time.now
>                                 else
>                                         res = ParseDate.parsedate(activitydates['sent'])
>                                         date_sent = Time.local(*res)
>                                 end
>                                 if !defined?(activitydates['due']) or activitydates['due'].strip.length == 0
>                                         date_due = Time.now + (21 * 24 * 60 * 60)       # Add 21 days by default

Might pay to set a constant for DAY_IN_SECONDS instead of calculating
it each run.

>                                 else
>                                         res = ParseDate.parsedate(activitydates['due'])
>                                         date_due = Time.local(*res)
>                                 end
>                         end
> 
>                         if change_received
>                                 if !defined?(activitydates['received']) or
> activitydates['received'].strip.length == 0
>                                         date_received = Time.now
>                                 else
>                                         res = ParseDate.parsedate(activitydates['received'])
>                                         date_received = Time.local(*res)
>                                 end
>                         end
> 
>                         new_activitydates = Activitydate.new do |ad|
>                                 ad['org_id'] = org.id
>                                 ad['year'] = year
>                                 ad['sent'] = date_sent
>                                 ad['due'] = date_due
>                                 ad['received'] = date_received
>                         end
> 
>                         if !new_activitydates.save
>                                 raise
>                         end

raise unless new_activitydates.save
 
>                         true
>                 rescue
>                         false
>                 end
>         end
> 
> --
> 
> All the best
> Glenn
> Aylesbury, UK
> 

Hope some of that helps. Welcome to Ruby, and good luck with your demo.


-- 
Bill Guindon (aka aGorilla)