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