Stories
Slash Boxes
Comments
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

use Perl Log In

Log In

[ Create a new account ]

rjbs (4671)

rjbs
  (email not shown publicly)
http://rjbs.manxome.org/
AOL IM: RicardoJBSignes (Add Buddy, Send Message)
Yahoo! ID: RicardoSignes (Add User, Send Message)
I'm a Perl coder living in Bethlehem, PA and working Philadelphia. I'm a philosopher and theologan by training, but I was shocked to learn upon my graduation that these skills don't have many associated careers. Now I write code.

Journal of rjbs (4671)

Sunday October 21, 2007
07:47 PM

strict pbp creationists

[ #34726 ]

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!


List all Journal entries
The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.

strict pbp creationists 16 Comments More | Login | Reply /

 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • 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

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

      • 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
      --
      rjbs

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

      • 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 return statements?

        In any case, my point was that if an explicit return would make the code self-documenting.

        • 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:

              ...
              $self->finalize;
            }

            ...and $self->finalize returns $self. Now optimize returns self. Someone, via whatever means (possibly debugging, possibly reading the source, etc), notices this and writes:

              print $object->optimize->name, " optimized!";

            Sure, h

            --
            rjbs
      • 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]
        • 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.
          --
          rjbs
  • 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.

    Pe
    • 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
      --
      rjbs
    • 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.
  • 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.

    And after some fiddling with Email::Send
    • Email::Send is quite broken, and will probably be replaced by Email::Sender, when I have the tuits to finish its test suite.
      --
      rjbs
Stories, comments, journals, and other submissions on use Perl; are Copyright 1998-2006, their respective owners.