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)

Tuesday April 29, 2008
05:39 AM

OO Code and Conditionals

[ #36272 ]

A few weeks ago, we implemented a reporting system. Since we're huge believers in agile methodologies, we threw things together fairly quickly, using TDD. This exposed two problems. First, the "class" we implemented was a sloppy mess of procedural code with a OO wrapper. Second, the tests were a similarly sloppy mess of procedural code.

The last few days, I've had to extend this and I found it very hard to do. I've rewritten the tests to be data driven and I now iterate over a list of known states. They're much easier to work with and understand, but they also make it clear that some of our code has needless duplication.

The big win, though, was rewriting the "class" to actually be a class. Instead of huge methods with a bunch of if/else statements, I relied on proper method dispatching. The resulting code is shorter (278 lines of code are now 101 lines of code), easier to read, and exposed some confusion in the underlying thinking. The big issue, though, was that I was left with a single "if" statement:

if ( 'yesterday' eq $time_period ) {
    $date->subtract( days => 1 );
}

I went from eight "if" statements to only one. The previous "if" statements were all failures of our code to take advantage of the dispatching capabilities of OO. The final "if" statement, however, was bugging the heck out of me. I realize now that it means we've hobbled ourselves. What if we want three days ago? Nine days ago? It turns out that we really can't do that. Fixing this is easy but it does require a change in the API. Once again, conditional checks in OO code prove themselves to be code smells.

Legitimate conditional checks should (I'm not a purist) generally be on pure booleans, not calculated ones. The condition above is faulty, but the following condition is reasonable:

if ( $debug ) {
    $self->log($message);
}

That's a pure boolean and is the sort of thing I don't really have an issue with. Calculated booleans in OO code always give me pause.

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.
  • conditional checks in OO code prove themselves to be code smells.

    Thanks for the heuristic! I never realized that before! I think this will help improve my coding.

    (If there's a larger source where you picked that up, I'd be benefitted by a pointer.)

    --
    J. David works really hard, has a passion for writing good software, and knows many of the world's best Perl programmers
    • (If there's a larger source where you picked that up, I'd be benefitted by a pointer.)

      Thought I've later found that others point this out, I doubt how I learned this [perl.org] will help you :)

    • I figured a concrete example would help. Imagine this awful method from some Catalyst code I was working on:

      sub _set_group_by : Private {
          my ( $self, $c ) = @_;
          my $group_by = 'day';
          my $period = $c->stash->{report_metadata}{period};

          my $report_range
                  = $c->stash->{report_metadata}{start}->day_month_year
                  . ' – '
               

      • Thanks for the example, but I'm still confused... take pity on this dim one, and explain why the conditional check hasn't just moved elsewhere in your code where you decide which method to call? Or is that the entire point?
        • In this case, it's done via a customer selecting a URL, so it's up to the customer to decide what they want and we don't have to.

          A better example would be with Java:

          public void setName(String name) {
              this.name = name;
          }

          public void setName(String first, String last) {
              this.name = first + " " + last;
          }

          In regular Perl (no modules), it sometimes gets mistakenly rewritten as this:

          sub name {
              my ( $self, $first, $last ) = @_;
              $self->{name} = $last ? "$

      • Remove duplication. Name things. These things go together.

        Look at the method names. Do they really focus on what the methods do? Where is the axis of duplication? (hint: group by)
        • This was a tiny example and merely a first-pass refactor. If you look at the actual code I checked in, you'll see it's much cleaner.

      • sub report : Chained('somewhere?') PathPart('') CaptureArgs(0) {
            my ( $self, $c, @args ) = @_;
            $c->action->chain->[-1]->name =~ /\Areport_for_(.*)\z/
                or die 'Chain ends at action with malformed name';
            $c->stash->{report_metadata}{period} = $1;
            $c->forward('_set_start_and_end_dates'); # inline this?
        }

        sub report_day : Chained('report') PathPart('') CaptureArgs(0) {
            my ( $self, $c, @args ) = @_;

  • Shouldn't you get rid of the conditional with ->log? Just let the method figure out what to do. And where did $debug come from? Shouldn't that be a method call? :)
    • The conditional is a point I make in my OO guidelines writeup [perlmonks.org] on PM. It's bad practice. I make the same point here, too [perl.org]. So yes, I do agree with you, but I plead mea culpa of posting a bad example to explain good code ;)

      • You could get rid of the "if ($debug) { $self->log(...) }" by using Class::Null [cpan.org]. That is, the log object (assuming it's an object) should be Class::Null by default and a real logging object if $debug is set. Then just do "$self->log(...)" without the conditional, and if it's a Class::Null object, the call is more or less ignored. The documentation of Class::Null actually uses this example.