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

Spenser has asked for the wisdom of the Perl Monks concerning the following question:

A few years ago I posted a message asking for the Perl Monks to critique my style (A Matter of Style in CGI). I received several responses that helped me to improve my Perl coding. Well, now I'd like to do that again. In those days I did only very simple CGI scripts, drawing data from MySQL. I still do plenty of CGI scripts displaying data from MySQL, but much more involved than before, as well as other things. I'd like to get some critiques again on my coding style related to CGI and MySQL and in a few other areas. I would love to post many pages of code to get feedback. However, to start, I'll stick to something similar to the last time: a script using CGI and DBI.

Below is a CGI Perl script on my personal web site which has a couple of uses—go to it if you want to see the results of this script. If the user provides no parameters, it will list the last ten of my musings (i.e., web log entries) stored in a table in MySQL. If the user provides a category (e.g., italian), it will give all musings from that category (e.g., all musings on learning Italian). If the user presents an identification number for a specific musing, the user is redirected to a different script.

#!/usr/bin/perl -w use strict; # Program Name: musings.cgi # Description of Program: # This is a Perl script which creates the page # for viewing a list of my web log entries. # set libraries, scripts, & paths require './library/main.pl'; require './library/musings.pl'; my $scripts = &get_script_variables(); # main.pl # capture user input my $musing_id = param('musing_id') || ''; my $ref_id = param('ref_id') || ''; # musing category # reroute user to musing if given an id if($musing_id =~ m|\d|g) { print redirect($scripts->{'musing'}->{'script'} . '?musing_id=' . $musing_id); exit; } if(!$ref_id) { # if no category requested, $ref_id = 'all'; # select all categories } # get web log entries and related info from MySQL my $entries = &get_musings_entries($musing_id,$ref_id); # musings.pl my $comment_cnt = &get_comments_count(); # musings.pl my $musing_stats = &get_reader_stats(); # musings.pls my $musing_cats = &get_musings_categories(); # musings.pls # get general text for web page from MySQL my $genl_text = &get_general_text('musings',1); # center.pl my ($pg_head,$pg_text,$pg_image) = @{$genl_text->[0]}; my $n = "\015\012"; # start web page &heading('musing',undef,$pg_head,undef,$pg_text); # header.pl # left margin &panel_start(1); # main.pl &fiction(); # left_margin.pl &miscellany(); # left_margin.pl &musings_sites(); # left_margin.pl &panel_end(); # main.pl # center panel of web page &panel_start(2); # main.pl # display page heading print start_div({id=>'page_heading'}), $n; if($musing_id =~ /[a-z]/ && $musing_id ne 'all') { my $cat_head = ucfirst($musing_cats->{$musing_id}->{'ref_name'}); $pg_head = $cat_head . $pg_head; my $sub_genl_text = &get_general_text('musing_cat',1); # center.pl my ($sub_pg_head,$sub_pg_text) = @{$sub_genl_text->[0]}; $pg_text = $sub_pg_text; &page_heading($pg_head,$pg_text,undef); # center.pl } else { &page_heading($pg_head,$pg_text,undef); # center.pl } print end_div(), $n,$n; # end div#page_heading # display web log entries foreach (@$entries) { my ($entry_id, $ref_id, $log_head, $entry, $entry_date) = @$_; my $category = lc($musing_cats->{$ref_id}->{'ref_name'}); my $count = $comment_cnt->{$entry_id}->{'comment_cnt'} || 'no'; my $reader_stat = $musing_stats->{$entry_id}->{'stat'}; $entry_date = lc($entry_date); print div({class=>'content_item'}, $n, div({class=>'musing_abstract'}, $n, h3( a({href=>$scripts->{'musing'}->{'script'} . '?musing_id=' . $entry_id}, $log_head) ), $n, span($entry), $n ), $n, div({class=>'item_stats'}, $n, qq|posted: $entry_date ;  readers: $reader_stat |, $n, a({id=>'comment_notation', href=>$scripts->{'musing'}->{'script'} . '?musing_id=' . $entry_id}, $count, 'comments; '), $n, 'category: ', a({id=>'comment_notation', href=>$scripts->{'musings'}->{'script'} . '?ref_id=' . $ref_id}, $category) ), $n ), $n,$n; } &panel_end(); # main.pl # right margin &panel_start(3); # main.pl &display_page_image($pg_image); # right_margin.pl &get_display_category_data(); # musings.pl &stats_musings(); # right_margin.pl &panel_end(); # main.pl &footer(); # main.pl exit;

