views:

353

answers:

7

I need to write a storage related app in Perl. The app needs to upload files from the local machine to some other storage nodes. Currently, the uploading method is FTP, but in the future it may be bittorrent or some unknown super-file-transferring method.

For every file that needs to be uploaded, there is a configuration file which defines the file name, the storage node which the file will be uploaded to and what transferring method should be used during the uploading.

Of course, I can use the following method to solve my problem:

{
  if ( $trans_type == "ftp" ) { ###FTP the FILE}
  if ( $trans_type == "bit" ) { ###BIT the FILE}
  ### etc ###
}

But even with my basic OO knowledge learned in school, I still feel that this is not a good design. (The question title might be a little bit misleading. If you think my problem can solved gracefully with a non-OO solution, it's quite OK for me. Actually it will be better, since I have limited OO knowledge.)

So could you guys give me some advice in general? Of course, if you provide some sample code as well, this will be a great help.

+10  A: 

First, string equality testing in Perl is eq, not ==.

If you have methods to do the work, say named bit and ftp,

my %proc = (
    bit => \&bit,
    ftp => \&ftp,
);

my $proc = $proc{$trans_type};
$proc->() if defined $proc;
xcramps
I would recommend adding a bit more description of what is going on here just in case, but still good answer.
Chris Lutz
No need for defined because none of the false values is a valid coderef. Also, you should emit a warning if the method cannot be found in the lookup table. An alternative is to put all methods in a class and use `can`.
Sinan Ünür
@Sinan Ünür- What about if $trans_type eq "fronobulax?" In other words, a type he wasn't expecting, or hadn't anticipated?
xcramps
Chas. Owens
Use an exists check, otherwise the assignment to $proc will alter your data structure. if( exists $proc{$trans_type}) {my $proc = $proc{$trans_type}; #validate $proc here; $proc->() }
daotoad
@daotoad: no, it won't. Perhaps you are thinking of `if ( $proc{$trans_type}{'process_file'} )` turning a nonexistent $proc{$trans_type} into a ref to an empty hash, but simply looking up a value in a hash won't create a hash element there.
ysth
@Chas: Jesus wept, I didn't fill in all the blanks for him. At least the code I posted is safe. If not, tell us why not.
xcramps
+1  A: 

OO would be overkill. My solution would probably look something like this:

sub ftp_transfer { ... }
sub bit_transfer { ... }
my $transfer_sub = { 'ftp' => \&ftp_transfer, 'bit' => \&bit_transfer, ... };
...
sub upload_file {
    my ($file, ...) = @_;
    ...
    $transfer_sub->{$file->{trans_type}}->(...);
}
tetromino
Chris Lutz
derobert
It is very rarely overkill to have some OO. And this example screems to be solved OO-wise.
innaM
+6  A: 

You can use a hash for this...

  1. Have each transfer method register itself in the hash. You can do this OO (by calling a method on some transfer method factory) or procedurally (just make the hash a package variable, or you could even put it in the main package if you don't want to modularize).

    package MyApp::Transfer::FTP;
    $MyApp::TransferManager::METHODS{ftp} = \&do_ftp;
    sub do_ftp { ... }
    1;
    
  2. Each transfer method uses a consistent API. Maybe its just it a function, or it could be an object interface.

  3. Call the transfer through the hash.

    sub do_transfer {
        # ...
        my $sub = $MyApp::TransferManager::METHODS{$method}
            or croak "Unknown transfer method $method";
        $sub->($arg1, $arg2, ...);
        # ...
    }
    

BTW: The OO register method would look something like this:

package MyApp::TransferManager;
use Carp;
use strict;

my %registered_method;

sub register {
    my ($class, $method, $sub) = @_;

    exists $registered_method{$method}
        and croak "method $method already registered";

    $registered_method{$method} = $sub;
}

# ...

1;

(None of this code is tested; please forgive missing semicolons)

derobert
a hash still has the problem that you are listing the possible transfer agents. There is no reason to hard code this list. Just create TransferAgent::FTP, TransferAgent::SCP, TransferAgent::BitTorrent, etc. A factory class can then be responsible for instantiating the right class.
Chas. Owens
@Chas. Owens: Where am I hardcoding the list? Each method implementation is responsible for registering itself. Its fairly easy to have a config file specify which transfer modules to load (if you want that level of customization, e.g., maybe you want to turn off a very dependency-heavy module) or load all .pm files in a given directory (if you want that level of magic)
derobert
@derobert How do the individual classes get themselves run? If I have a program that needs to transfer to multiple server types, do I have to specify each type as a separate `use` statement in my program? Classes can't register themselves until they are used. That means somewhere you are hardcoding what classes a given program can use (such as the config file you pointed out). By requiring a class only when it is asked for you don't need that sort of hardcoding.
Chas. Owens
Package variables *are* global.
jrockway
@Chas. Owens: I think you missed the last part, of loading all the .pm files in a given directory (e.g., treat them like plugins). One example of doing things that way: Catalyst. Even if you do go with explicitly listing in a config file, that's not so bad, as you already were doing it (your config needs to give connect details).<br><br>@jrockway Yes, you're right, they are. I'll fix...
derobert
@Chas. Owens: See, for example, Module::Pluggable
derobert
So you are going to load every possible implementation even though you only need one?
Chas. Owens
@Chas. Owens: I can take my pick — I can either specify in a config file which ones to load, or I just load them all, because loading a few extra packages doesn't take that long, anyway and I'm not likely to have many unused ones.
derobert
+5  A: 

The correct design here is a factory. Take a look at how the DBI handles this. You will wind up with a TransferAgent class that instantiates one of any number of TransferAgent::* classes. Obviously you will want more error checking than the implementation below provides. Using a factory like this means that you can add new types of transfer agents without having to add or modify any code.

TransferAgent.pm - the factory class:

package TransferAgent;

use strict;
use warnings;

sub connect {
    my ($class, %args) = @_;

    require "$class/$args{type}.pm";

    my $ta = "${class}::$args{type}"->new(%args);
    return $ta->connect;
}

1;

TransferAgent/Base.pm - contains the base functionality of a TransferAgent::* class:

package TransferAgent::Base;

use strict;
use warnings;

use Carp;

sub new {
    my ($class, %self) = @_;
    $self{_files_transferred} = [];
    $self{_bytes_transferred} = 0;
    return bless \%self, $class;
}

sub files_sent { 
    return wantarray ?  @{$_[0]->{_files_sent}} : 
     scalar @{$_[0]->{_files_sent}};
}

sub files_received { 
    return wantarray ?  @{$_[0]->{_files_recv}} : 
     scalar @{$_[0]->{_files_recv}};
}

sub cwd    { return $_[0]->{_cwd}       }
sub status { return $_[0]->{_connected} }

sub _subname {
    return +(split "::", (caller 1)[3])[-1];
}

sub connect    { croak _subname, " is not implemented by ", ref $_[0] }
sub disconnect { croak _subname, " is not implemented by ", ref $_[0] }
sub chdir      { croak _subname, " is not implemented by ", ref $_[0] }
sub mode       { croak _subname, " is not implemented by ", ref $_[0] }
sub put        { croak _subname, " is not implemented by ", ref $_[0] }
sub get        { croak _subname, " is not implemented by ", ref $_[0] }
sub list       { croak _subname, " is not implemented by ", ref $_[0] }

1;

TransferAgent/FTP.pm - implements a (mock) FTP client:

package TransferAgent::FTP;

use strict;
use warnings;

use Carp;

use base "TransferAgent::Base";

our %modes = map { $_ => 1 } qw/ascii binary ebcdic/;

sub new {
    my $class = shift;
    my $self  = $class->SUPER::new(@_);
    $self->{_mode} = "ascii";
    return $self;
}

sub connect    { 
    my $self = shift;
    #pretend to connect
    $self->{_connected} = 1;
    return $self;
}

sub disconnect {
    my $self = shift;
    #pretend to disconnect
    $self->{_connected} = 0;
    return $self;
}

sub chdir { 
    my $self = shift;
    #pretend to chdir
    $self->{_cwd} = shift;
    return $self;
}

sub mode {
    my ($self, $mode) = @_;

    if (defined $mode) {
     croak "'$mode' is not a valid mode"
      unless exists $modes{$mode};
     #pretend to change mode
     $self->{_mode} = $mode;
     return $self;
    }

    #return current mode
    return $self->{_mode};
}

sub put {
    my ($self, $file) = @_;
    #pretend to put file
    push @{$self->{_files_sent}}, $file;
    return $self;
}

sub get {
    my ($self, $file) = @_;
    #pretend to get file
    push @{$self->{_files_recv}}, $file;
    return $self;
}

sub list {
    my $self = shift;
    #pretend to list remote files
    return qw/foo bar baz quux/;
}

1;

script.pl - how to use TransferAgent:

#!/usr/bin/perl

use strict;
use warnings;

use TransferAgent;

my $ta = TransferAgent->connect(
    type     => "FTP",
    host     => "foo",
    user     => "bar",
    password => "baz",
);

print "files to get: ", join(", ", $ta->list), "\n";
for my $file ($ta->list) {
    $ta->get($file);
}
print "files gotten: ", join(", ", $ta->files_received), "\n";

$ta->disconnect;
Chas. Owens
I don't think you want that `use base "TransferAgent"` line in the FTP class. Especially since your factory connect method won't work in a derived class (will get the wrong value of class, or even worse an instance instead). Maybe you meant to use `__PACKAGE__` instead in your `require` and `new` lines?
derobert
You can also use Class::Factory from the CPAN for this. It's a pretty small module, but very easy to implement and use.
Chris Winters
@derobert Yes, it was late and I hadn't slept yet. The pattern should have a separate class to get the base functionality from (which is what I was intending for TransferAgent to be in addition to being the factory). I have corrected the code and fleshed it out a little now that I am awake.
Chas. Owens
@Chris Winters I have never used Class::Factory before. It looks interesting, but a quick glance seems to say it is no better than the hash solution. It looks like it requires you to register the classes that can be created by it. That defeats the main reason to use a factory class in my opinion (i.e. you don't need to know in advance what implementations might exist).
Chas. Owens
I like Module::Pluggable.
ysth
+1  A: 

You said initially it will use FTP and move to other transfer methods later. I wouldn't get "elegant" until you actually need to add the second or third technology. That second transfer method may never be required. :-)

If you want to do it as a "science project" then great.

I am tired of seeing OO design patterns complicating solutions to problems that never arrive.

Wrap the first transfer method in an uploadFile method. Add an if then else for the second method. Get elegant and refactor on the third method. By then you will have enough examples that your solution will probably be pretty generic.

Of course, my main point is that the second and third methods may never be required.

DanM
The problem with the I-will-make-it-nice-latter method is that by the time you need to make it nice there are a bunch of existing programs that are using the not-so-nice interface. Of course, you must always balance future needs against the simple need to get it done. In this case, the factory design pattern is well understood and it is fairly simple to implement and you will lose very little time providing a nice interface for the future.
Chas. Owens
+1  A: 

I have several examples in Mastering Perl in the sections on dynamic subroutines.

brian d foy