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

    by Ovid (2709)

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

    by Ovid (2709)

    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
                . ' – '
             
    • 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
• Too much work (Score:2)

  by brian_d_foy (44)

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

    by Ovid (2709)

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

      by hanekomu (8123)

      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.