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)

Thursday February 05, 2009
05:15 AM

Class::Sniff Notes

[ #38410 ]

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.

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

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

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

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