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


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:

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