--- Nigel Peck <[EMAIL PROTECTED]> 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 -- Buy the book -- http://www.oreilly.com/catalog/perlhks/ Perl and CGI -- http://users.easystreet.com/ovid/cgi_course/ -- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] http://learn.perl.org/