I added some hard-returns above to items such as a({href=>''} so that the lines wouldn't wrap here, to make it easier to read. You'll notice also that there are references to functions contained in several libraries. I don't want to wear y'all out by posting all of my libraries. However, I've included one function, &get_musings_entries() from my musings.pl library. In another post I might include a sampling of some key functions I've created, but for now I'll settle for this one. This function queries the MySQL database, with the SQL statement set based on whether the user has provided a musing category, has requested all categories, or has provided an identification number for a specific musing. Incidentally, the connection to MySQL is established by a different function elsewhere.

sub get_musings_entries { # get musings from MySQL my ($musing_id,$ref_id) = @_; if($ref_id && $ref_id eq 'all') { $sql_stmnt = qq|SELECT musing_id, ref_id, heading,abstract, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings ORDER BY entry_date DESC|; } elsif($ref_id) { $sql_stmnt = qq|SELECT musing_id, ref_id, heading, abstract, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings WHERE ref_id = '$ref_id' ORDER BY entry_date DESC LIMIT 10|; } else { $sql_stmnt = qq|SELECT musing_id, ref_id, heading, entry, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings WHERE musing_id = '$musing_id'|; } $sth = $dbh->prepare($sql_stmnt); $sth->execute(); my $entries = $dbh->selectall_arrayref($sql_stmnt); $sth->finish(); return $entries; }

As I said in my post several years ago, I'd like all of the criticisms that I can get. However, please don't feel obligated to review and comment on everything. You could just pick out one line that stands out. Whatever you have time to do is appreciated. I would like comments on functionality, documentation, layout, spacing, etc. No criticism will be considered too minor. Thanks again in advance.

-Spenser

That's Spenser, with an "s" like the detective.

Replies are listed 'Best First'.
Re: How's My Style Now?
by jethro (Monsignor) on Feb 06, 2010 at 02:22 UTC

    Maybe you have a reason for it, but nowadays require'ing .pl files looks somewhat antiquated. Libraries should end up as modules to be use()d. You get to choose which subroutines pollute your namespace (via the Exporter module). You have a clean interface (where someone could look at the module and know which subroutines are internal and which are for other modules or scripts to use)

    If you really need the comments where a function is from, modules also give you a fitting (explicit) syntax. Instead of

    get_general_text('musing_cat',1); # center.pl

    you could write

    center::get_general_text('musing_cat',1);

    This way the interpreter will correct you if get_general_text were in header.pm instead

      I second the module suggestion and sdd a couple of things to jethro's comments.

      When you do create modules, do not use lower-case module names. Lower-case names are (by convention) reserved for pragmatic modules (in effect compiler directives). Additionally, placing your modules into a provate namespace can protect against name collision. For instance, you could use your initials so that require './library/main.pl'; becomes use RJTD::Main; (See also: perlmodstyle, perlnewmod, perlmod)

      I also find the # center.pl comments a bit jarring, though my recommended alternative would be to document their origin at the beginning of the file like so1:

      use RJTD::Main qw/ get_script_variables panel_start panel_end footer / +; use RJTD::Center qw/ get_general_text page_heading /; #... later get_general_text('musing_cat',1);

      This way, you do not need to document each use of the function since you know that the list of imported functions is always at the top of the file.

      1 When creating your modules use @EXPORT_OK rather than @EXPORT; See Exporter.

      Update: Split EXPORT vs EXPORT_OK into footnote so that things makes sense.

      Good Day,
          Dean

      This is a very cool idea. Using modules would lift me further out of the amateur ranks. It will make maintaining code easier. Plus, it sounds like a fun idea.

      -Spenser

      That's Spenser, with an "s" like the detective.

        Well, it took three long and intensive days once I started, but I converted all of libraries to modules and changed all of my programs which used the libraries to use the new modules. The code is much better now. Next I will have to do the same for my other web sites.

        As an update on where I am now as a coder, I think a couple of years ago I would not have been able to have done this in just three days--it would have taken me at least three weeks, maybe months. I would have tried, gotten confused, stopped, and started again several times before working it out. When I made the previous post on this topic (A Matter of Style in CGI) several years ago, I would not have been able to have changed my libraries and all over to modules. It would have been beyond my comprehension. So, thanks especially for this suggestion. It has helped me to test my skills doing something in Perl that I had never tried to do before, or even thought of doing.

        -Spenser

        That's Spenser, with an "s" like the detective.

Re: How's My Style Now?
by toolic (Bishop) on Feb 06, 2010 at 01:24 UTC
Re: How's My Style Now?
by cjcollier (Novice) on Feb 06, 2010 at 00:58 UTC

    First of all, write your docs using pod.

    $ perldoc perlpod

    I've got to go home so I can start on my homework. woe.

Re: How's My Style Now?
by elTriberium (Friar) on Feb 06, 2010 at 02:23 UTC
    This:
    if($musing_id =~ /[a-z]/ && $musing_id ne 'all') { [...] &page_heading($pg_head,$pg_text,undef); # center.pl } else { &page_heading($pg_head,$pg_text,undef); # center.pl }
    Can be simplified to:
    if($musing_id =~ /[a-z]/ && $musing_id ne 'all') { [...] } &page_heading($pg_head,$pg_text,undef); # center.pl
Re: How's My Style Now?
by ww (Archbishop) on Feb 06, 2010 at 00:59 UTC
    Much improved.

    Downside: way too many comments on obvious statements, such as above the require...s

    And a style note re posting here:

    "added some hard-returns above ... so that the lines wouldn't wrap here, to make it easier to read."

    Good intent but not a good idea: May or may not, depending on the viewer's mindset, make it easier to read, but it makes any testing the visitor might wish to do go badly, at a minimum because the line numbers may be different than your originals... and it may even obfuscate your understanding or intentions.

Re: How's My Style Now?
by apl (Monsignor) on Feb 06, 2010 at 12:38 UTC
    I have a very minor comment. I'd change
    my $category = lc($musing_cats->{$ref_id}->{'ref_name'}); my $count = $comment_cnt->{$entry_id}->{'comment_cnt'} || 'no'; my $reader_stat = $musing_stats->{$entry_id}->{'stat'}; $entry_date = lc($entry_date);
    to
    my $category = lc($musing_cats->{$ref_id}->{'ref_name'}); my $count = $comment_cnt->{$entry_id}->{'comment_cnt'} || 'no +'; my $reader_stat = $musing_stats->{$entry_id}->{'stat'}; $entry_date = lc($entry_date);
    As a purely subjective observation, I think the second is more readable.
Re: How's My Style Now?
by molecules (Monk) on Feb 06, 2010 at 13:19 UTC

    I like code reviews. Good idea.

    (1) Since you have three SQL statements starting with identical text, I would create a subroutine for that part.

    sub sql_select_from_musings{ return qq| SELECT musing_id, ref_id, heading, entry, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings |; }
    Then for one of your SQL statements, you could type
    $sql_statement = sql_select_from_musings() . qq|ORDER BY entry_date DESC|;

    (2) I like using PerlTidy. I use the Perl Best Practices .perltidyrc with it.

    This will help you apply ap1's suggestion above automatically. For example:

    my $apples = 5; my $watermelons = 7;
    becomes
    my $apples = 5; my $watermelons = 7;

    without my having to type extra spaces. If I changed $watermelons to $pears, then Perl Tidy can respace it without me having to add or delete spaces manually.

    Several IDEs/editors have ways you can set it up to run Perl Tidy with a single command.


    (3) I also like Data::Alias. For example, you could have this line:

    alias my $musing_script = $scripts->{'musing'}->{'script'};

    After which, you can just type

    $musing_script

    instead of having to use two levels of dereferencing.

    Just my ideas; use whatever suits you.

      Thanks for the suggestion. I see your point about making a sub-routine. I go back and forth on the idea of sub-routines: sometimes I can't get enough of them (e.g., that &panel_end() is just a print end_div() line). Other times, especially when I return to a program many months later and confuse myself with overuse of sub-routines, I decide to be more conservative. For instance, for CGI scripts I try to use only sub-routines (calling them from a library) to get data or to process it. I leave the printing of data in the program, not the library. I guess I could do a compromise and use sub-routines for printing, but leave them in the program, at the bottom. I just find that a program with only a list bunch of home-made functions can become confusing later when I need to modify the program.

      -Spenser

      That's Spenser, with an "s" like the detective.

        I understand the tension between too-much modularity and not enough. It's a continuing part of self-aware coding.

        Moving more of your code into modules with POD documentation will help make subroutines less burdensome.

        Using modules lets you group your code into specific name spaces, and when you use explicit imports, you have an instant reminder where the sub came from.

        Using POD lets you use perldoc to read about your code.

        Consider this code:

        use MyMod qw( foo ); foo( $somearg, $another, 57, 'bunnies' );

        I know that foo() comes from MyMod, so I can simply do a perldoc MyMod, to read about foo().

        This beats the heck out of:

        require './mylib.pl'; foo( $somearg, $another, 57, 'bunnies' ); # mylib.pl
        Escpecially since if I refactor MyMod or mylib, and move foo() to another file, failure to update the location info in my code causes a fatal error for the module. With library requires, I can easily get stuck with comments that lie.


        TGI says moo

Re: How's My Style Now?
by pobocks (Chaplain) on Feb 06, 2010 at 09:30 UTC

    This is, perhaps, not exactly what you had in mind, but on your website, the comments on musings seem to be broken. I attempted to post a response to the one on burglar vs. home invader (a false dichotomy, by the way - the concepts are not synonymous, and burglar is alive and well in English), and was told that the page was missing.

    for(split(" ","tsuJ rehtonA lreP rekcaH")){print reverse . " "}print "\b.\n";
Re: How's My Style Now?
by YuckFoo (Abbot) on Feb 06, 2010 at 23:02 UTC
    Overall it's consistent and readable, but for minor nit picks I choose this line:
    print redirect($scripts->{'musing'}->{'script'} . '?musing_id=' . $musing_id);
    Drop unnecessary quoting of hash keys, prefer interpolation to dot concatenation. This, less noisy:
    print redirect("$scripts->{musing}{script}?musing_id=$musing_id");

      I agree with your suggestion here. However, let me tell you a little about the genealogy of this bit of code, because there's another stage to come and I'd like a suggestion about how to complete this. For links I used to have something like this:

      a({href=>"/cgi-bin/musing.cgi?musing_id=$musing_id"}, $musing_heading)

      This worked fine for a long time. However, I would change occasionally the name of a script or move it to a different directory. For example, musing.cgi used to be called web_log.cgi. Then I'd have to go through my scripts and other places where script names were contained and change it. Last year I came up with the idea of entering the names of all of my scripts in a table in MySQL I named site_scripts. Now if I want to change the name of a script or location of a script, I change it in my site_scripts table in MySQL. I just use the same key name (e.g., musing) to retrieve it. This has worked well for me so far.

      Now what I want to do is to add the particular ID for the script. For instance, for a musing I generally have musing_id as the CGI variable name. For it, I'd like to change '?musing_id=' to something less hard-coded to something like this:

      a({href=>scripts->{'musing'}->{'script'} . '?' . script_ids->{'musing'}->{'id'} . '=' $musing_id}, $musing_heading)

      To tighten this up more, and to reduce the interpolation you mention, I could add the ? and the = to the hash containing the id label. Of course, once I have this problem worked out, I can have a sub-routine for my hyperlinks: hyperlink('musing',$musing_id,$musing_heading)

      That would be so much nicer, wouldn't it? It would be simple to set up another column in my site_scripts table in MySQL to hold the ID label. However, for some scripts I need more than one CGI variable: /cgi-bin/musing.cgi?musing_id=$musing_id&ref_id=$ref_id. That might also through off my hyperlink() idea. So, any suggestions on how I might organize multiple ID possibilities in MySQL or how to sort through them in Perl when deployed with a function?

      -Spenser

      That's Spenser, with an "s" like the detective.

        You could put all the link particulars in a module so that changes can be made in one place. Here is a Links.pm to give you ideas. It would be better to go a step further and have the module read a configuration file, but I didn't want to spoil all your fun.

        If you only have a handful of links to maintain, a database may be overkill. By putting this in a module, you should be able to make the change from a config file to database with little, if any, change in the calling scripts.

        #!/usr/bin/perl package Links; use strict; use CGI qw(:standard); my $Links = { musings => { text => 'Musings', link => '/cgi-bin/musing.cgi', id_list => [ 'musing_id', ], }, rants => { text => 'Rants', link => '/cgi-bin/rants.cgi', id_list => [ 'rant_id', 'ref_id', ], }, stuff => { text => 'Other Stuff and Junk', link => '/cgi-bin/misc.cgi', id_list => [ ], }, }; #----------------------------------------------------------- sub make { my $key = shift; my $vals = shift; (defined($Links->{$key})) or return; my $info = $Links->{$key}; my @id_list; for my $id (@{$info->{id_list}}) { if (defined($vals->{$id})) { push @id_list, "$id=$vals->{$id}"; } else { # what if? } } my $url = $info->{link}; if (@id_list) { my $id_str = join('&', @id_list); $url = "$url?$id_str"; } return a({-href=>$url}, $info->{text}); } 1;
        #!/usr/bin/perl use strict; use Links; my $vals = { musing_id => 42, rant_id => 1, ref_id => 2, foo_id => 3, }; my $link = Links::make('musings', $vals) or die; print "$link\n"; my $link = Links::make('rants', $vals) or die; print "$link\n"; my $link = Links::make('stuff', $vals) or die; print "$link\n";
Re: How's My Style Now?
by juster (Friar) on Feb 06, 2010 at 18:54 UTC

    You don't need to prefix subroutine calls with &. Thats a throwback from perl 4 like 10 years ago? I dunno for sure, sometimes I see it. Its ugly and extra work anyways!.

    Nested dereferencing "arrows" (->) can be ommitted. I'm not sure how best to describe it, but here is the before:

    my $category = lc($musing_cats->{$ref_id}->{'ref_name'});

    After:

    my $category = lc $musing_cats->{$ref_id}{'ref_name'};

      Thats a throwback from perl 4 like 10 years ago
      And then some! From perlhist:

      Larry 5.000 1994-Oct-17
      Yes, cjcollier has told me about this habit I have of prefixing sub-routine calls with an ampersand. It may not be deprecated, but it is somewhat self-deprecating on my part to use them. Like the Anonymous poster here, I find it easier visually to locate them. However, it is suggesting that my functions are of a lesser grade and it's unnecessary formatting. Alright, I'll stop using that.

      -Spenser

      That's Spenser, with an "s" like the detective.

      I like &'s for readability. They aren't deprecated, and easy to jump to when using vi.

        ... and they have unwanted side effects, like overriding prototypes. Bad idea.

        Alexander

        --
        Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
Re: How's My Style Now?
by chb (Deacon) on Feb 08, 2010 at 08:52 UTC
    Please please use placeholders in all SQL, everytime, everywhere, always. Imagine what happens when someone submits the value 0'; DROP TABLES; -- as $ref_id. If you use placeholders in your prepared statements, the DBI driver would quoute away some of the danger in this.

      I usually do use placeholders. However, in this case one of the three possible SQL statements has no value required. How would deal with that? For the other two I can create a variable (e.g., $particular) and set it in block of the elsif and the else, to be used like so, $sth->execute($particular);. However, for the if statement, won't I get an error even if $particular is set to blank?

      -Spenser

      That's Spenser, with an "s" like the detective.

        Just change your control flow slightly and prepare/execute the entire statement inside the if/elsif/else blocks (warning: untested code)
        sub get_musings_entries { # get musings from MySQL my ($musing_id,$ref_id) = @_; if($ref_id && $ref_id eq 'all') { $sth = $dbh->prepare(qq|SELECT musing_id, ref_id, heading,abstract, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings ORDER BY entry_date DESC|); $sth->execute(); } elsif($ref_id) { $sth = $dbh->prepare(qq|SELECT musing_id, ref_id, heading, abstract, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings WHERE ref_id = '?' ORDER BY entry_date DESC LIMIT 10|); $sth->execute($ref_id); } else { $sth = $dbh->prepare(qq|SELECT musing_id, ref_id, heading, entry, DATE_FORMAT(entry_date, '%M %e, %Y') FROM musings WHERE musing_id = '?'|); $sth->execute($musing_id); } my $entries = $dbh->selectall_arrayref($sql_stmnt); $sth->finish(); return $entries; }
Re: How's My Style Now?
by scorpio17 (Canon) on Feb 08, 2010 at 15:19 UTC

    You might consider using CGI::Application and HTML::Template. Note that templates can contain other templates, so you can have a single, top-level template that pulls in templates for things like header, footer, main panel, left panel, etc. Then the CGI script simply builds up a hash of all the data, and sends it to the template. The benefit is that you remove virtually all of the HTML related stuff from your script, and you can change the entire layout of your site by simply editing the template files (which are like html files with place holders for the data).

      As a follow-up to my original post, below is how the code for that same program looks now. I'm posting it here in case someone else wants to convert to modules and want to see how that would look. Thanks again for all of the advice everyone gave me. I'm still incorporating your ideas.

      #!/usr/bin/perl # program name: musings.cgi # set libraries, scripts, & paths use strict; use warnings; use library::core; use library::musings; use CGI qw(:standard); my $scripts = core::get_script_variables(); my $script = $scripts->{'musing'}->{'script'}; my $top_script = $scripts->{'musings'}->{'script'}; # capture user input my $musing_id = param('musing_id') || ''; my $ref_id = param('ref_id') || ''; # check if user data is complete if($musing_id =~ m|\d|g) { print redirect($script . '?musing_id=' . $musing_id); exit; } # get general text for web page my $general_text = core::get_general_text('musings',1); my ($page_heading,$page_text,$page_image) = @{$general_text->[0]}; my $n = "\015\012"; # get web log entry my $entries = musings::get_musings_entries($ref_id); my $comments_cnt = musings::get_comments_cnt(); my $musing_stats = musings::get_reader_stats(); my $musing_cats = musings::get_musings_cats(); # alter heading if musing category selected if($ref_id) { $general_text = core::get_general_text('musings_category',1); ($page_heading,$page_text,$page_image) = @{$general_text->[0]}; $page_heading = ucfirst($musing_cats->{$ref_id}->{'ref_name'}) . $p +age_heading; } # web page core::heading($page_heading,undef,$page_text); core::panel_start(1); core::fiction(); core::miscellany(); core::musings_sites(); core::panel_end(); core::panel_start(2); core::page_heading($page_heading,$page_text,undef); # display musing entries foreach (@$entries) { my ($id, $ref_id, $log_head, $entry, $entry_date) = @$_; my $cat = lc($musing_cats->{$ref_id}->{'ref_name'}); my $cnt = $comments_cnt->{$id}->{'comments_cnt'} || 'no'; my $reader_stat = $musing_stats->{$id}->{'stat'}; $entry_date = lc($entry_date); print div({class=>'content_item'}, $n, div({class=>'musing_abstract'}, $n, h3(a({href=>$script . '?musing_id=' . $id}, $log_head) ), $n, span($entry), $n ), $n, div({class=>'item_stats'}, $n, 'posted:', $entry_date, '; readers: ', $reader_stat, '; ', $n, a({id=>'cmt_note', href=>$script . '?musing_id=' . $id}, $cnt, 'comments'), $n, 'category: ', a({id=>'cmt_note', href=>$top_script . '?ref_id=' . $ref_id}, $cat) ), $n ), $n; } core::panel_end(); core::panel_start(3); core::display_page_image($page_image); musings::get_display_category_data(); core::stats_musings(); core::panel_end(); core::footer(); exit;

      -Spenser

      That's Spenser, with an "s" like the detective.