Code Critique Request

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

[James Edward Gray II <james@grayproductions.net>, 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.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

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

···

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

[Carlos <angus@quovadis.com.ar>, 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.

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

···

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.

[James Edward Gray II <james@grayproductions.net>, 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

[James Edward Gray II <james@grayproductions.net>, 2004-08-27 16.07 CEST]

>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

···

On Aug 27, 2004, at 8:43 AM, Carlos wrote:

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

···

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