Critique

[phenom]

Hello all. Recently a friend asked me to write a script to do the following:

1. user inputs start month/day, end month/day;
2. randomly choose breakfast, lunch, dinner every day during that span.
3. save to file

I won't bore you with why he wanted this, but here's what I came up with:

#!/usr/bin/perl

use warnings;
use strict;

my %months = ( 1 => "Jan", 2 => "Feb", 3 => "Mar", 4 => "Apr", 5 => "M
+ay", 6 => "June", 7 => "July", 8 => "Aug", 9 => "Sept", 10 => "Oct", 
+11 => "Nov", 12 => "Dec" );

my @breakfast = qw/bagel cereal toast yogurt/;
my @lunch     = qw/sandwich milkshake pb&j hoagie/;
my @dinner    = qw/pasta rice chicken steak/;

my $file = '';
my ($start_day, $start_mon, $end_day, $end_mon);
my ($total, $len, $curr_mon, $curr_day);
my $three_one = join ('|', qw/1 3 5 7 8 10 12/); #months with 31 days
my $three_zero = join ('|', qw/4 6 9 11/); #months with 30 days

print "Enter start month (ie., for January, enter 1): ";
    chomp($start_mon = <STDIN>);
print "Enter start day (ie., 20): ";
    chomp($start_day = <STDIN>);
print "Enter end month (ie., for March, enter a 3): ";
    chomp($end_mon = <STDIN>);
print "Enter end day (ie., 21): ";
    chomp($end_day = <STDIN>);
print "Save to file [hit enter for default 'plan']: ";
    chomp($file = <STDIN>);
if($file eq '') {
    $file = "plan";
}

open(FH, ">$file") or die "open: $!\n";

$curr_mon = $start_mon;
$curr_day = $start_day;

$total = get_days(); # get # of days for the current month

while (1) {
    last if($curr_mon eq $end_mon && $curr_day == $end_day + 1);
    print FH "$months{$curr_mon} $curr_day\n";
    $len = length($months{$curr_mon}) + length($curr_day) + 1;
    print FH "-" x $len, "\n";

    print FH "Breakfast: ", $breakfast[ rand @breakfast], "\n";
    print FH "Lunch    : ", $lunch[ rand @lunch], "\n";
    print FH "Dinner   : ", $dinner[ rand @dinner], "\n";
    print FH "-" x $len, "\n";

    $curr_day++;
    if($curr_day > $total) {
        $curr_mon++;
        $curr_day = 1;
        $total = get_days();
    }
}
print "Written to file $file.\n";
close(FH);

sub get_days {
    if($curr_mon =~ /$three_one/) {
        return 31;
    } elsif($curr_mon =~ /$three_zero/) {
        return 30;
    } elsif($curr_mon == 2) {
        return 29;
    } else {
        die "Bad month entered!\n";
    }
}

It works fine - it just seems long and not very neat. I'm still a beginner and was wondering how more experienced monks might have written it.

[BrowserUk]

This doesn't prompt for a filename, as I find it easier to just redirect the output via the command line.

#! perl -slw
use strict;
use Date::Manip;

sub prompt{ printf $_[ 0 ]; chomp( local $_ = <stdin> ); $_; }

my @breakfast = qw[ bagel cereal toast yogurt ];
my @lunch     = qw[ sandwich milkshake pb&j hoagie ];
my @dinner    = qw[ pasta rice chicken steak ];

my( $start, $end ) = ('')x 2;
$start = prompt 'Enter start date: ' until $start = ParseDate( $start 
+);
$end   = prompt 'Enter end   date: ' until $end   = ParseDate( $end   
+);

do{
    my $date = UnixDate( $start, '%F' );

    print $date, $/, '-' x length $date;
    print 'Breakfast: ', $breakfast[ rand @breakfast ];
    print 'Lunch    : ',     $lunch[ rand @lunch     ];
    print 'Dinner   : ',    $dinner[ rand @dinner    ];

    print '-' x length $date, $/;
} until(
    not Date_Cmp(
        $start = DateCalc( $start, '+1 day'  ),
        $end
    )
);

