Any suggestions for my programming style?

Discussion in 'Perl Misc' started by mahurshi, Dec 19, 2005.

  1. mahurshi

    mahurshi Guest

    Hi folks.. I write code as a hobby to keep me from getting bored. Of
    late, I have been trying to think of ways to improve my coding style.
    (organizing, commenting, indenting, error checking, uncluttering, etc)

    I don't want to be all anal but I want to write good code. (I know the
    word "good" is subjective)

    Anyway, I just wanted to see what you guys thought of this. And I
    would find it helpful if you gave me some suggestions regarding the
    same.

    I am pasting my code here. The program is supposed to find the day of
    the week for a given date.

    #!/usr/bin/perl -w

    #################################################
    ## this program has a set of functions
    ## that will help determmine the day of the week
    ## for a given day
    #################################################


    use Getopt::Std;
    getopt('mdy');


    ## this function returns a century code given a year
    sub get_century_code
    {
    my($year) = @_;
    my($century);

    $century = int($year/100);

    if ($century == 17) { return 4; }
    elsif ($century == 18) { return 9; }
    elsif ($century == 19) { return 0; }
    elsif ($century == 20) { return 6; }
    else
    { print "Can't find century code. Century = $century, Year =
    $year\n";
    print "Year should be between 1700 and 2099\n"; die; }

    }

    ## this function returns 1 if a given year is leap year
    ## it returns 0 otherwise
    sub is_leap_year
    {
    my($year) = @_;

    ## if year is divisible by 4
    if ($year%4)
    {
    ## if year is divisible by 100
    if ($year%100)
    {
    ## if year is divisible by 400
    if ($year%400) { return 1; }
    ## if not
    else { return 0; }
    }
    else { return 1; }

    }
    else { return 0; }
    }

    ## this function returns the month code given a month and a year
    sub get_month_code
    {
    my($month, $year) = @_;

    my($leap_year);
    $leap_year = is_leap_year($year);

    if ($month == 1)
    {
    if ($leap_year == 1) { return 0; }
    else { return 1; }
    }
    elsif ($month == 2)
    {
    if ($leap_year == 1) { return 3; }
    else { return 4; }
    }
    elsif ($month == 3) { return 4; }
    elsif ($month == 4) { return 0; }
    elsif ($month == 5) { return 2; }
    elsif ($month == 6) { return 5; }
    elsif ($month == 7) { return 0; }
    elsif ($month == 8) { return 3; }
    elsif ($month == 9) { return 6; }
    elsif ($month == 10) { return 1; }
    elsif ($month == 11) { return 4; }
    elsif ($month == 12) { return 6; }
    else { print "Can't find month code. (Invalid month?) Month = $month,
    Year = $year\n";
    die; }
    }

    ## this function checks the sanity of the given input
    sub check_sanity
    {
    my($month, $date, $year) = @_;
    my($leap_year);

    ## if year is out of range
    if ($year < 1700 || $year > 2099)
    { print "Please enter a year between 1700 and 2099\n"; die; }

    ## if month is out of range
    if ($month < 1 || $month > 12)
    { print "Invalid month!\n"; die; }

    ## if date is below 1
    if ($date < 1)
    { print "Invalid date!\n"; die; }

    ## if date is greater than 31 for some months
    if ($month == 1 || $month == 3 || $month == 5 || $month == 7 ||
    $month == 8 || $month == 10 || $month == 12)
    {
    if ($date > 31)
    { print "Invalid date!\n"; die; }
    }
    ## if date is greater than 30 for some months
    elsif ($month == 4 || $month == 6 || $month == 9 || $month == 11)
    {
    if ($date > 30)
    { print "Invalid date!\n"; die; }
    }
    ## for february
    elsif ($month == 2)
    {
    $leap_year = &is_leap_year($year);

    ## allow only upto 29 days for leap years and
    ## 28 days for non leap years
    if ($leap_year == 1)
    {
    if ($date > 29)
    { print "Invalid date!\n"; die; }
    }
    else
    {
    if ($date > 28)
    { print "Invalid date!\n"; die; }
    }
    }
    }

    ## function returns the year code given a year
    sub get_year_code
    {
    my($year) = @_;

    my($last_two_digits);
    my($divide_by_4_quotient);
    my($divide_by_7_remainder);
    my($sum);

    ## get the last two digits of the year
    $last_two_digits = $year % 100;

    ## divide year by 4 and take the quotient
    $divide_by_4_quotient = int($last_two_digits/4);

    ## add the results up
    $sum = $last_two_digits + $divide_by_4_quotient;

    ## divide by 7 and take the remainder
    $divide_by_7_remainder = $sum % 7;

    ## that's the result
    return $divide_by_7_remainder;

    }

    ## function returns a number (0 thru 6), 0 corresponding to saturday
    ## and 6 corresponding to friday
    ## function takes month, date, and year as arguments
    sub get_day_of_week
    {
    my($month, $date, $year) = @_;

    my($month_code);
    my($century_code);
    my($year_code);
    my($day_of_week);

    $month_code = &get_month_code($month, $year);
    $century_code = &get_century_code($year);
    $year_code = &get_year_code($year);

    ## add month code, century code, year code and date,
    ## divide by 7, and take the remainder
    $day_of_week = ($month_code + $century_code + $year_code + $date) % 7;

    return $day_of_week;
    }

    ## show usage information
    if (!defined($opt_m) || !defined($opt_d) || !defined($opt_y))
    { print "Usage: day_for_date.pl -m <month> -d <date> -y <year>\n"; die;
    }

    $day = get_day_of_week($opt_m, $opt_d, $opt_y);
    print "$opt_m/$opt_d/$opt_y (MM/DD/YYYY) is $day (where: 0 - Sat ... 6
    - Fri)\n";
     
    mahurshi, Dec 19, 2005
    #1
    1. Advertisements

  2. Since you are "doing this for fun", my comments might not be entirely
    appropriate, but...
    You do know that this problem has been solved before, right?

    To get todays (thats 19 dec 2005) wday, try this oneliner:

    perl -MTime::Local -e'print((localtime(timelocal(0,0,0,19,11,2005)))[6])'

    If you want to experiment with the algorithms, thats fine, if you just
    want to get the job done, I suspect using the builtin functions and
    modules that come with Perl is going to be easier and less trouble to
    maintain.

    (I'm intrigued by your solution though :)

    big
     
    Iain Chalmers, Dec 19, 2005
    #2
    1. Advertisements

  3. mahurshi

    usenet Guest

    You may be interested in reading Larry Wall's ideas on Perl coding
    style:

    perldoc perlstyle

    Cheers!
     
    usenet, Dec 19, 2005
    #3
  4. mahurshi

    Ian Stuart Guest

    Likewise, go hunt out perltidy[1]

    Perltidy allows you to define new comand-line options, which are like
    macro's for the basic option-sets. I have two set up: one for our
    c-code-stylist and one of the rest of us. I try to persuade people to
    re-format the code before-and-after any edits... it means that we each
    see the layout of the code (braces, comments, etc) as we prefer it, and
    can concentrate on the actual code.

    One other thing: comment!
    I find it very helpful to have a comment saying what the following code
    is trying to do, or what the state of affairs is when deep in nested
    if/else/while threads...

    I also find comments helpful what I have to revisit code I wrote 3 years
    ago:
    # we now have two hashes: %params is all the parameters that were
    # passed in, %badparams is any that did not validate for some
    # reason. If we have ANY bad parameters, then we need to reload the
    # form, preloaded with the current parameters (and the wrong fields
    # highlighted).



    [1] Perltidy is a parser/re-formatter that takes code and re-writes the
    file to a defined presentation-style.

    I got the "-gnu" flag included ;-)
     
    Ian Stuart, Dec 19, 2005
    #4

  5. Have you seen

    perldoc perlstyle

    yet?


    Ask for all the help you can get:

    #!/usr/bin/perl
    use warnings; # lexical warnings are better than global (-w) warnings
    use strict;


    If you put your documentation in POD markup, then "perldoc"
    will be able to find and display it:

    perldoc perlpod

    die "Year should be between 1700 and 2099\n"
    unless $year >= 1700 and $year <= 2099;


    Declare it when you first use it:

    my $century = int($year/100);


    Those are each mutually exclusive, so:

    return 4 if $century == 17;
    return 9 if $century == 18;
    return 0 if $century == 19;
    return 6 if $century == 20;

    die "how did control ever get here?\n";


    No it doesn't.

    Have you tested it?


    Your code below will be true if year is NOT evenly divisible by 4...

    if ($year % 4 == 0)


    Error messages should go on STDERR, normal output should go on STDOUT.

    unless (defined($opt_m) and defined($opt_d) and defined($opt_y)) {
    die "Usage: day_for_date.pl -m <month> -d <date> -y <year>";
    }
     
    Tad McClellan, Dec 19, 2005
    #5
  6. mahurshi

    Anno Siegel Guest

    -w is obsolete in favor of "use warnings".

    Also, add "use strict". You have declared most of your variables as
    lexicals (as it should be), but not all. "strict" would have caught
    that.
    Better use a lexical hash instead of the package variables ($opt_d etc.)
    created by Getopt::Std.

    getopt( 'mdy', \ my %opt);

    Later in the program replace $opt_d with $opt{ d}, etc.
    Since century codes aren't common knowledge, an explanation would be in
    order, probably in the form of a literature pointer like (made up):

    # Cf. Daysi Goodyear, _Time and Age_, p. 928 ff
    get_century_code() is basically a lookup. In Perl that is usually best
    realized using a hash (an array would also work in this case, but a hash
    is more generally applicable):

    BEGIN {
    my %century_code = (
    17 => 4,
    18 => 9,
    19 => 0,
    20 => 6,
    );

    sub get_century_code {
    my $key = int( shift()/100);
    defined and return $_ for $century_code{ $key};
    die "Year should be between 1700 and 2099"
    }
    }

    It is not strictly necessary to make the block a BEGIN block, but it
    is safer in case you want to call get_century_code() before it is defined.
    No, it does the opposite. You have the logic reversed.
    The comment and the code are out of sync. If $year % 4 is true (nonzero),
    4 does *not* divide $year. Similar comments apply throughout.
    Correcting and compacting the logic, the function could be written thus:

    sub is_leap_year {
    my $year = shift;
    return !( $year % 4) and (
    ( $year % 100) or ( $year % 400)
    );
    }
    Again, point to an explanation or give it in the comment.
    Like with get_century_code, most of the routine is lookup. Use a hash
    for this, similar to get_century_code(), probably placed in the same
    bare (or BEGIN-) block:

    BEGIN {

    my %month_code = (
    1 => 0,
    2 => 3,
    3 => 4,
    4 => 0,
    5 => 2,
    6 => 5,
    7 => 0,
    8 => 3,
    9 => 6,
    10 => 1,
    11 => 4,
    12 => 6,
    );

    sub get_month_code_1 {
    my ( $month , $year) = @_;
    defined( my $code = $month_code{ $month}) or
    die "Invalid month: $month";
    $code += is_leap_year( $year) if $month <= 2;
    return $code;
    }
    }

    The code assumes that the logic in is_leap_year() is reversed as suggested
    It reproduces the results of your code, though the logic is slightly changed.

    [rest of code skipped and snipped]

    Anno
     
    Anno Siegel, Dec 19, 2005
    #6
  7. mahurshi

    mahurshi Guest

    thanks for all your replies folks. its good to know that we can write
    the same thing in many different ways (if/else if--> return, unless,
    etc). i also like the idea of using the hash for table lookup and for
    getopt. i also fixed the == 0 logic. (i forgot to type that in)

    oh, and i named those variables that way because i didn't even know
    what they'd do. i saw this on tv like... 7/8 yrs ago and remembered
    it. thought it'd be cool to write a program. :)

    thanks again.
     
    mahurshi, Dec 20, 2005
    #7
  8. mahurshi

    John Bokma Guest

    My problem with the above line is that you have your info coded in two
    places: one the hash, two, the error message. If you extend the hash, with
    for example: 16 => 3, then the die message isn't in sync.
    No: don't write your own leap_year code. To often I have seen the % 400
    forgotten (or other mistakes). Check CPAN for a module that has a
    leap_year sub.
     
    John Bokma, Dec 20, 2005
    #8
  9. mahurshi

    Anno Siegel Guest

    Sure, but using a lookup table offers a chance to do better:

    my $msg = do {
    my ( $first, $last) = ( sort { $a <=> $b } keys %century_code)[ 0, -1];
    $_ *= 100 for $first, $last;
    $last += 99;
    "Year should be between $first and $last";
    };

    and later

    die $msg;

    The equivalent can't be done when selection of the century code
    happens in program logic.
    Yes, for production code. This was clearly a programming exercise, so
    other rules apply.

    Anno
     
    Anno Siegel, Dec 21, 2005
    #9
  10. mahurshi

    John Bokma Guest

    [ .. ]
    Yup, that was my point a bit: don't hard code messages if they can become
    out of sync when, in this case, a lookup table is extended.
    Oh, but we agree on that ;-)

    [ leap_year sub ]
    But I doubt this means "we" can get away with open without handling the
    error, or not using strict ;-).
     
    John Bokma, Dec 21, 2005
    #10
    1. Advertisements

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.