[jifty-devel] Janitorial: lib/Jifty/Request.pm

[Edgar Whipple]

Jesse,


On Sat, 2007-03-03 at 10:06 +0000, Jesse Vincent wrote: 
> > On Mar 3, 2007, at 4:19 AM, Edgar Whipple wrote:
[snip]
> I've done a preliminary review.   If you're going to be doing  
> reformatting, it probably makes sense to
> use perltidy for everything we can.   I've pasted my .perltidyrc (aka  
> the Jifty-canonical .perltidyrc ;) at t
> he end of this meesage. At the same time, we're fairly near the merge  
> of the T::D branch to trunk, so it
>   might make sense to wait until then before a major tidying run.
Agreed on all points. 

Your .perltidyrc is a subset of what I used. I would recommend using
yours for any automated tidy pass, with one addition. I propose adding
-csc (Closing Side Comments). This annotates the closing brace of a
large block. I did not use it in Request.pm, because it's a
non-whitespace change. However, as it helps readability *and* it's
guaranteed semantically harmless, I strongly recommend it for the
future.

It works like so:

    sub foo {
        # ... more than 6 lines will trigger CSC
    } ## end sub foo
      ^^^^^^^^^^^^^^ comment automatically added

I also find -iob useful (--ignore-old-breakpoints), but I would only
recommend it when the changes will be reviewed manually.

Last tidy point: I'll insert your .perltidyrc in the Style.pod once I
get my notes together (see separate post on Style Questions). If no
objection, I'll also add a suggestion about coders using format-skipping
tags (#<<<, #>>>) to preserve manual layout in small areas.

> > -- changed '\|' to '[|]' and '\?' to '[?]' in regexes.
> 
> Hm. I don't actually find that more readable, but I'm flexible. Other  
> folks?

I suspect I may be better qualified, due to proximity, to anticipate how
an inexperienced Perl-er will view a given snippet of code :-). Trust
me, this is less confusing to a newcomer's eye. (theDamian said so too,
so nyaah!)

> > -- added explicit 'return' to final expression in subs.
> 
> In trivial subs, I'd usually leave this off, but for something  
> complex, that seems reasonable

An instance of defensive programming. Any given trivial sub has a finite
non-zero probability of becoming non-trivial in the future. The problem
is, you can't tell in advance which subs are which.

Also, without the terminal return, those subs return a semantically null
value that is effectively random. Someone's eventually going to use that
value by accident or misunderstanding, and badness will follow.

I figure bare returns are free, so we might as well put them in
everywhere.

> > -- changed 'return undef' to bare 'return'.
> 
> Ok. I know it's bad style, but I prefer "return undef". I should  
> probably be broken of this preference. (IE, I don't think there's a  
> need to go wioth my preference)

Bad Perl hacker! Bad! No XP for you!

You can bet that someday, someone will inappropriately call your sub in
list context. BOOM! Bigger badness, very hard to trace. It's enough the
poor slobs have to troubleshoot their broken code; don't throw in an
erroneous data value to make it worse.

Come to think of it, perhaps a good item on the janitorial list would be
identifying the scalar-only subs and marking them with 'return scalar'.
(See PBP 186-188.)

> > -- in Jifty::Request->clone(), refactored clone creation out of  
> > 'bless'
> >    call into separate 'my $new_object', for clarity.
> 
> I don't actually find the temporary object to be a win, though better  
> formatting is probably worthwhile
> 

Again, please trust my young and innocent eyes. This is better for the
new reader.

> > -- transferred 'use strict' and 'use warnings' to the inside of each
> >    package. (Those pragmas are lexically scoped.)
> 
> 
> Keeping them outside the package declaration means they're going to  
> act on the whole file, which is a win.
> There's no need to repeat them in every package in a file. (And doing  
> so means there's a greater chance we'll
> miss one when adding a new package.
> 

Yeah, I wasn't sure about this one. I'll back it out.

Help me understand, though. I thought that a package declaration
terminated file scope. In other words, that unlike (unpackaged) blocks
and subs, the contents of a package are not a sub-scope of the file's
scope. Wrong?


Edgar
-- 
ln -s /dev/null ~/.sig