Some day some bright light will come forward and lay down the law about how Perl code should be formatted. And, just as likely, they will be roundly ignored, primarily out of sheer spite, but with a grain of “perl lets us do what we want. We don’t need no steenkin standards” tossed in just to muddy the argument.
Today’s annoyance is cleaning up some long forgotten programmer’s way of of formatting blocks of code. Only slightly edited, here’s a sub in one of the cgi scripts:
sub _getMediaList( $ $ $ ;$ )
{
my ($carrierId, $contentType, $contentIds, $formatted) = @_;
return undef if (!defined($contentType)); # Charts can have no contents and thus no content type
my $gpd = GPD::getDbh();
my @mediaList;
foreach my $contentId (@$contentIds)
{
my $resultList = mediaSearch(dbh => $gpd,
carrierId => $carrierId,
contentType => $contentType,
id => $contentId,
showdown => 1);
next if (!$resultList->[0]); # Shouldn't happen
push(@mediaList, $resultList->[0]);
}
if ($formatted)
{
foreach my $media (@mediaList)
{
$media->{title} = "$media->{artist} - $media->{name} - $media->{album}";
}
return formatMediaListColored('[%s] [%s] [%s] %s [%s] [%s] [%s] [%s]', [ 'id', 'isrc', 'upc', 'title', 'genre', 'label', 'relate
d_content_isrc', 'release_date'], @mediaList, 1);
}
return @mediaList;
}
Now, I’m not harping on the code itself – this is pretty innocuous, if slightly braindead. But what annoys me is the overuse of newlines to stand off functional blocks. They’re not necessary, the indenting is painful, and it makes the code hard to read. How hard is it to rework it like this:
sub _getMediaList( $ $ $ ;$ ) {
my ($carrierId, $contentType, $contentIds, $formatted) = @_;
my $gpd = GPD::getDbh();
my @mediaList;
return undef if (!defined($contentType)); # Charts can have no contents and thus no content type
foreach my $contentId (@$contentIds) {
my $resultList = mediaSearch(dbh => $gpd,
carrierId => $carrierId,
contentType => $contentType,
id => $contentId,
showdown => 1);
next if (!$resultList->[0]); # Shouldn't happen
push(@mediaList, $resultList->[0]);
}
if ($formatted) {
foreach my $media (@mediaList) {
$media->{title} = "$media->{artist} - $media->{name} - $media->{album}";
}
return formatMediaListColored('[%s] [%s] [%s] %s [%s] [%s] [%s] [%s]',
[ 'id', 'isrc', 'upc', 'title', 'genre', 'label', 'related_content_isrc', 'release_date'],
@mediaList, 1);
}
return @mediaList;
}
To me this is far easier to work with and understand where teh blocks are and how they work together. In my opinion, there’s a couple basic rules about code layout:
- Consistent indenting – 4 spaces or 2, I don’t really care which, just use the same thing EVERYWHERE.
- Curly braces to open a block go on the same line as the introductory keyword (if, foreach, sub, whatever).
- Lines longer than 80 characters should be broken into sane sub-lines. Just hit enter and indent, folks. Not that hard.
- Declarations at the top of the block when the variables are used across the entire block.
- Separate declarations from functional code by a newline.
Now, this particular piece of code has no comments in it, so I’ll leave comments for another rant. But I think these basics sure would make perl code a lot more maintainable if everyone at least marginally followed them.
