Refactoring into oblivion

Ovid on 2005-12-04T07:17:57

So I stumbled across the following:

sub schema_for_class {
  my ( $self, $class ) = @_;
  return grep { $_ }
    $self->table_for_class($class),
    $self->indexes_for_class($class),
    $self->constraints_for_class($class),
    $self->view_for_class($class),
    $self->insert_for_class($class),
    $self->update_for_class($class),
    $self->delete_for_class($class);
}

That's a lot of repetition and therefore a lot of noise. So I reworked it a bit.

sub schema_for_class {
  my ( $self, $class ) = @_;
  return
    grep { $_ }
      map {
        my $method = "${_}_for_class"; 
        $self->$method($class)
      } qw(
        table
        indexes
        constraints
        view
        insert
        update
        delete
      );
}

Now I feel like a maintenance programmer can glance at it and get an idea of what's going on. But I do worry that sometimes I'm refactoring so aggressively that maybe it's getting too abstract. I do try and find a balance. I'm just not sure that I balance quite where others do.

---

A Fine Refactoring

chromatic on 2005-12-04T07:44:32

I did a similar refactoring just tonight with far fewer similar method names.

Re:A Fine Refactoring

Ovid on 2005-12-04T09:15:39

With all due respect, I know you're going to get it. I've seen your work (hell, I've worked with you) and I know that you won't blink when you see that. I just worry about the poor maintenance programmer. I'm not too terribly worried as I think that the maintenance programmer who works on this code had better be pretty damned nimble with the Perl chainsaw in the first place, but then, I'm so used to seeing this that I've no idea if it's obscure or not.

Drawing the line

robin on 2005-12-04T16:03:05

You're right that different people will draw the line in different places. I think that the original is actually better code, because it's immediately obvious what it does. The refactored version requires a bit of “decoding”.

The important thing is that you’re thinking about it rather than just refactoring it because you can. (And I guess that this example must be close to your own borderline, or you wouldn't be asking about it.)

Re:Drawing the line

djberg96 on 2005-12-04T16:34:31

I agree that the original is better as well, unless the refactored version is significantly faster somehow (and that's an oft repeated operation).

Re:Drawing the line

Theory on 2005-12-04T17:03:51

Thirded. The new version is much more abstract for no really good reason that I could see.

—Theory

Re:Drawing the line

ChrisDolan on 2005-12-04T18:29:36

Fourthed. The second version is worse for searchability -- say, if I want to find all instances of "delete_for_class" in my code.

Re:

Aristotle on 2005-12-04T22:04:41

Only if he actually composes them as strings. The version I posted has the clever bits save for interpolation, and searchability wouldn’t suffer with it.

Re:Drawing the line

malte on 2005-12-04T19:40:02

Fifthed: You shouldn't put method names in vars unless you absolutely have to.

unnecessary

lachoy on 2005-12-04T18:58:02

fifthed: IMO you've taken something very straightforward and made it something to stumble over for no real benefit. And Chris's note about making your method calls ungreppable (or ctaggable) should make this an easy undo.

Re:

Aristotle on 2005-12-04T21:57:41

I share your instinct and to refactor this, but your refactored version would strike me as odd immediately upon finding it: why grep { $_ } map { $foo } when that’s exactly the same as grep { $foo }?

And I think the interpolation is just too clever. It also makes it unnecessarily hard to add or remove method calls that may not follow this naming scheme, should that ever be necessary. So I’d simply say

sub schema_for_class {
    my $self = shift;
        my ( $class ) = @_;
    return grep {
        my $method = $_;
        $self->$method( $class );
    } qw(
        table_for_class
        indexes_for_class
        constraints_for_class
        view_for_class
        insert_for_class
        update_for_class
        delete_for_class
    );
}

(This is another case where I wish that aliasing constructs other than foreach made it possible to name the iterator instead of leaving it at “$_ is what’s for dinner, bud.”)

Re:

Aristotle on 2005-12-04T22:00:12

(No, I didn’t intentionally forget to convert the spaces to a tab in my ( $class ) = @_;.)

Re:

bpphillips on 2005-12-05T12:51:21

Isn't the return value from your version simply going to be:
qw(
                table_for_class
                indexes_for_class
                constraints_for_class
                view_for_class
                insert_for_class
                update_for_class
                delete_for_class
);
?

You can't introduce side affects in a grep block. $_ always comes out the left side regardless of what happens inside the grep block (which is why you would want a grep {$_} map {$foo} pattern).

Re:

Aristotle on 2005-12-05T14:19:47

Ack! Of course. Sigh.

I guess one could use

map {
    my $method = $_;
    $self->$method( $class ) || ();
} @method;

but I’m not sure how well advised that is…