Mechanical

Geek Alert: Detecting Bad OO Code

Having gotten a bit tired of seeing bad examples in OO code, I decided to write a tool which makes it easier to detect bad OO code in Perl. I call it Class::Sniff (also available on github). Currently it looks for and reports on the following:

  • Overridden methods (not always bad, but you want to see if you've overridden something you didn't know about)
  • Multiple inheritance
  • Unreachable methods
  • Exported subroutines (exporting croak into your class makes it available as a method)</p>
  • Duplicate subroutines (experimental "cut-n-paste" detection based on op trees)

Those are often "code smells" and I explain each on in the docs. Sample report after the cut.

Report for class: Grandchild

Overridden Methods
.--------+--------------------------------------------------------------------.
| Method | Class                                                              |
+--------+--------------------------------------------------------------------+
| bar    | Grandchild                                                         |
|        | Abstract                                                           |
|        | Child2                                                             |
| foo    | Grandchild                                                         |
|        | Child1                                                             |
|        | Abstract                                                           |
|        | Child2                                                             |
'--------+--------------------------------------------------------------------'
Unreachable Methods
.--------+--------------------------------------------------------------------.
| Method | Class                                                              |
+--------+--------------------------------------------------------------------+
| bar    | Child2                                                             |
| foo    | Child2                                                             |
'--------+--------------------------------------------------------------------'
Multiple Inheritance
.------------+----------------------------------------------------------------.
| Class      | Parents                                                        |
+------------+----------------------------------------------------------------+
| Grandchild | Child1                                                         |
|            | Child2                                                         |
'------------+----------------------------------------------------------------'
Exported Subroutines
.--------+---------+----------------------------------------------------------.
| Class  | Method  | Exported From Package                                    |
+--------+---------+----------------------------------------------------------+
| Child1 | croak   | Carp                                                     |
|        | inspect | Sub::Information                                         |
'--------+---------+----------------------------------------------------------'
Duplicate Methods (Experimental)
.---------------+-------------------------------------------------------------.
| Method        | Duplicated In                                               |
+---------------+-------------------------------------------------------------+
| Abstract::baz | Grandchild::bar                                             |
|               | Grandchild::quux                                            |
|               | Grandchild::foo                                             |
|               | Child2::bar                                                 |
|               | Abstract::bar                                               |
'---------------+-------------------------------------------------------------'
  • Current Mood: accomplished accomplished
Tags:
Uhm, isn't providing your a method in a subclass to override the parent one of the points of OO?

Correctly written OO code deals fine with that.

Note the comment above: "not always bad, but you want to see if you've overridden something you didn't know about". I just want people to be able to see a report on their class and say "hey, I didn't realize I overrode that!". On the other hand, if you have an inheritance hierarchy five levels deep and you have the same method defined five different ways, that is a code smell. It's a sign that maybe you have an inappropriate responsibility in a class.

Aside from those caveats, you're right :)

Depends. If you're using an OO package which enforces C3 MRO, then the 'unreachable' methods will be wrong. Also, "duplicate methods" will tend to pick up method auto-generators. You can ignore those if you see them, but I'm thinking about making an option whereby "duplicate" methods with different names aren't listed as duplicates, thereby not listing duplicate methods which are auto-generated.

Other than that, this is pretty much straightforward symbol table, @ISA walking, thus making it pretty generic (other than the fact that it's for Perl)