__END__
C:\test>stuff
Enter start date: 1st jan 2003
Enter end   date: 2003/02/01
Wednesday, January  1, 2003
---------------------------
Breakfast: toast
Lunch    : pb&j
Dinner   : rice
---------------------------

Thursday, January  2, 2003
--------------------------
Breakfast: bagel
Lunch    : hoagie
Dinner   : chicken
--------------------------

Friday, January  3, 2003
...

[ihb]

Nice code!

But unfortunately I found a bug. If $start is after $end then your routine will loop till death.

Just changing not to 0 <= won't do the whole trick though, as your construction still would print the meals for the first date. A regular for/while loop would suite better here.

This is a good example of why one shouldn't have == instead of <= or >= unless one has to in loop conditions.

ihb

[BrowserUk]

That's not a bug, that's GIGO :)

How could start be after end?

[ihb]

This is my take. It uses yet a different tool set than the two previous posts.

It does perhaps look a bit long, but that's because I've put some comments in the code to explain and motivate some of the code. Without the comments the code is 47 lines (including the empty lines). I admit though that some of the code I put there just to illustrate particular techniques and show what I believe is a healthy programming style.

    use strict;
    use warnings;

    use Date::Calc::Object qw/ Month_to_Text /;

    if (@ARGV < 2) {
        die "$0 STARTDATE ENDDATE FILE\n";
        # $0 is the program filename and the newline at the end
        # makes perl not report filename and line number of the
        # die call.
    }

    my ($start_str, $end_str, $file) = @ARGV;
    # I prefer to get all parameters directly and don't mess
    # with @ARGV (or @_ for subroutines) later on.
    # I also chose to get the dates by parameters rather than
    # prompting for it. I dislike prompting if parameter
    # passing will work.

    my $start = Date::Calc::Object::->new(split /-/, $start_str, 3)
        or die "Invalid start date\n";
    my $end   = Date::Calc::Object::->new(split /-/, $end_str, 3)
        or die "Invalid end date\n";
    # Note the colons last in the class name. That helps you
    # spot typos. (Try removing the use() line above.)

    my $fh;
    if (defined $file) { # Use defined() since '0' is a valid file nam
+e.
        open $fh, '>', $file or die "Couldn't open $file for write: $!
+";
        # Three-argument open is new for Perl 5.6, but recommended.
    }
    else {
        $fh = \*STDOUT; # A reference to a GLOB. See "perldoc -f print
+"
                        # and perldata section "Typeglobs and Filehand
+les".
    }
    # I chose to set STDOUT as default.

    # A slight change of data structure:
    my %meals = (
        breakfast => [ qw/bagel cereal toast yogurt/ ],
        lunch     => [ qw/sandwich milkshake pb&j hoagie/ ],
        dinner    => [ qw/pasta rice chicken steak/ ],
    );

    # The overloading of Date::Calc::Object objects makes
    # it easy to use dates as regular counters.
    for (my $date = $start->clone; $date <= $end; $date++) {
        my $d = substr(Month_to_Text($date->month), 0, 3) . ' ' . $dat
+e->day;
        print $fh "$d\n";
        print '-' x length $d, "\n";

        # Since I don't like to repeat myself I abstracted
        # the meal names.
        for (qw/ breakfast lunch dinner /) {
            printf $fh "%-9s: %s\n",
                ucfirst,
                $meals{$_}->[rand @{$meals{$_}}] # Note the arrow!
            ;
            # The arrow operator above is a dereferencing operator
            # and you can learn more about references and dereferencin
+g
            # in perlreftut and perlref.
            # @{$meals{$_}} is also a deref expression and you'll lear
+n
            # about that too in those two documents mentioned above!
        }

        print '-' x length $d, "\n\n";
    }

Hope I've helped,
ihb

[phenom]

