All the Perl that's Practical to Extract and Report

Detecting Long Methods Without PPI

Ovid on 2009-02-03T14:01:35

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.

Test::Smell?

Theory on 2009-02-03T16:41:46

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.

Re:Test::Smell?

vek on 2009-02-03T18:06:30

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?

Re:Test::Smell?

Ovid on 2009-02-03T18:11:56

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.

Re:Test::Smell?

Ovid on 2009-02-03T18:09:26

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

Do you have data backing that up?

btilly on 2009-02-03T17:12:53

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? and I have not read anything new on the topic since. So I still have the cognitive dissonance of feeling that short is better, but the concrete data I have says that the optimal subroutine length is much longer than I like.

Re:Do you have data backing that up?

Ovid on 2009-02-03T18:04:36

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. You're a great programmer and maybe longer methods may work better for you. That's OK. Shorter ones work better for me. That's OK too :)

Re:Do you have data backing that up?

btilly on 2009-02-03T22:09:11

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?

Re:Do you have data backing that up?

Ovid on 2009-02-04T13:59:03

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.