All the Perl that's Practical to Extract and Report

Class::Sniff Notes

Ovid on 2009-02-05T10:15:58

Ben Tilly called me on my claim that long methods are a code smell. After doing some reading and thinking about it, I have to conclude that he's right: there's not a lot of evidence to back up this claim. Of course, if "long" is defined as greater than 200 lines or so, then things change, but 10, 20, 50 lines or fewer? Apparently, there's not much research to back that up. Code Complete (2nd Edition, not sure about the first) has a nice summary of various studies on this. I might just pull this bit of Class::Sniff. I'm not sure yet, but at the very least, I'm going to bump up the "long method length" default by quite a bit. However, it a few ways it can break, so I might just have to pull it completely if it's too unreliable.

He also pointed out a serious issue with my duplicate method detection:

for my $method (@methods) {
    no strict 'refs';
    *$method = sub {
        my $self = shift;
        return $self->{$method} unless @_;
        $self->{$method} = shift;
        return $self;
    };
}

Now there are plenty of "duplicate" methods declared in the same package, but clearly this isn't an issue. Lots of false positives here. I'll have to give this more thought. Maybe I should exclude duplicate methods if they're all from the same package?

I'd also love to be able to do this:

my $sniff = Class::Sniff->new({ classes => \@classes });

That would let us auto-generate a much larger graph, but the interface would change enough that I'm unsure if it's worth it.

why dump it?

jdv on 2009-02-05T15:12:42

You could just make it configurable and give it a decent default.

This particular piece of functionality seems like it could be good for Perl::Critic as well.

Re:why dump it?

Ovid on 2009-02-05T15:23:33

My only concern is that it turns out to be misleading. I don't want to be seen promoting shorter methods if it turns out they're not actually a good idea. Still, if I can make it work more reliably, I will try that. If I can't make it work reliably, then it's a moot point :)

Re:why dump it?

jplindstrom on 2009-02-05T17:14:51

Well if you encounter a very long method, it's probably not bad in general, it's probably bad for specific reasons, like:

It's doing more than one thing, and the things are probably unrelated (which also makes it more difficult to name properly).

Even if they're not unrelated, it's a missed opportunity to write self documenting code by giving the functionality a (method) name. There are probably documenting comments in there to demarcate the functionality anyway. If not, there should be. Well, unless the method was split up.

The scope in a long method is probably unecessarily large, making it more difficult to understand, grasp the consequences of, and refactor.

These are all things that are likely to be true for very long methods.

Re:why dump it?

Ovid on 2009-02-05T17:18:20

I agree with all of this and it's the reason I included this code smell. That being said, the research seems to indicate otherwise. The subjective experience of many programmers seems to be contradicted by the objective experience of quite a few studies on the subject. See Code Complete 2 for a full list.

Labeled Blocks

jackal on 2009-02-05T17:40:48

As a side note you may wish to electively reduce the penalty of whatever "long" is if its broken up with labelled blocks.

They're not necessarily *as* good as breaking it into smaller functions, but for cases where you would otherwise call the function only once I consider it an "acceptable" alternative.

Re:Labeled Blocks

Ovid on 2009-02-06T11:54:07

I don't know how to identify labelled blocks without PPI. That being said, given that the "long method" code smell turns out to be rather bogus, I'm not overly inclined to go much further there. Interesting point, though.