http://qs321.pair.com?node_id=178775


in reply to Help with my Coding Style - another chance to critique

It's far from the worse code I've seen ;-) Seriously it's not bad. There are a few minor nits here and there.
use Date::Calc qw(Date_to_Text_Long);
qw is unnecessary for a single element, and inconsistent with line 3
use DBI();
do you really mean a null import list? it would also perhaps be better to insert whitespace between DBI and the parens. (I did say these were nits ;-)
$self->tmpl_path('/path/to/my/include/files/');
Use of ' for things that needn't be interpolated is a good habit.
'index' => 'do_index',
keys of \w+ don't need to be quoted with =>
$self->param('dbh' => DBI->connect("DBI:mysql:database=authorweb;host=localhost",
You might look at DBIx::Connect.
if ($cgi->param('alpha') =~ /^(A-Za-z){1}$/)
the {1} is meaningless here
" "
This might be a good place for a dispatch table or mock switch statement, especially if you might add functionality in the future.
'Corwin\'s AuthorWeb'
This is unnecessarily repeated, you might consider setting it up in a package global or constant?
my is somewhat expensive
You might consider grouping all of them at a head of the block eg; my($foo, $bar) The difference is rather small (4% for 4 scalars in my tests) but could make a difference on high traffic site. As a matter of taste it might be prefertial to see all of the variables of a block in one location.
my $ref = $sth->fetchrow_hashref();
I would instead immediately pass the fetch result directly to your sub, this would also allow you to collapse the entire if statement into one lie. either foo if bar or bar && foo
$_
There are several places you could take advantage of $_, but this is personal preference, I happen to like dense code.
if ($ref->{'dob'})
thse kind of things often look and read (as English) better as a single line of code like the previous if.

--
perl -pew "s/\b;([mnst])/'$1/g"

Replies are listed 'Best First'.
Re: Re: Help with my Coding Style - another chance to critique
by crazyinsomniac (Prior) on Jul 02, 2002 at 07:27 UTC
      Rat is, my point is that several params are being given the same substr. Having this substr be stored in one place would make it easier to update, etc.

      --
      perl -pew "s/\b;([mnst])/'$1/g"

        Good point, belg4mit. I've actually gone one beyond this and, combined with one of crazyinsomniac's suggestions from below, I've made the program quite a bit more customizable. The CGI script now has the following line, allowing me to set both the path top the templates and the main application title:

        my $webapp = AuthorWeb->new( TMPL_PATH => '/path/to/includes/', PARAMS => { apptitle => 'Corwin\'s AuthorWe +b'} );