I liked Damian Conway's book Perl Best Practices. It had a lot of sound advice that can help a programmer or programming group decide on a set of house rules. For those who aren't interested in making a lot of decisions, it can even be used as a pre-built set of standards (although a few of its suggestions, generally those involving modules releaed by Damian for the book, are untenable).
Perl::Critic provided a fantastic way to check your code gains the rules in PBP, or against many other kinds of rules, and it became quite popular.
What has begun to really drive me nuts is the application of these rules without regard to how they affect the program. More than once, now, I've seen code changed to comply with PBP, only to be completely broken as a result. To paraphrase Mark Jason Dominus, apparently it is important to these programmers to get the wrong answer as maintainably as possible.
Sure, tests would solve this problem, but Perl::Critic can't tell you that you have uncovered branches. It can just tell you that you separated statements with a comma, or that you named a method after a builtin.
Here's tonights example, which drove me to rant about this:
sub prepare_report {
my ($self, $file) = @_;
$self->analyze($file)->report;
}
Oh no! There's no explicit return statement! We better fix that or Perl::Critic will complain!
sub prepare_report {
my ($self, $file) = @_;
$self->analyze($file)->report;
return;
}
Ugh!
Re:Why "Ugh!"?
bart on 2007-10-22T11:13:28
In the absence of some explicit documentation that says the example function doesn't return anything, the change you show seems reasonable.I disagree. If it doesn't say it returns nothing, then you should assume it returns something.
In complete absence of info on what it returns, the code should thus become:sub prepare_report {
my ($self, $file) = @_;
return $self->analyze($file)->report;
}Re:Why “Ugh!”?
Aristotle on 2007-10-22T11:45:15
Mindlessly putting it one place is just as wrong as mindlessly putting it in another. If you don’t know where it goes, you don’t put it anywhere – you go and look at the docs and the rest of the codebase. If that’s not in the cards, for whatever reason, then you leave damn well alone – even if that means Perl::Critic continues moaning about it. Because it should. Filling in the paperwork just to make it happy without knowing what you’re doing is just self-deception.
Re:Why "Ugh!"?
rjbs on 2007-10-22T11:39:26
Well, first of all, there *were* explicit docs that said that text was returned.
Beyond that, though, this change caused the entire module to become completely useless, which means that it wasn't desk checked. Why not? Because the module wasn't undergoing any actual maintenance. In the course of other work, someone saw Perl::Critic complain about "no explicit return" and added one, without further testing.
Sure, automated testing would've caught this, but if there are no automated tests, you don't make changes without desk checking them. More importantly, if the code is working, you don't change it without a reason.
Maybe "improve maintainability" is a reason for some changes, but not until there are tests to ensure that your changes don't, you know, break the program.
The original code doesn’t make it obvious whether anything should be returned or not. Putting in an explicit return – assuming, of course, that you put it in the right place – definitely improves the code.
Re:In all fairness
waltman on 2007-10-22T11:30:15
Perhaps that argument might be true for a longer, more complicated subroutine. I agree that implicit returns can be confusing. Perhaps a better standard would be to ban them instead of requiring unnecessary return statements. Then it would be clear that a this function doesn't return anything useful.
I'll also note that rjbs didn't say what the subroutine's documentation says. Maybe it reads "Prepares a report and saves it in $file. Doesn't return a damn thing."
Re:In all fairness
Aristotle on 2007-10-22T11:39:11
Perhaps a better standard would be to ban them instead of requiring unnecessary return statements.
Euhm. How are you going to ban implicit returns without requiring
returnstatements?In any case, my point was that if an explicit
returnwould make the code self-documenting.Re:In all fairness
waltman on 2007-10-22T14:00:07
What I meant was having a standard that if the function returns a useable value, there should be an explicit return statement. If not, then it's OK to leave it out. This way there are fewer returns cluttering up your code, and the syntax is similar to languages like C and Java that have void functions.Euhm. How are you going to ban implicit returns without requiring return statements?Re:In all fairness
rjbs on 2007-10-22T14:22:48
The reason that requiring an explicit return is a good idea is that it prevents people from relying on accidental return values. Say I have a method "optimize." It exists only to alter the object, but it happens to end with:...and $self->finalize returns $self. Now optimize returns self. Someone, via whatever means (possibly debugging, possibly reading the source, etc), notices this and writes:...
$self->finalize;
}Sure, he shouldn't do that, but preventing other people from making simple mistakes is part of the job of a good programmer. If you don't document the accidentally useful return value, someone will use it. If you don't document the useless return value (say, false) it isn't going to be used.print $object->optimize->name, " optimized!";
It means, too, that if you leave it useless and undocumented, later you can make it useful and documented. If it was useful and undocumented to begin with, people's code will break when you change it -- even if you document it.
Sure, people shouldn't used undocumented features, but most people don't think of the RV of a routine as a secret.
Of course, you could always put, in each void-ish routine's docs, "Do not rely on the return value," but it's easier to have it return nothing useful.Re:In all fairness
jonasbn on 2007-10-22T11:39:34
The good thing about Perl::Critic, is that it make you think about your code, so you do not just put or leave out statements by habit.
I have just finished a project for a client, I have disabled 8 policies and have documented as to why I have done so in the POD.
So disable the policy and document it[-Subroutines::RequireFinalReturn]Re:In all fairness
rjbs on 2007-10-22T13:06:43
Absolutely! I love Perl::Critic, and have written my own policies to remind me to think about stuff. I suggest Perl::Critic readily -- as a diagnostic tool, not as a master.
Re:/xsm
rjbs on 2007-10-22T21:39:38
I'm torn. That's a good one, and more likely to be sort of insidious. I really take exception to some of the blanket "use wacky modules!" suggestions. version.pm tends, in my experience, to lead to heartbreak, or at least needlessly-inflated prereqs. The best case I saw was someone who'd uploaded a VERY simple, pure-core module to the CPAN, but then required version.pm and Smart::Comments. I mean, the beauty of Smart::Comments is that it won't be a prereq, isn't it?
And then there's... well, other modules. I'll stop before I get a head of steam going. Re:/xsm
jjore on 2007-10-23T00:23:30
I've personally hated the/xsm recommendation because it means I then have to hunt through the expression to find if there are any uses of ^, $, ., or whitespace that are being modified. It's far easier to look at an expression and see that its flags are only set when meaningful.
I think it's far easier anyway. I'm only one person.
Re:Guilty!
rjbs on 2007-10-25T03:30:36
Email::Send is quite broken, and will probably be replaced by Email::Sender, when I have the tuits to finish its test suite.