--- Nigel Peck wrote:
A cut down version of my code goes like this...
>
######################################################################
package MIS::Common::Image_magick;
use Image::Magick;
sub new {
my ( $class, $data ) = @_;
$data->{image_magick_object} = Image::Magick::new();
my $error = $data->{image_magick_object}->Read( $data->{path} );
croak $error if $error;
return bless $data, $class;
}
sub output {
my ( $self, $args ) = @_;
my $error = $self->{image_magick_object}->Write( $args->{path} );
croak $error if $error;
}
sub resize {
my ( $self, $args ) = @_;
$error = $self->{image_magick_object}->Resize( geometry =>
$args->{geometry} );
croak $error if $error;
}
sub crop {
my ( $self, $args ) = @_;
$error = $self->{image_magick_object}->Crop( geometry =>
$args->{geometry} );
croak $error if $error;
}
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
What could I do better?
What you're looking at is delegation and most of your code is
identical. I'd factor out the common bits. A first pass (not quite
compatible) looks like this:
package MIS::Common::Image_magick;
use strict;
use warnings;
use Carp 'croak';
use Image::Magick;
BEGIN {
my %delegate = map { $_ => ucfirst $_ } qw/read resize crop/;
$delegate{output} = 'Write';
while ( my ( $method, $delegate ) = each %delegate ) {
no strict 'refs';
*$method = sub {
my $self = shift;
my $result = $self->_magick->$delegate(@_);
croak $result if $result;
return $self;
};
}
}
sub new {
my ( $class, $data ) = @_;
my $self = bless {} => $class;
$self->_initialize($data);
}
sub _initialize {
my ( $self, $data ) = @_;
$self->{image_magick_object} = Image::Magick->new;
$self->read( $data->{path} );
return $self;
}
sub _magick { shift->{image_magick_object} }
1;
The problem with this approach is that the delegated methods require
the same same arguments as the target methods and that's not how your
code works. However, if users of your code realize this, they merely
need to consult the Image::Magick documentation to understand what
arguments are allowed.
If that is not desirable, it's not too hard to change the above code to
do what you want. You'd need a richer structure in your begin block to
specify how delegate methods arguments map to the the target method
arguments.
Cheers,
Ovid
