Beefy Boxes and Bandwidth Generously Provided by pair Networks
Your skill will accomplish
what the force of many cannot
 
PerlMonks  

problematic OO

by stonecolddevin (Vicar)
on Aug 03, 2005 at 08:50 UTC ( #480398=perlquestion: print w/ replies, xml ) Need Help??
stonecolddevin has asked for the wisdom of the Perl Monks concerning the following question:

Alright, I have this bit of code that's giving me some problems.

I'm passing two (or three, depending on it's usage) named parameters:
subject message to
if to is not passed, it defaults to my email address. The other two, however, are required.

My problem is neither of the two required are being included in the email that is sent to whoever (at this time it is only me) the email is sent to.
The Mail class:
package PL::Mail; use strict; use Mail::Sendmail; my $server = 'pop.dnvr.qwest.net'; my %mail; sub new { my $class = shift; my $self = {}; $mail{Smtp} = $server; $mail{From} = 'Postmaster <noreply@dhoss.cjb.net>'; $mail{To} = $self->{TO} ? $self->{TO} : 'Devin Austin + <devin.austin@gmail.com>'; $mail{Subject} = $self->{SUBJECT}; $mail{Message} = $self->{MESSAGE}; bless $self, $class; return $self; } sub send { my ($self, $user, $msg) = @_; my $date = Mail::Sendmail::time_to_date(); $mail{Message} .= <<"" Message sent on $date From $ENV{REMOTE_ADDR} All information contained in this message do not reflect the ideas of Devin Austin or MorningStarWeb and it's affiliates. ; if (sendmail %mail) { print STDERR $Mail::Sendmail::error; } else { print STDERR "Error sending mail ($date): $!"; } }

How it's called:
my $obj = PL->new; $obj->Mail->send( subject => 'Comments from ' . $q->param('name'), message => $msg );
Any ideas, wise monks?
meh.

Comment on problematic OO
Select or Download Code
Re: problematic OO
by greenFox (Vicar) on Aug 03, 2005 at 09:01 UTC

    I haven't run the code but it looks to me like your Mail class is looking for "SUBJECT" and "MESSAGE" and you are calling it with "subject" and "message"?

    Any-way why aren't you using one of the good mail modules from CPAN? :-)

    --
    Murray Barton
    Do not seek to follow in the footsteps of the wise. Seek what they sought. -Basho

      Case shouldn't matter with named parameters...at least afaik...
      Well MIME::Lite was having problems on my system...but i'm going to try Mail::Sender out, does that count? :-D
      meh.

        Case shouldn't matter with named parameters...at least afaik...

        I don't think that is correct, at least that is not what I am seeing :)

        FWIW I use Tilly's code to check named parameters.

        --
        Murray Barton
        Do not seek to follow in the footsteps of the wise. Seek what they sought. -Basho

Re: problematic OO
by castaway (Parson) on Aug 03, 2005 at 09:05 UTC
    Your problems are manyfold:
    • You call the new() sub without passing it any parameters, and inside the sub you attempt to read from $self, which is an empty hashref.
    • You pass the subject and message params to send as a hashref, yet they end up in the $user variable, which itself is never used
    You seem to be missing the fact that OO is not quite magic, methods are just ordinary subs, so you still need to read their parameters from what gets passed in. Is the subject supposed to be set on new() (ie object creation), or when you send the email?

    Also, you have a global %mail hash, which will stay the same for every PL::Mail object you create, which probably isnt what you want.. To make it unique per object, you need to store that data in $self.

    Also you need to be very careful about case sensitivity, $self->{SUBJECT} and $self->{subject} are not the same thing.

    Here is an attempt at repair:

    package PL::Mail; use strict; use Mail::Sendmail; ## Only global is your server since it doesnt change (?) my $server = 'pop.dnvr.qwest.net'; ## The new constructor just sets up the server and the From address si +nce these are static (could put the server name here directly and not + have a global variable) sub new { my $class = shift; my $self = {}; $self->{mail}{Smtp} = $server; $self->{mail}{From} = 'Postmaster <noreply@dhoss.cjb.net>'; bless $self, $class; return $self; } ## The send method is passed a hashref, containing subject, message an +d optionally a to key. ## eg $obj->send({subject => 'testing', message => 'just trying to sen +d mail', to => 'me'}); ## The $self hashref, which represents the object, is used to store al +l the parts of the %mail hash, which is then passed to sendmail sub send { my ($self, $data) = @_; $self->{mail}{To} = $data->{to} ? $self->{to} : 'Devin Austi +n <devin.austin@gmail.com>'; $self->{mail}{Subject} = $data->{subject}; $self->{mail}{Message} = $data->{message}; my $date = Mail::Sendmail::time_to_date(); $self->{mail}{Message} .= <<"" Message sent on $date From $ENV{REMOTE_ADDR} All information contained in this message do not reflect the ideas of Devin Austin or MorningStarWeb and it's affiliates. ; if (sendmail %{$self->{mail}) { print STDERR $Mail::Sendmail::error; } else { print STDERR "Error sending mail ($date): $!"; } }

    C.

Re: problematic OO
by polypompholyx (Chaplain) on Aug 03, 2005 at 09:17 UTC

    There are a few problems here. Mostly they seem to be to do with your understanding of what $self actually is. $self is generally a hashref, which contains your instance variables, like 'To' and 'Smtp'.

    At the moment, your new method blesses an empty hashref containing no data. $self should probably contain the data you currently assign to %mail. Something like...

    package PL::Mail; my $server = 'pop.dnvr.qwest.net'; sub new { my ( $class, $to ) = @_; my $self = { Smtp => $server, From => 'Postmaster', To => $to ? $to : 'devin.austin@gmail.com', }; # hashref containing instance variables return bless $self, $class; }

    There are similar problems in your send method. You are also calling the send method incorrectly. You want something like...

    my $obj = PL::Mail->new( $recipient ); $obj->send( subject => 'blah', comments => 'ftui' ):

    Note that because you are passing these parameters to send as a list, the way you handle send's arguments needs to be something like...

    my( $self, %params) = @_; $self->{ subject } = $params{ subject }; ...

    Don't be too disheartened: at least you're using  strict :) I think you could do with having a look at perltoot. Also, Mail::Sendmail is so simple, I'm not sure if you really need to be writing a wrapper for it...

      It is very simple....and I had thought of trashing my wrapper until I got it to work...
      meh.
Re: problematic OO
by umbra (Acolyte) on Aug 03, 2005 at 10:42 UTC
    I'm not OO Perl guru but in constructor (new sub) You declared $self as hashref but seems to me that You never initialize referred hash. All items like $self->{SUBJECT} or $self->{SUBJECT} are always undefined because You never assign something to them.

    Then when calling You should do
    PL::Mail->new();

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others drinking their drinks and smoking their pipes about the Monastery: (12)
As of 2014-09-18 11:15 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    How do you remember the number of days in each month?











    Results (112 votes), past polls