fireartist has asked for the wisdom of the Perl Monks concerning the following question:

I'm in the process of writing a module, and find that I have many subs that effectively do exactly the same thing, but to a different variable.
I would like to have the logic happening in only one sub, and the others using that to do the work.
I've thought of using a method such as in the [perdoc://perlboot|perlboot] tutorial, but the model doesn't match my structure closely enough.

Below is an extract from the module to show exactly what I've done, and an explanation of what I'd like to achieve.

package SQL::Builder::Select;
use strict;
use warnings;
use Exporter;
use Carp;
use vars qw / $VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS /;

$VERSION = 1.00;
@ISA = qw/ Exporter /;
@EXPORT_OK = qw / new column table /;

sub new {
    my $proto = shift;
  my $class = ref($proto) || $proto;
  my $self = {};
  
  bless ($self, $class);
  return $self;
}

sub column {
    my $self = shift;
    my $count = @_;
    
    for (@_) {
        push @{$self->{column}}, $_;
    }
    return $count;
}

sub table {
    my $self = shift;
    
    for (@_) {
        push @{$self->{table}}, $_;
    }
}

1;

[download]
The module is used like so, 
my $select = new SQL::Builder::Select;

$select->column( @cols );
$select->table( @tables );

[download]
As you can see, both column() and table() simply shove the arguments into a variable @{$self->{sub name}}.
I'd like instead to have a sub named something like _put_list() and have column() and table() call that sub.

I'd prefer if it didn't require changes in the use, as I'm already using this module in my code, but if it's worth it, then ok.
Any suggestions or help at all would be most appreciated, thanks.

Replies are listed 'Best First'.
Re: abstraction - reusing OO module subs
by dws (Chancellor) on Nov 21, 2002 at 09:39 UTC
    I have many subs that effectively do exactly the same thing, but to a different variable. I would like to have the logic happening in only one sub, and the others using that to do the work.

    What the two methods have in common is that they add things to lists. The lists are different. Adding things to lists is a pretty generic operation. You gain little by factoring out the list-adding code into a separate method.

    What you can do is simplify. Instead of writing 

        for (@_) {
        push @{$self->{column}}, $_;
    }

    [download]
    write     push @{$self->{column}}, @_; That reduces the amount of shared code (in lines) by 66%. Then, is it really worth creating a new method because two methods each have a single line of code that sort of looks the same? The effect would be to add lines of code, for no gain in functionality. That seems to me to be a step in the wrong direction.

[reply]
[d/l]
[select]
Re: abstraction - reusing OO module subs
by djantzen (Priest) on Nov 21, 2002 at 09:33 UTC

    I suppose you could write a generalized sub that utilizes either an extra parameter to name the the key/array you want to push to, or the calling sub's information available via caller. However, unless there's some important behavior in column and table that you haven't described that would end up in _put_list, I don't really see what the fuss is about. I'd recommend simply streamlining what you've got by dropping the for loop. What's wrong with push(@{$self->{table}}, @_)? This small amount of redundant code is better than a trivial subroutine.

    Also, if you mean to use this in an OO manner, exporting your subroutines is not necessary. And the way they're written they expect to be called as methods, not functions.

[reply]
[d/l]
[select]
      Thanks for your reply.
      Ok, I can see that there would be little benefit, especially when my for loop is reduced to a single line.
      The reason I has wanted to do it was because there's not just 2 subs that do this, but many. There's also another group of subs that do another similar function, again where only the variable name changes.
      However, I'm happy with the sub reduced as it has been now.

      ...And the way they're written they expect to be called as methods, not functions.
      Could you explain this please?
      I'm still working my way (repeatedly) through the perltoot, perltooc tutorials, and only slowly learning this 'OO' thang.
      Many thanks.
[reply]
[d/l]

        It is quite common in OO programming to have the sort of redundancy among methods that you're worried about. It's a natural consequence of the emphasis on encapsulation that you end up with many similar getters and setters (or accessors/mutators) to mediate interaction with variables. To alleviate the tedium some people do things such as write modules like Class::MethodMaker to automate the process.

        As for functions vs methods, the main difference is how they are called. A function, once defined or imported into the current namespace, may be called directly by name. In contrast, a method must be called on either a package name or an instance of the package. See Re: Perl Prototypes for a fuller explanation.

[reply]
Re: abstraction - reusing OO module subs
by tomhukins (Curate) on Nov 21, 2002 at 10:28 UTC
[reply]
Re: abstraction - reusing OO module subs
by UnderMine (Friar) on Nov 21, 2002 at 10:26 UTC 
    sub _put_list ($@) {
   my $target = shift;
   my $self = shift ;
   push @{$self->{$target}}, @_;
   return scalar(@_);
} 

eval "sub $_ { return _put_list($_ => \@_);}" for qw{column table};

    [download]
    Note: Not 100% tested code.

    Hope this is of use.. Quite liked the dynamic evaluation but that is not neccessary. 

    sub column {
   return _put_list(column => @_);
}

    [download]
    Hope this helps
    UnderMine
[reply]
[d/l]
[select]

      Why AUTOLOAD exists: 

      my %accessors = map { $_ => 1 } qw( table column );

sub AUTOLOAD
{
    our $AUTOLOAD;
    $AUTOLOAD =~ s/::([^:]+)$//;
    return if $AUTOLOAD eq 'DESTROY';

    if (exists $accessors{ $AUTOLOAD })
    {
        no strict 'refs';
        *{ $AUTOLOAD } = sub {
            my $self = shift;
            push @{ $self->{ $AUTOLOAD } }, @_;
        };
    }
}

      [download]
[reply]
[d/l]

Back to Seekers of Perl Wisdom