Beefy Boxes and Bandwidth Generously Provided by pair Networks
more useful options

OO Design question.

by Anonymous Monk
on Sep 27, 2011 at 13:52 UTC ( #928090=perlquestion: print w/ replies, xml ) Need Help??
Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

This is a simplified version of a class I have.
It does the job. It returns the correct object type for the parsed email and leads a profitable life.

I feel I've missed a trick. My OO design skills are poor crap. But hey, I'm a Unix Sys admin so for the Perl I write OO is usually a pointless overhead, especially one liners :-).

Seriously, I wonder if my parsing classes should be sub classes of a parent as they all have the same attributes (nearly) and only have the one method? So it would seem so but I'm not sure what benefit that would give me?

Also am I complicating things by returning objects of a different class? I think that my severity attribute is only there so I don't have to call ref $obj in the calling code.

As an aside I'm reading and since Ruby is Perl Lite I find it a readable and informative. Anyone know of a decent Perl design patterns reference?

package MailReader; BEGIN { push @INC, "U:\Perl"; } use Moose; use Harmless; use Critical; use UnSeen; has 'severity' => ( is => 'rw', isa => 'Str', default => 'unknown' ); has 'mailbody' => ( is => 'rw', isa => 'Str' ); sub parse_body { my $self = shift; # Look for Segmentation faults my $cm = Critical->new(data => $self->mailbody()); $cm->analysis(); if ($cm->is_critical) { $self->severity('critical'); return $cm; } # harmless my $hl = Harmless->new(data => $self->mailbody()); $hl->analysis(); if ($hl->is_harmless) { $self->severity('harmless'); return $hl; } # Still here then not seen this before my $us = UnSeen->new(data => $self->mailbody()); $us->analysis(); $self->severity('unknown'); return $us; } __PACKAGE__->meta->make_immutable; 1;

Comment on OO Design question.
Download Code
Replies are listed 'Best First'.
Re: OO Design question.
by JavaFan (Canon) on Sep 27, 2011 at 16:15 UTC
    From scanning the code, it looks to me you're analysing it up to three times. That just feels wrong to me, whether you're using OO or not. (And up to two times, you create an object, call a method on it, then the object is discarded).

    Also, you set a severity in $self, but isn't the severity already implied by the type of object returned from parse_body?

    I'd probably make it like:

    sub analysis { .... returns one of 'critical', 'harmless', 'unseen'; } sub parse_body { my $self = shift; $self->set_severity(analysis($self->mailbody))->severity; } sub set_severity { $_[0]{severity} = $_[1]; # Substitute your favourite OO implemen +tation $_[0]; } sub severity { $_[0]{severity}; }
      I also don't like repeated calls to the analysis method hence my original post.

      Severity is sort of implied from the returned object but its not a one to one mapping so I've left it as is.

      Basically I'm trying (and failing) to keep the balance between KISS and elegance (would they were the same).
      Thanks for your post.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://928090]
Approved by Corion
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others exploiting the Monastery: (6)
As of 2015-11-26 00:25 GMT
Find Nodes?
    Voting Booth?

    What would be the most significant thing to happen if a rope (or wire) tied the Earth and the Moon together?

    Results (693 votes), past polls