Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Ruby > Code Critique Request

Reply
Thread Tools

Code Critique Request

 
 
James Edward Gray II
Guest
Posts: n/a
 
      08-26-2004
I've mentioned it before, but if you don't know Perl programmers
maintain a "Quiz of the Week". If you want to know more, the page is
here:

http://perl.plover.com/qotw/

This week's Regular Quiz (there also an Expert Quiz) came out last
night. The curious can read it here:

http://article.gmane.org/gmane.comp....f-the-week/105

*** SPOILER WARNING ***

Stop reading this message if you read the Quiz and would like to solve
it yourself, without seeing a solution first.

Okay, now to my point, finally...

I've solved the Quiz in Ruby. If you've seen my earlier posts, you
know I'm brand new to Ruby. I've spent the last week and a half
learning it and this is really my first attempt to put it to use.

If any of you can spare the time, I would appreciate an honest review
of the code I generated. I'm interested in any opinions you might have
about constructs, style, whatever. I'm especially looking for things
I'm handling in a non-Ruby fashion. Break my bad habits before they
take hold... <laughs>

Thanks in advance for sharing your time. Here's the code:

#!/usr/bin/ruby -w

require "getoptlong"

class Event
attr_reader :category, :day, :month, :year, :name

def Event.parse( string, cat )
if string =~ /^\s*(?\d+)\/)?(\d+)(?:\/(\d+))?\s+(.+?)\s*$/
return Event.new( cat, $4, $2, $1, $3 )
else
return
end
end

def initialize( cat, event, dd, mm = nil, yyyy = nil )
@category = cat
@name = event

@day = dd.to_i
@month = mm.to_i
@year = yyyy.to_i
end

def <=>( other )
if not year.zero? and not other.year.zero? and year != other.year
return year <=> other.year
elsif not month.zero? and not other.month.zero? and month !=
other.month
return month <=> other.month
else
return day <=> other.day
end
end

def between?( min, max )
times = [ min ]
until times[-1].year == max.year and times[-1].month == max.month and
times[-1].day == max.day
times << times[-1] + (60 * 60 * 24)
end

result = times.find do | time |
(year == 0 or year == time.year) and
(month == 0 or month == time.month) and
day == time.day
end
if result
times.index result
else
return
end
end

def date
date = day.to_s

unless month.zero?
date = month.to_s + "/" + date
end
unless year.zero?
date += "/" + year.to_s
end

return date
end
end

delta = 7
GetoptLong.new( [ "-n", GetoptLong::REQUIRED_ARGUMENT ] ).each do |
opt, arg |
delta = arg.to_i if opt == "-n"
end

NOW = Time.now
LIMIT = NOW + (delta * 24 * 60 * 60)

OFFSETS = [ " ===>", " -->", " -->", " -->", "-->", "->", ">", "" ]

DIR = File.join ENV["HOME"], '.upcoming';

events = { }

Dir.foreach DIR do | file_name |
next if file_name =~ /^\./

File.open File.join( DIR, file_name ) do | file |
while line = file.gets
e = Event.parse line, file_name
if e
events[file_name] = [ ] unless events.key? file_name
events[file_name] << e
end
end
end
end

puts
events.each do | key, value |
next unless value.find { | e | e.between? NOW, LIMIT }

puts key
value.sort.each do | e |
offset = e.between? NOW, LIMIT
next unless offset

case offset
when 0..7
printf "%-7s ", OFFSETS[offset]
else
printf "%-7s ", "(" + offset.to_s + ")"
end
printf "%-10s %s\n", e.date, e.name
end
puts
end

__END__

James Edward Gray II



 
Reply With Quote
 
 
 
 
Carlos
Guest
Posts: n/a
 
      08-27-2004
[James Edward Gray II <(E-Mail Removed)>, 2004-08-26 22.37 CEST]
...
> This week's Regular Quiz (there also an Expert Quiz) came out last
> night. The curious can read it here:
>
> http://article.gmane.org/gmane.comp....f-the-week/105

...
> If any of you can spare the time, I would appreciate an honest review
> of the code I generated. I'm interested in any opinions you might have
> about constructs, style, whatever. I'm especially looking for things
> I'm handling in a non-Ruby fashion. Break my bad habits before they
> take hold... <laughs>


Hi, James:

I didn't try your code, but I've seen that you don't use the builtin Date
class, neither you have an array with the month's length. So I guess your
program will think that an event scheduled for the 31st of the month will
happen tomorrow, if today is the 30th April. (Sorry if I'm mistaken.)

By the way, I would solve this in the non-object-oriented (and maybe
non-rubyesque) way of putting the events in nested hashes, as in the
following sketch:

events = {
# first level: days
1 => {
# second level: months
"*" => [ "Pay day" ],
1 => {
# third level: years
"*" => [ "New year", "First day" ],
2005 => [ "First day of 2005" ]
},
2 => ...
},
2 => ...
}

And later, retrieve the events by looping so:

d = Date.today
# n is the parameter
n.times do
e = [ events[d.day]["*"] ]
if x = events[d.day][d.mon]
e += [ x["*"], x[d.year] ]
end
e.flatten!
...etc...
d += 1
end

HTH



 
Reply With Quote
 
 
 
 
James Edward Gray II
Guest
Posts: n/a
 
      08-27-2004
On Aug 27, 2004, at 7:56 AM, Carlos wrote:

> I didn't try your code, but I've seen that you don't use the builtin
> Date
> class, neither you have an array with the month's length. So I guess
> your
> program will think that an event scheduled for the 31st of the month
> will
> happen tomorrow, if today is the 30th April. (Sorry if I'm mistaken.)


Thanks for the interest.

Originally, I was actually trying to solve it with Date, but I was
having a heck of a time trying to make that work. The Quiz
specification allows the event files to leave out a year, or worse a
month. When the year is missing, the event occurs every year on the
given date. If the month is missing, it's every month. This made it
hard to simply parse and compare dates, because you would have to try
several values for these events. It's very possible I just couldn't
see the easy answer here, but this approach defeated me.

I do THINK my code handles dates correctly, but I've been wrong before.
<laughs>

I solved the above problem by generating a list of days (with Time
objects) we are looking at for the calendar and then checking if an
event could occur on that day. That was easier to get my head around.

Thanks again for the comments though and the sample code. I like your
output code, so I'm off to see just how it works...

James Edward Gray II



 
Reply With Quote
 
Carlos
Guest
Posts: n/a
 
      08-27-2004
[Carlos <(E-Mail Removed)>, 2004-08-27 14.56 CEST]

Hi again:

> I didn't try your code, but I've seen that you don't use the builtin Date
> class, neither you have an array with the month's length. So I guess your
> program will think that an event scheduled for the 31st of the month will
> happen tomorrow, if today is the 30th April. (Sorry if I'm mistaken.)


You DO use Time (and Date is not builtin). That should tell you about my
merits as a reviewer and rubyist .

Anyway, don't use Time: you will have problems when you add 86400 seconds to
advance a day if it's near midnight and on the day when the summer time
changes.



 
Reply With Quote
 
James Edward Gray II
Guest
Posts: n/a
 
      08-27-2004
On Aug 27, 2004, at 8:43 AM, Carlos wrote:

> Anyway, don't use Time: you will have problems when you add 86400
> seconds to
> advance a day if it's near midnight and on the day when the summer time
> changes.


You bring up a good point, but is this true...

Time keeps track of it's value in seconds since the Epoch right? Then
just conveniently answers questions about what day that number of
second would fall on, well if I understand it correctly anyway.

My program just pushes the number of seconds forward. Then Time should
answer my questions based on the new count, taking into account things
like Daylight Savings Time, Leap Year, etc., right?

Again, I could be very wrong. That's just how I think it works.

James Edward Gray II



 
Reply With Quote
 
Carlos
Guest
Posts: n/a
 
      08-27-2004
[James Edward Gray II <(E-Mail Removed)>, 2004-08-27 15.31 CEST]
...
> I do THINK my code handles dates correctly, but I've been wrong before.


I do, too, after actually reading the code ) (taking into account the
"adding seconds" issue).

> I solved the above problem by generating a list of days (with Time
> objects) we are looking at for the calendar and then checking if an
> event could occur on that day. That was easier to get my head around.


I think it is a good approach. How about building the list beforehand,
instead of on each call of #between? ?

You could add a method to see if two events fall on the same day, and since
you used the same attribute names, it will work also for Time objects:

def same_day? other
return (year==0 || other.year==0 || year==other.year) &&
(month==0 || other.month==0 || month==other.month) &&
(day==other.day)
end

Later, you can retrieve the events by using:

events.each do |key, value|
result = list_of_times.map {|t| value.find_all {|e| e.same_day? t } }
next if result.all? {|r| r.empty? }
puts key
result.each_with_index {|r,i|
next if r.empty?
print( i<7 ? OFFSET[i] : "(#{i})" )
...etc...
end

(not tested)

Just another approach... I see that your Events class is more generic with
its #between? method.

HTH


 
Reply With Quote
 
Carlos
Guest
Posts: n/a
 
      08-27-2004
[James Edward Gray II <(E-Mail Removed)>, 2004-08-27 16.07 CEST]
> On Aug 27, 2004, at 8:43 AM, Carlos wrote:
>
> >Anyway, don't use Time: you will have problems when you add 86400
> >seconds to
> >advance a day if it's near midnight and on the day when the summer time
> >changes.

>
> You bring up a good point, but is this true...
>
> Time keeps track of it's value in seconds since the Epoch right? Then
> just conveniently answers questions about what day that number of
> second would fall on, well if I understand it correctly anyway.
>
> My program just pushes the number of seconds forward. Then Time should
> answer my questions based on the new count, taking into account things
> like Daylight Savings Time, Leap Year, etc., right?


It does, but saving time breaks the continuity. If x seconds from the epoch
means 15:00:00, x+1 seconds could mean 16:00:01.

Look:

irb(main):001:0> t=Time.mktime 2004,"mar",27,23,30,0
=> Sat Mar 27 23:30:00 CET 2004
irb(main):002:0> t+86400
=> Mon Mar 29 00:30:00 CEST 2004


 
Reply With Quote
 
James Edward Gray II
Guest
Posts: n/a
 
      08-27-2004
On Aug 27, 2004, at 10:01 AM, Carlos wrote:

> irb(main):001:0> t=Time.mktime 2004,"mar",27,23,30,0
> => Sat Mar 27 23:30:00 CET 2004
> irb(main):002:0> t+86400
> => Mon Mar 29 00:30:00 CEST 2004


Yuck. Excellent point. Thanks for the lesson.

Remember the fundamental rule of computing, "Nothing with dates is
easy." <laughs>

Ironically, I also did the "Expert Quiz" this week and it was much
easier for me. Go figure.

James Edward Gray II



 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Critique Request: CheckBoxColumn Fao, Sean ASP .Net 0 02-15-2006 05:09 PM
rubynuby code critique request Jeff Dickens Ruby 2 12-08-2003 12:34 PM
code critique request rihad C Programming 4 10-22-2003 06:44 AM
Critique request: x01 Andrew Cameron HTML 53 09-17-2003 08:56 PM
critique request Cynthia Turcotte HTML 7 09-13-2003 07:33 PM



Advertisments