Code Critique Request

Discussion in 'Ruby' started by James Edward Gray II, Aug 26, 2004.

  1. 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, Aug 26, 2004
    #1
    1. Advertising

  2. James Edward Gray II

    Carlos Guest

    [James Edward Gray II <>, 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
     
    Carlos, Aug 27, 2004
    #2
    1. Advertising

  3. 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
     
    James Edward Gray II, Aug 27, 2004
    #3
  4. James Edward Gray II

    Carlos Guest

    [Carlos <>, 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.
     
    Carlos, Aug 27, 2004
    #4
  5. 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
     
    James Edward Gray II, Aug 27, 2004
    #5
  6. James Edward Gray II

    Carlos Guest

    [James Edward Gray II <>, 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})" )
    ...etc...
    end

    (not tested)

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

    HTH
     
    Carlos, Aug 27, 2004
    #6
  7. James Edward Gray II

    Carlos Guest

    [James Edward Gray II <>, 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
     
    Carlos, Aug 27, 2004
    #7
  8. 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
     
    James Edward Gray II, Aug 27, 2004
    #8
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Fao, Sean

    Critique Request: CheckBoxColumn

    Fao, Sean, Feb 15, 2006, in forum: ASP .Net
    Replies:
    0
    Views:
    518
    Fao, Sean
    Feb 15, 2006
  2. Cynthia Turcotte

    critique request

    Cynthia Turcotte, Sep 12, 2003, in forum: HTML
    Replies:
    7
    Views:
    478
    Chris Leonard
    Sep 13, 2003
  3. Andrew Cameron

    Critique request: x01

    Andrew Cameron, Sep 14, 2003, in forum: HTML
    Replies:
    53
    Views:
    1,371
    picayunish
    Sep 17, 2003
  4. rihad

    code critique request

    rihad, Oct 21, 2003, in forum: C Programming
    Replies:
    4
    Views:
    336
    rihad
    Oct 22, 2003
  5. Jeff Dickens

    rubynuby code critique request

    Jeff Dickens, Dec 6, 2003, in forum: Ruby
    Replies:
    2
    Views:
    115
    Robert Klemme
    Dec 8, 2003
Loading...

Share This Page