Thank you all for your comments! The reasons I didn't use any modules is because the box I did it on doesn't have many and it's not connected to the net - not to mention I haven't learned many yet ;)

Anyway, I've learned some things from this, and appreciate your time and help!

[duff]

I probably would have converted the start/end month/day to GMT values and then iterated over them. Something like this:

#!/usr/bin/perl

use warnings;
use strict;
use POSIX qw(strftime);
use Time::Local;

my @breakfast = qw/bagel cereal toast yogurt/;
my @lunch     = qw/sandwich milkshake pb&j hoagie/;
my @dinner    = qw/pasta rice chicken steak/;

print "Enter start month (ie., for January, enter 1): ";
chomp(my $start_mon = <STDIN>);
print "Enter start day (ie., 20): ";
chomp(my $start_day = <STDIN>);
print "Enter end month (ie., for March, enter a 3): ";
chomp(my $end_mon = <STDIN>);
print "Enter end day (ie., 21): ";
chomp(my $end_day = <STDIN>);
print "Save to file [hit enter for default 'plan']: ";
chomp(my $file = <STDIN>);
$file ||= "plan";

my $start_gmt = timegm(0,0,0,$start_day,$start_mon-1,0);
my $end_gmt = timegm(0,0,0,$end_day,$end_mon-1,0);

open(FH, ">$file") or die "open: $!\n";
for (my $g = $start_gmt; $g <= $end_gmt; $g += 86400) {
    print FH my $d = strftime("%b %e", gmtime($g)), "\n";
    print "-" x length($d), "\n";
    print FH "Breakfast: $breakfast[rand @breakfast]\n";
    print FH "Lunch    : $lunch[rand @lunch]\n";
    print FH "Dinner   : $dinner[rand @dinner]\n";
    print "-" x length($d), "\n";
}
print "Written to file $file.\n";
close(FH);

There are all sorts of modules on CPAN dealing with dates and times that could have helped you out too.

[pg]

"it just seems long and not very neat"

I feel the opposite. Your code is neat and I like it. The logic is clear, easy to understand and easy to maintain. You have a good coding style.

Is it too long? No. You don't predetermine the size of your code. As long as it does the right thing in the right way, whatever it is long or short, that is the right size.

Although it is just a piece of demo code, I somehow would like it to be able to handle leap year:

sub get_days {
    if($curr_mon =~ /$three_one/) {
        return 31;
    } elsif($curr_mon =~ /$three_zero/) {
        return 30;
    } elsif($curr_mon == 2) {
        if (is_leap_year()) {
            return 29;
        } else {
            return 28;
        }
    } else {
        die "Bad month entered!\n";
    }
}

sub is_leap_year {
    my (undef, undef, undef, undef,undef, $year) = localtime(time);
    $year += 1900;
    return 1 if ($year % 400 == 0);
    return 0 if ($year % 100 == 0);
    return 1 if ($year % 4 == 0);
    return 0;
}

[phenom]

Thank you very much, pg. There were two extra things I was trying to use here for learning purposes: hashes and the lines pertaining to the regex under get_days. Hashes, since I understand the basics about them, but not more advanced stuff (like the array of hashes, hashes of hashes, blah blah blah); and also, I had just seen someone here recently use the following:

my $blah = join('|', @array);

to produce the automatic "or" effect. Thanks again!!

[xenchu]

Is the code in your note a copy of the actual code you ran? The reason I ask is that your month hash is misnumbered. The hash has two month 8's.

xenchu

[ihb]

You hit the head of the nail when you pointed out that it doesn't handle leap years. This is my only real problem with the code: it having its own date logic.

ihb

[phenom]

Since it was only supposed to deal with 2004, I never thought to include leap year functionality. But since some of the responses have given me some ways of doing it, I'm going to re-write it now. That's a good thing - I'm learning a lot from all these responses.

[ysth]

If we get rid of the clunky way too long variable names and use simple $m and $y, we can have:

