Beefy Boxes and Bandwidth Generously Provided by pair Networks
Pathologically Eclectic Rubbish Lister
 
PerlMonks  

Re: flower box comments

by adrianh (Chancellor)
on Nov 17, 2003 at 15:01 UTC ( [id://307673]=note: print w/replies, xml ) Need Help??


in reply to flower box comments

I have my doubts about this style too :-)

#--------------------------------------------------------- # Function:   get_records (taken from process_member.pl) # Arguments:  DB handle #             sql query string # Returns:    result array of SQL query #--------------------------------------------------------- sub get_records {    my ( $db_handle_ref, $sql ) = @_;    my @recs; # [...]    return @recs; }

Several issues:

First off, as several other people have pointed out, if this is documentation aimed at the users of the code then POD would be the preferred format. That way you get the support of a lot of POD:: modules for formatting and reading your documentation.

Second, look at what the comments tell you - or rather what they don't:

# Function:   get_records (taken from process_member.pl)

The function name and the filename it lives in. Both of which we already know (we're looking at the file to see the comment, and the subroutine name is part of the subroutines declaration.)

# Arguments:  DB handle #             sql query string

The arguments are a database handle (to what?) and an SQL query string (of what format?). This gives us no more information that we would get from the first line of the function:

   my ( $db_handle_ref, $sql ) = @_;

It would be even easier if we called the database handle $dbh which would be familiar to all users of DBI, and a more descriptive name for the sql query.

Finally this:

# Returns:    result array of SQL query

Tells us, I think, that we get an array of the rows returned by the SQL query. This is:

  • Phrased badly. I could read that as an array of SQL query objects rather than the results from the SQL query.
  • It doesn't tell me what array consists of. Is it an array of hashes keyed of column names? An array of values in order? What?

All of this information is better expressed in the code itself. Rather than spending time writing comments that can easily get out of sync with the code, spend time writing clear expressive code. If you can't look at a subroutine and understand what it does rewrite it until you do. Break it into smaller subroutines, use better subroutine names, use better variable names, etc.

For example the original routine might be better expressed as (guessing it's actual use since the comment doesn't actually say :-)

sub fetch_matching_process_records {    my ( $dbh, $sql_query ) = @_;    my @process_records; # Something that populates @process_records with # ProcessRecord objects with a known API    return @process_records; }

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://307673]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others wandering the Monastery: (4)
As of 2024-04-19 04:06 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found