Code Critique Request

[James Edward Gray II]

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.lang.perl.qotw.quiz-of-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

 

[Carlos]

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.lang.perl.qotw.quiz-of-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

 

[James Edward Gray II]

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

 

[Carlos]

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.

 

[James Edward Gray II]

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

 

[Carlos]

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})" )
...etc...
end

(not tested)

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

HTH

 

[Carlos]

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

 

[James Edward Gray II]

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