How should this really be coded in the Ruby style?

[Glenn Smith]

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


def save_activitydates(change_sent, change_received, org, year)
return true if !(change_sent or change_received)

begin
activitydates = @params['activitydates']

if change_sent
if !defined?(activitydates['sent']) or
activitydates['sent'].strip.length == 0
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
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

true
rescue
false
end
end

 

[Bill Guindon]

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.

 

[Douglas Livingstone]

In particular, similar code sort of repeats itself, which makes me
wonder if it could be better structured (using a 'proc' maybe?).

Here's my play with it, though as Daniel said, you didn't include
tests so I'm not 100% sure it does the same thing as your code

def format(date)
unless date.to_s.strip.empty?
res = ParseDate.parsedate date
Time.local(*res)
end
end

def save_activity_dates(change_sent, change_received, org, year)

return true unless change_sent or change_received

dates = @params['activity_dates']

sent = format(dates['sent']) || Time.now
due = format(dates['due']) || Time.now + (21 * 86400) # Add
21 days by default
received = format(dates['received'])|| Time.now

Activitydate.new do |ad|
ad['org_id'] = org.id
ad['year'] = year
ad['sent'] = sent if change_sent
ad['due'] = due if change_sent
ad['received'] = received if change_received
end.save

end

Cheers,
Douglas

 

[Robert Klemme]

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?).

I can't really comment on the structure because I never used Rails. Some
minor remarks:

- Don't use defined? - it's superfluous typing and doesn't gain you a thing
IMHO. nil is treated as false in boolean context anyway.

- Bind (21 * 24 * 60 * 60) to a constant with meaningful name, this is
faster and saves you the comment

- On one hand you raise if saving fails but then you catch that exception
at the end and convert it to a boolean return value. That's inconsequent
(and probably imperformant). I prefer exceptions for such failures but in
that case the whole begin-rescue-end is superfluous. If you need the boolean
return, just do any of these

new_activitydates.save or return false
return false unless new_activitydates.save
(I like the first one best because it resembles most to natural language)

Anyway, over to the guru's.

G


def save_activitydates(change_sent, change_received, org, year)
return true if !(change_sent or change_received)

begin
activitydates = @params['activitydates']

if change_sent
if !defined?(activitydates['sent']) or
activitydates['sent'].strip.length == 0
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
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

true
rescue
false
end
end

Btw, I think the indentation got garbled because your original had tabs in
it - spaces are much safer.

Kind regards

robert

 

[Bill Kelly]

Btw, I think the indentation got garbled because your original had tabs in

it - spaces are much safer.

To be fair, its indentation was intact. It's just $#@#%#$ Outlook
Express that's too dain bramaged to display tabs reasonably.
(If you "Properties -> Message Source" it'll display properly.)


[P.S. This post should not be construed as an argument for or against
tabs. <grin>]


Regards,

Bill

 

[Glenn Smith]

Gents

All really excellent points/suggestions.

I like Douglas' use of the separate date format function which
obviously removes some of the repetitiveness.

Interesting how everybody picks up on the 'unless this do this' or 'do
this unless' code. I also played with a number of styles for these.
My thoughts were that I wanted to put 'what would happen usually'
first, and 'what would happen if it failed' second. ie.

'do this but if it fails do that'

or

'if !save then raise'

On the other hand I like the tidier English of 'raise unless save'.
Mmm, not sure which one to use now.

Other than that, it sounds like in this small example I'm not a
million miles away from writing things correctly. Thanks all.

G

Btw, I think the indentation got garbled because your original had tabs in
it - spaces are much safer.

To be fair, its indentation was intact. It's just $#@#%#$ Outlook
Express that's too dain bramaged to display tabs reasonably.
(If you "Properties -> Message Source" it'll display properly.)

[P.S. This post should not be construed as an argument for or against
tabs. <grin>]

Regards,

Bill

 

[Robert Klemme]

To be fair, its indentation was intact. It's just $#@#%#$ Outlook
Express that's too dain bramaged to display tabs reasonably.
(If you "Properties -> Message Source" it'll display properly.)

To be fair, OE actually displayed tabs in the original post correctly.
They got garbled during creation of the reply template.

[P.S. This post should not be construed as an argument for or against
tabs. <grin>]

IMHO it's widely known that tabs are treated differently depending on
platform and tool OE being not the only one with sub optimal handling.
Using spaces (in mails but also source code) usually saves the hassle of
formatting surprises....

Cheers

robert

 

[Jacob Fugal]

Interesting how everybody picks up on the 'unless this do this' or 'do
this unless' code. I also played with a number of styles for these.
My thoughts were that I wanted to put 'what would happen usually'
first, and 'what would happen if it failed' second. ie.

'do this but if it fails do that'

or

'if !save then raise'

On the other hand I like the tidier English of 'raise unless save'.

You can have the best of both worlds with 'or'.

save or raise "Couldn't save!"

This only works if save always returns a true value for success and
false value for failure (which from your examples, it looks like it
does).

Jacob Fugal