strict pbp creationists

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.

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
Without JavaScript enabled, you might want to
use the classic discussion system instead. If you login, you can remember this preference.

Please Log In to Continue

I've had to fix enough code where the original programmer's intent wasn't clear, or is lost in the mists of time.In the absence of some explicit documentation that says the example function doesn't return anything, the change you show seems reasonable.

And if you have explicit documentation that says the example function doesn't return anything, the change also seems reasonable -- it guarantees that someone using your interface can't ignore the documentation, depend on undocumented functionality, and force y

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.

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 cha

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.

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."

Euhm. How are you going to ban implicit returns without requiring return statements?

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.

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:

My favorite PBP worst practice is adding/xsm to every regular expression. All of a sudden, regexes start to behave oddly, and when asked about this in code reviews, people can only say "Damian says to do that" but can't explain why they should or even what the options do (but hey, that's why I get paid to do code reviews:).It might be good to write new regexes with/x, but unless you want the features that/sm provides (that is, want to change the meaning of., ^, or $), you need to stay away from them.

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 modu

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.

Not quite that egregiously, but I did just get bit by such a critter. P::C griped about my "open(FILE, '>filename')" bits, not without justification, so I finally got around to remembering how they were supposed to work.So okay, I changed a bit of code that piped to sendmail (exim, actually).

But then, and here's where I ran into trouble, before I tested it I made some other cleanups, one of which involved deciding to replace the hardcoded sendmail with Email::Send.