use Perl

• Code smell heuristic (Score:2)

  by jdavidb (1361)

  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
  • Re:Code smell heuristic (Score:2)

    by Ovid (2709) on 2008.04.29 11:42 (#62472) Homepage Journal

    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
                . ' – '
                . $c->stash->{report_metadata}{end}->day_month_year;
    
        if ($period eq 'today' || $period eq 'yesterday') {
            $group_by     = 'hour';
            $report_range = $c->stash->{report_metadata}{start}->day_month_year;
        }
    
        $c->stash->{group_by}     = $group_by;
        $c->stash->{report_range} = $report_range;
        return;
    }

    As you can see, we have a nasty conditional which basically throws away the work previously done. We could turn that into an if/else block and at least ensure that the assignments are only done once, but as it turns out, the core problem was this:

    sub report : Path {
        my ( $self, $c, @args ) = @_;
        $c->stash->{report_metadata}{period} = $args[0];
        $c->forward('_set_start_and_end_dates');
        $c->forward('_set_group_by');
    }

    First, we have the problem that the "period" can now be just about anything and we have to manually check it. Second, because we do things manually, we have that nasty if/else block. Instead, I wrote something like this:

    sub report_day : LocalRegex('^(yesterday|today)/$') {
        my ( $self, $c, @args ) = @_;
        $c->stash->{report_metadata}{period} = $c->req->captures->[0];
        $c->forward('_set_start_and_end_dates');
        $c->stash->{group_by} = 'hour';
        $c->stash->{report_range}
          = $c->stash->{report_metadata}{start}->day_month_year;
    }
    
    sub report_month : LocalRegex('^(last7days|month)/$') {
        my ( $self, $c, @args ) = @_;
        $c->stash->{report_metadata}{period} = $c->req->captures->[0];
        $c->forward('_set_start_and_end_dates');
        $c->stash->{group_by} = 'day';
        $c->stash->{report_range}
          = $c->stash->{report_metadata}{start}->day_month_year
            . ' – '
            . $c->stash->{report_metadata}{end}->day_month_year;
    }

    That's still pretty ugly, but the '_set_group_by' method goes away and the conditionals are eliminated. It also clarifies the code and groups the needed information together. After a lot of this was done, the simplification was revealing all sorts of underlying problems that our complex code was masking. The big win, though, is that we no longer need to check whether or not our 'period' is valid because that's handled by the method dispatching! (This is obscured in this instance because it's Catalyst, but Catalyst really can save you a lot of pain here)

    Reply to This

    Parent

    • Re: (Score:1)

      by theguvnor (1723)

      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?
      • Re: (Score:2)

        by Ovid (2709)

        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 ? "$
    • Re: (Score:1)

      by jplindstrom (594)

      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)
      
      • Re: (Score:2)

        by Ovid (2709)

        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.
    • Re: (Score:1)

      by Aristotle (5147)

      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 ) = @_;
      
      • Re: (Score:1)

        by jplindstrom (594)

        Ctrl-C + Ctrl-V