RFC: "Best Practices" code review section

[rvosa]

Almost every day I learn about more little devils that might slip into my code details. For example, I'm slowly getting used to not doing:

open(FILE, $filename);

but instead doing:

my $fh;
open($fh, '<', $filename) or die "can't open $filename: $!"

Either approach is semantically correct, and there's more than one way to do things, but the second one is better practice because:

• The file handle $fh is lexically scoped, preventing weird action-at-a-distance.
• The mode is explicitly set, preventing unintended operations, i.e. overwriting $filename.
• open is tested, and errors returned by the file system - i.e. $! - are returned.

I am sure there's an endless number of other things that I'm still doing "wrong" because I don't know about them - although my programs execute just fine.

Wouldn't it be a good idea to erect a place on perlmonks where those issues could be discussed in the form of mini code reviews? For example, someone has written a few blocks of code < $maxlength, where $maxlength is something manageable, like 100 lines. The code executes just fine, but the coder is inspired by TheDamian's book on Perl best practices and would like further nitpicking for the edification of all and sundry. Other monks move in, and crush submitter's spirit with maddeningly anal comments.

(I am reminded here of UWashington's ecology department's lunch seminar series Eco-Lunch, where willing grad students present research and field questions. The series is internally known as the Ego-Crunch.)

The idea is that the code already works - i.e. not SoPW material -, is very much NOT obfuscated and might even be very UNCOOL, i.e. accountancy software.

Your comments please!

[tye]

The sections at PerlMonks are mostly not by topic but by intent. So, if you want to share (and discuss) a best practice that you discovered, then you post to Meditations. If you want to seek Perl knowledge about best practices of some (hopefully specific) type, then you post to SoPW. If you want to provide a detailed, gentle, and polished presentation covering best practices, then you might post to Tutorials (after getting some feedback, such as by posting a draft to Meditations).

If you want to find a bunch of discussions about best practices, then things get a bit more complicated with a lot of possible approaches, none of which is perfect. But adding a dedicated section in hopes of addressing that is not a more perfect solution, despite it sounding simple and effective at first blush. Most threads cover more than one topic and so attempting to partition up threads into groups based on subject is fundamentally flawed. Once one realizes this, one next jumps to building overlapping groups of threads by topic. And this is a fine approach but the devil is in the details. And most of the existing approaches do just that, choosing different ways of determining who gets to pick which threads are appropriate for inclusion under which topics.

