in reply to Critique

"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;
}

[download]

Replies are listed 'Best First'.
Re: Re: Critique
by phenom (Chaplain) on Dec 06, 2003 at 03:24 UTC
    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);

    [download]
    to produce the automatic "or" effect. Thanks again!!
[reply]
[d/l]
      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


      ****sigfilesigfilesigfilesigfilesigfilesigfilesigfilesigfile

      Perl has one Great Advantage and one Great Disadvantage:

      It is very easy to write a complex and powerful program in three lines of code.

      ********************************************

      The Needs of the World and my Talents run parallel to infinity.
[reply]
[d/l]
        No, it wasn't. I printed it from the original system and retyped it... Should have proof read it.
[reply]
Re: Re: Critique
by ihb (Deacon) on Dec 06, 2003 at 10:59 UTC

    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
[reply]
[d/l]
      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.
[reply]
Re: Re: Critique
by ysth (Canon) on Dec 08, 2003 at 00:19 UTC
    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))}

    [download]
    And if we ask really nicely, bart may write it in 68k assembler for us :)
[reply]
[d/l]
Re: Re: Critique
by Paulster2 (Priest) on Dec 07, 2003 at 14:09 UTC

    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!

    Paulster2

[reply]

In Section Seekers of Perl Wisdom