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 ]

Ovid (2709)

Ovid
  (email not shown publicly)
http://publius-ovidius.livejournal.com/
AOL IM: ovidperl (Add Buddy, Send Message)

Stuff with the Perl Foundation. A couple of patches in the Perl core. A few CPAN modules. That about sums it up.

Journal of Ovid (2709)

Tuesday February 03, 2009
09:01 AM

Detecting Long Methods Without PPI

[ #38400 ]

How to determine the length of a method.

my $start_line    = B::svref_2object($coderef)->START->line;
my $end_line      = B::svref_2object($coderef)->GV->LINE;
my $method_length = $end_line - $start_line;

The $start_line returns the line number of the first expression in the subroutine, not the sub foo { ... declaration. The subroutine's declaration actually ends at the ending curly brace, so the following method would be considered 3 lines long, even though you might count it differently:

sub new {
    # this is our constructor
    my ( $class, $arg_for ) = @_;
    my $self = bless {} => $class;
    return $self;
}

This is a very dodgy and experimental hack, but as you might guess, long methods are ... wait for it ... a CODE SMELL!

So yes, I've just added this to Class::Sniff. I've set the default method length to a generous fifty lines, but you can set it yourself in the constructor:

my $sniff = Class::Sniff->new({
    class         => 'My::ResultSource::Customer',
    ignore        => qr/^DBIx::Class/, # we're only checking our own code
    method_length => 20,
});
print $sniff->report;

Note that this won't work with exported methods or any method where a reference is taken prior to the method declaration, so I just skip those.

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • I can see where you're going with this, Ovid. Can Test::Smell be far behind (um, pun not intended)?

    I can see it testing all of the classes in your project and emitting passes or failures for inheritance patterns, hidden method, long method bodies, etc.

    • I'm not so sure a code smell should be a test failure. Perhaps Perl::Critic is where Ovid's sniff logic would be put to best use?

      • This is like Critic, but I'm using running code, not document analysis. Therefore, I can do something things which Perl::Critic cannot (just as it can do some things which I cannot). For example, it's very, very hard to get a good inheritance tree from Critic. There are a number of obscure ways of setting up inheritance (see DBIx::Class and Catalyst as examples) which PPI wouldn't find terribly well.

    • Wow. I hadn't thought of that. Personally, I'd prefer someone else write that. These are good rules of thumb, but they don't satisfy the binary nature of tests :)

  • I've seen plenty of claims that long methods are a code smell. I've repeated such claims myself. But are you aware of any actual data demonstrating that short methods are better? I am not, and in fact the highly limited data that I am aware of says the exact opposite.

    For what it is worth, the last time I tried to get my thoughts down on this subject was several years ago in Short routines matter more in OO? [perlmonks.org] and I have not read anything new on the topic since. So I still have the cognitive dissonance of

    • As I've said before, it's a code smell, not a code error. If your method is 100 lines long but it's doing one, cohesive thing, maybe it's not an error. Maybe trying to abstract everything into methods from that would hurt readability. Maybe it does three things but you're happy with that. Your mileage may vary.

      If you don't like the heuristic, set your threshold to 5000 or something and don't worry about it. I'm just trying to provide a tool and I expect people to evaluate its utility for themselves. Y

      • I think you miss my point. It is not that I want to have long subroutines. It is that I prefer short subroutines, but am concerned that the concrete data I have says that I really should be using longer ones.

        So which is right? My gut sense? Or the data?

        • After doing a bit more reading on this (reading Code Complete's section, for example), it does sound like you're right and perhaps this is something wrong to put in here. This, coupled with some implementation issues, may lead me to pull this.

          Thanks for raising this issue.