And a big problem to solve is motivation. You are currently motivated to collect information about best practices. So I suggest you Super Search and google and such for threads on that topic and for existing collections of links to items on that subject and start building your own collection. You can start building this collection on your home node or your scratchpad. As you notice gaps or other issues, you might be inspired to seek wisdom within a specific gap or to try to fill a gap or improve upon existing information sources. And to do that you might consult my first paragraph again. (:

[tinita]

my $fh;
open($fh, '<', $filename) or die "can't open $filename: $!"

it's probably offtopic for this thread, but i have to comment on that =)
i prefer
open(my $fh, '<', $filename) or die "can't open $filename: $!";

[rvosa]

Ha! Now we're talking :)

[trammell]

A better practice is to put $filename (or similar) in some sort of delimiter in the die() call, like this:

open(my $fh, '<', $filename) or die "can't open '$filename': $!"

This gives you a handle on edge cases where e.g. $filename has trailing whitespace.

[blazar]

Yes, it may be a good idea. But fundamentally I think that the Tutorials section may alread be providing the facilities for your hypothetical "place". If you're asking about suggestions regarding best practices, three that spring to mind immediately are:
• use a simple glob or File::Find (or one of its enhanced brothers) instead of an explicit opendir, readdir, etc.
• use Perl-style loops instead of C-style ones unless you have particular (good) reasons to do so,
• either use $_ for what it is, i.e. the topicalizer or use fully qualified variable names. (Do not mix up!)
I've found myself repeating these recommendations quite frequently the last few days.

[rvosa]

Thanks for your reply! Yeah, perhaps a "Best Practices" section in the Tutorials department could work too, but I was thinking specifically about a facility where people can submit code blocks for review, and I'm not sure if Tutorials is the appropriate place for that?

[blazar]

How 'bout Meditations? Then the discussion arisen from a meditation may evolve into a tutorial. I bet this has already happened. Hasn't it?

[aukjan]

Well... If you post in the Snippets Section, you will probably get some comments. Also (knowing a little about monks) they will pick at the code and suggest alternatives for it, which in some cases can spawn a discussion on why something is better/worse...

[davidrw]

The idea is that the code already works - i.e. not SoPW material -, is very much NOT obfuscated and might even be very UNCOOL, i.e. accountancy software.

But that's perfectly ok to post in SOPW, right? I've gotten the impression (i swore a higher monk mentioned it in a reply not too long ago--something along the lines of "ask for review in SOPW to get it finalized/touched up before submitting to Snippets" or something like that) that code review requests should go to SOPW (as opposed to Snippets or Meditations)... Which seems to make sense if you consider the question to be of the form "I have this working code that does ___ -- what 'better' or different way would you do it?".

[talexb]

my $fh;
open($fh, '<', $filename) or die "can't open $filename: $!"

Maybe it's the QA or documentatino guy in me coming out (Back! back!), but I'd much rather see the error message say something like Can't open $filename for reading: $!. Then again, I find it's very rare that I have a filehandle open longer than a dozen lines of code, so I'm not sure I would ever have need of your improvement.

[ady]

Considering the TMOWTDI fuzzines of Perl (and also having just read the Enterprise Perl thread in perlmeditation) i think a more interesting initiative than a code review section on Perl Monks could be a tool to support Perl code reviews, -- something more potent than a Perl beautifier or a Perl Syntax Checker or a simple Perl Lint, -- more a configurable Perl code analysis, review and conformance evaluation tool, somewhat like FxCop for .NET.
===========================================================
As the eternal tranquility of Truth reveals itself to us, this very place is the Land of Lotuses
-- Hakuin Ekaku Zenji

[radiantmatrix]

I use something like this:

package OpenOrDie;
use strict; use warnings;
use vars '@ISA @EXPORT $VERSION';

require Exporter;

$VERSION = '0.30';
@ISA = 'Exporter';
@EXPORT = 'Open';

sub Open {
    my ($mode, $file) = @_;
    
    open my $fh, $mode, $file 
        or die("Unable to open file '$file' using mode '$mode': $!")
    
    return $fh;
}

Which I then use like:

use OpenOrDie;

my $FILE = Open('<',$filename);

This also means I can catch the error consistently:

eval {
   my $FILE = Open('<',$filename);
}; if ($@) {
   warn("Cannot parse configuration; $@");
   $config{skip} = 1;
}

[merlyn]

Was Fatal too complicated, or something? After all, that's core now.

[radiantmatrix]

No.

Fatal overrides open; overriding I/O functions has been prohibited by coding standards nearly everywhere I've worked.

Also, the above is very much simplified from the live module I use (NDA and all that, can't be too careful): my actual code calls out to Log::Log4Perl and optionally calls out to Carp's croak or carp functions, falling back to warn if these can't be found.

AFAIK, Fatal doesn't do those things. Besides, even if I didn't have those requirements, most code I write falls under requirements such that CPAN modules must be used as-installed (no modification): since the functionality I require is so easy to duplicate, better to have it in a module I can actually extend later.

[adrianh]

Was Fatal too complicated, or something? After all, that's core now.

I've worked places where something like this was the preferred style. The argument went (and I think it's at least vaguely reasonable) that by using a different subroutine you could tell by looking at the code in question whether an exception would be thrown - rather than having to look at the top of the file for the use Fatal. It also had the advantage of breaking at runtime if you forgot to load the 'safe' module - whereas forgetting to load Fatal.pm meant the code continued to 'work'.