sub get_days{$m-2?30+($m*3%7<4):28+!($y%4||$y%400*!($y%100))}

And if we ask really nicely, bart may write it in 68k assembler for us :)

[Paulster2]

While this works out very well, Date::Manip can handle it without putting in extra code. The perldoc on this goes to great lengths to explain everything about itself, also. It is a very well written and documented module, highly worth the time to check it out. I recommend it for all of your date manipulation needs!

UPDATE: I am refering to the Re: Critique written by pg above. Sorry about that!

[davido]

Here are a few snippets you can use as examples for "shortening" your code. Whether it improves your code is something you'll have to decide for yourself. Golfed down code isn't necessary more legible or readable.

#hash slice with a range
my %months;
@months{1..12} = qw/Jan Feb Mar Apr
                    May Jun Jul Aug
                    Sep Oct Nov Dec/;

You could write your "get_days()" function like this:

sub get_days {
    my $month = shift;
    return 31 if grep { $month eq $_ } qw/1 3 5 7 8 10 12/;
    return 30 if grep { $month eq $_ } qw/4 6 9 11/;
    return 29 if $month eq '2';
    die "Bad month entered!\n";
}

This method eliminates the use of global variables that intentionally leak into functions.

For more speed-efficient lookup tables hashes are preferable, but these are small lists, and you're probably not checking thousands of times a second, so grep is probably fine. Not sure why I chose to treat month numbers as strings. I guess because I usually think of hash keys as strings too.

Those are the biggest changes I can think of, and they're really not necessary. Just look at them as "Another Way To Do It"

[phenom]

I've never seen this before:

my %months;
@months{1..12} = qw/Jan Feb Mar Apr
                    May Jun Jul Aug
                    Sep Oct Nov Dec/;

Thanks, I like it.

[ihb]

Now you've gotten some constructive general critique, so I feel I can dissect your code now. :-)

At first I didn't feel like dissecting your code, as it seemed to work fine, but then I looked closer and found some issues I'd like to point out.

General comments:

It has its own date logic. Really, one shouldn't do this. It's too easy to have a flawed logic. pg pointed out the leap year issue, and I'll soon point out another.

It declares all variables at the top. This is not necesary in Perl, and generally avoided. Don't declare a variable until you need it, and do it as late as possible/suitable. This makes maintaining the code easier.

Details:

If you have a hash that only has positive integers as keys you might want to use an array instead.

    my @months = (undef, qw/Jan Feb Mar Apr May Jun Jul Aug Sep Oct No
+v Dec/);

About the subroutine &get_days. The first thing about it is that it uses a global variable instead of a parameter. It would be much preferable to see it written as

    sub get_days {
        my ($mon) = @_;
        ...;
    }

if I overlook that I would prefer it to not exist. ;-)

Unfortunately, the subroutine has a bug. If $curr_mon is 11 then it'll match the pattern '1', which is found in $tree_one. Generally, when using joined patterns like this, you want to first group it and then anchor it: /^(?:$three_one)$/.

If it wasn't for the leap year issue (or that this would be part of a date logic that already is handled in &Days_in_Month in Date::Calc) one could write it like this:

    my @months = (
        undef,
        [ Jan => 31 ],
        [ Feb => 28 ],
        [ Mar => 31 ],
        [ Apr => 30 ],
        [ May => 31 ],
        [ Jun => 30 ],
        [ Jul => 31 ],
        [ Aug => 31 ],
        [ Sep => 30 ],
        [ Oct => 31 ],
        [ Nov => 30 ],
        [ Dec => 31 ],
    );

and use $month[$curr_mon]->[0] to get the name, and $month[$curr_mon]->[1] to get the day count. But as said, don't do that in this particular case. I just wanted to show a nice way to couple data.

You can almost always discuss and elaborate on a piece of code till death, but now I feel there's just nit-picking left (as if I haven't been nit-picking already ;-)). Hope this've helped though, and given you new ideas of how to program in Perl.

ihb