Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

Module Code Review

by TStanley (Canon)
on Sep 15, 2001 at 03:58 UTC ( #112582=perlquestion: print w/ replies, xml ) Need Help??
TStanley has asked for the wisdom of the Perl Monks concerning the following question:

Lately I have been using a particular set of code over and over again, and I finally decided to put it into a module. I would like everyone's overall opinion of this code, and whether or not they think it might even be worthy of CPAN.
package Date::Lastday; $VERSION=0.01; use strict; ## GLOBALS ## my %Max_Days=("Jan"=>"31","Feb"=>"28","Mar"=>"31","Apr"=>"30", "May"=>"31","Jun"=>"30","Jul"=>"31","Aug"=>"31", "Sep"=>"30","Oct"=>"31","Nov"=>"30","Dec"=>"31"); my %Months=("01"=>"Jan","02"=>"Feb","03"=>"Mar","04"=>"Apr", "05"=>"May","06"=>"Jun","07"=>"Jul","08"=>"Aug", "09"=>"Sep","10"=>"Oct","11"=>"Nov","12"=>"Dec"); ## Functions ## sub new{ my $self=shift; return bless {} $self; } sub last{ my $self=shift; my ($Day,$Month,$Year)=(localtime)[3,4,5]; $Month=sprintf("%02d,($Month+1)); $Year=sprintf("%04d,($Year+1900)); ## Provide for leap years ## my $Leap=$Year % 4; if($Leap==0){ $Max_Days{"Feb"}=29; } if($Day==1 and $Month==1){ # Cover January 1st $Month=12; $Day=31; $Year=$Year-1; }elsif($Day==1){ # Cover the 1st of any month $Month-=1; $Month=sprintf("%02d",$Month); $Day=$Max_Days{$Months{$Month}}; }else{ $Day-=1; $Day=sprintf("%02d",$Day); } return $Month,$Day,$Year; } sub cal{ my $self=shift; my ($Day,$Month,$Year)=(localtime)[3,4,5]; $Month=sprintf("%02d,($Month+1)); $Year=sprintf("%04d,($Year+1900)); ## Provide for leap years ## my $Leap=$Year % 4; if($Leap==0){ $Max_Days{"Feb"}=29; } if($Day==1 and $Month==1){ # Cover January 1st $Month="Dec"; $Day=31; $Year=$Year-1; }elsif($Day==1){ # Cover the 1st of any month $Month-=1; $Month=sprintf("%02d",$Month); $Month=$Months{$Month}; $Day=$Max_Days{$Months}; }else{ $Day-=1; $Day=sprintf("%02d",$Day); $Month=$Months{$Month}; } return $Month,$Day,$Year; } 1; __END__; =pod =head1 NAME Date::Lastday - Perl extension to calculate previous day's date =head1 SYNOPSIS use Date::Lastday; my $Last=new Date::Lastday; my ($Month,$Day,$Year)=$Last->last(); ## OR ## my ($Month,$Day,$Year)=$Last->cal(); =head1 DESCRIPTION Date::Lastday is used to calculate the date of the previous day. It us +es the localtime function of Perl to get the date, then manipulate it + from there. The last function returns the month and day as 2 digit numbers, and th +e year as a 4 digit number. If you wish to have the month returned as + the 3 letter abbreviation (i.e. Jan, Feb, Mar, etc.), use the cal fu +nction. =head2 EXPORT None by default =head1 AUTHOR Thomas Stanley Thomas_J_Stanley@msn.com =head1 COPYRIGHT Copyright (C) 2001 Thomas Stanley. All rights reserved. This program i +s free software; you can redistribute it and/or modify it under the s +ame terms as Perl itself. =head1 SEE ALSO perl(1) =cut

TStanley
--------
There's an infinite number of monkeys outside who want to talk to us
about this script for Hamlet they've worked out
-- Douglas Adams/Hitchhiker's Guide to the Galaxy

Comment on Module Code Review
Download Code
Re: Module Code Review
by japhy (Canon) on Sep 15, 2001 at 04:24 UTC
    Your leap year calculation is incorrect. 2004 is a leap year, 2000 is a leap year, 1904 is a leap year, but 1900 is NOT. The rule is:
    • divisible by 4, and
    • not divisible by 100, or
    • divisible by 400
    if (!($y % 4) and ($y % 100 or !($y % 400))) { ... }
    Other than that, here's a much simpler method for finding out yesterday:
    use Time::Local; my $noon = timelocal(0,0,12, (localtime)[3,4,5]); my ($d,$m,$y) = (localtime($noon - 86400))[3,4,5];

    _____________________________________________________
    Jeff[japhy]Pinyan: Perl, regex, and perl hacker.
    s++=END;++y(;-P)}y js++=;shajsj<++y(p-q)}?print:??;

      You don't even need Time::Local,

      my $yesterday= do { my($y,$m,$d)= (localtime( time-60*60*(12+(localtime)[2]) ))[5,4,3]; sprintf "%04d-%02d-%02d", 1900+$y, 1+$m, $d; };
      or, just for fun:
      my $yesterday= join"-",mapcar{sprintf "%0".shift()."d",pop()+pop()} [4,2,2],[(localtime( time-60*60*(12+(localtime)[2]) ))[5,4,3]],[1900,1,0];

              - tye (but my friends call me "Tye")
Re: Module Code Review
by princepawn (Parson) on Sep 15, 2001 at 05:21 UTC
    The datetime@perl.org mailing list is reworking the entire Date/Time infrastructure now.

    Post your code there for a thorough critique.

Re: Module Code Review
by merlyn (Sage) on Sep 15, 2001 at 05:34 UTC
    I'm not sure why you just didn't use Date::Calc, particularly the Days_in_Month and Calendar functions.

    Using an object here seems a bit of an overkill. I'm not sure I'd ever subclass this, and there's no internal state that might change over time.

    -- Randal L. Schwartz, Perl hacker

Re: Module Code Review
by TStanley (Canon) on Sep 16, 2001 at 05:09 UTC
    Thank you all for your honest critiques. The reason I did a lot of this was primarily a learning experience for myself, especially in the area of object oriented programming. Learning how to put a module together and using it, is basically what I was trying to accomplish. And if the material didn't exist on CPAN (fat chance obviously :-) ), I would gladly add it to the repository.

    TStanley
    --------
    There's an infinite number of monkeys outside who want to talk to us
    about this script for Hamlet they've worked out
    -- Douglas Adams/Hitchhiker's Guide to the Galaxy

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://112582]
Approved by root
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others perusing the Monastery: (9)
As of 2015-07-07 09:58 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The top three priorities of my open tasks are (in descending order of likelihood to be worked on) ...









    Results (88 votes), past polls