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/);

[download]

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) = @_;
        ...;
    }

[download]
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 ],
    );

[download]
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

In reply to Re: Critique by ihb
in thread Critique by phenom

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.