Any suggestions for my programming style?

M

mahurshi

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

Iain Chalmers

Hi folks.. I write code as a hobby to keep me from getting bored.
Click to expand...

Since you are "doing this for fun", my comments might not be entirely
appropriate, but...
I am pasting my code here. The program is supposed to find the day of
the week for a given date.
Click to expand...

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
 
U

usenet

I have been trying to think of ways to improve my coding style.
(organizing, commenting, indenting, error checking, uncluttering, etc)
Click to expand...

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

perldoc perlstyle

Cheers!
 
I

Ian Stuart

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

perldoc perlstyle
Click to expand...
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 ;-)
 
T

Tad McClellan

I have been trying to think of ways to improve my coding style.
Click to expand...


Have you seen

perldoc perlstyle

yet?

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.

#!/usr/bin/perl -w
Click to expand...


Ask for all the help you can get:

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

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


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

perldoc perlpod

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

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

my($century);
$century = int($year/100);
Click to expand...


Declare it when you first use it:

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

if ($century == 17) { return 4; }
elsif ($century == 18) { return 9; }
elsif ($century == 19) { return 0; }
elsif ($century == 20) { return 6; }
Click to expand...


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

## this function returns 1 if a given year is leap year
## it returns 0 otherwise
Click to expand...


No it doesn't.

Have you tested it?

sub is_leap_year
{
my($year) = @_;

## if year is divisible by 4
Click to expand...


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

if ($year%4)
Click to expand...

if ($year % 4 == 0)

## 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;
}
Click to expand...


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

Anno Siegel

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
Click to expand...

-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.
#################################################
## 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');
Click to expand...

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.
## this function returns a century code given a year
Click to expand...

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

}
Click to expand...

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.
## this function returns 1 if a given year is leap year
## it returns 0 otherwise
Click to expand...

No, it does the opposite. You have the logic reversed.
sub is_leap_year
{
my($year) = @_;

## if year is divisible by 4
if ($year%4)
Click to expand...

The comment and the code are out of sync. If $year % 4 is true (nonzero),
4 does *not* divide $year. Similar comments apply throughout.
{
## 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; }
}
Click to expand...

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)
);
}
## this function returns the month code given a month and a year
Click to expand...

Again, point to an explanation or give it in the comment.
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; }
}
Click to expand...

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
 
M

mahurshi

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.
 
J

John Bokma

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"
Click to expand...

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.
Correcting and compacting the logic, the function could be written
thus:
Click to expand...

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.
 
A

Anno Siegel

John Bokma said:
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.
Click to expand...

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.
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.
Click to expand...

Yes, for production code. This was clearly a programming exercise, so
other rules apply.

Anno
 
J

John Bokma

[ .. ]
Sure, but using a lookup table offers a chance to do better:
Click to expand...

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.
The equivalent can't be done when selection of the century code
happens in program logic.
Click to expand...

Oh, but we agree on that ;-)

[ leap_year sub ]
Yes, for production code. This was clearly a programming exercise, so
other rules apply.
Click to expand...

But I doubt this means "we" can get away with open without handling the
error, or not using strict ;-).
 

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. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Help with code plsss 0
Taskcproblem calendar 4
What date was so many months and years before 18
Getting a bad input error, not sure why. 2
Please critique my code for fun learning project. 5
Collect Excel Data from Website 5
Connected SQLite to my java program but information are not submitted 2
New coder looking for critique on fun project. 6

Members online

No members online now.
Total: 28 (members: 0, guests: 28)
Robots: 490

Forum statistics

Threads
473,769
Messages
2,569,579
Members
45,053
Latest member
BrodieSola

Latest Threads

Top