Page 1 of 2

Swift Mailer wrapper class - critique

Posted: Sat Jul 29, 2006 9:37 am
by Benjamin
I wrote a small class for sending mail with d11's swiftmailer. I'm just wondering if it's good or if there are things I should improve. Just want to make sure I'm getting this oop thing down.

Code: Select all

class smtpMail()
{
    function __construct()
    {
        $this->mailer = new Swift(new Swift_SMTP_Connection(SMTP_SERVER));

        try
        {
            $this->authenticate();
        } catch (Exception $e)
        {
            // error handler
        }
    }

    private function authenticate()
    {
        if ($this->mailer->isConnected())
        {
            if ($this->mailer->authenticate(SMTP_USERNAME, SMTP_PASSWORD)) {
                return true;
            }
            throw new Exception('Could not authenticate.');
        }
        throw new Exception('Could not connect to smtp server.');
    }

    public funtion send()
    {
        $this->mailer->send();
    }

    function __destruct()
    {
        $this->mailer->close();
    }
}
PS d11, I modified the mailer a little bit so that I could use it with an autoloader. I put all the files in a single directory.

Posted: Sat Jul 29, 2006 10:26 am
by Chris Corbyn
:arrow: Moved to Coding Critique

Posted: Sat Jul 29, 2006 10:28 am
by Benjamin
Is this a new category?

Posted: Sat Jul 29, 2006 10:31 am
by Chris Corbyn

Code: Select all

class smtpMail()
{
What's the parentheses for? ;)

Why are you using constants as function paramters in OOP? It makes a lot more sense to pass these to the object.

Where do you set the email body and the addresses etc?
PS d11, I modified the mailer a little bit so that I could use it with an autoloader. I put all the files in a single directory.
Another change on the version 2 line up is correct PEAR naming. The only bit in the code at present that won't load automatically if you change the directory layout is the authenticators, but that what the loadAuthenticator() method is for :)

Posted: Sat Jul 29, 2006 10:32 am
by Chris Corbyn
astions wrote:Is this a new category?
Indeed :) It was planned a while ago in order to prevent the number of critique type snippets being placed in Code Snippets. You can post code you want feedback on in here.... or just to show off your proud creations :P

Posted: Sat Jul 29, 2006 10:57 am
by Ambush Commander
Hmm... from what I see, this is a convenience wrapper for the Swift mailer class if you want to specifically just want to send SMTP mail.

I notice that all the logic is in the constructor and destructor. If that really is the case, as long as you register Swift into a registry, this wrapper class should be unnecessary. All calls to Swift would be calling a single instance of the mailer.

I really can't comment anymore without knowing precisely why this class was created.

Posted: Sat Jul 29, 2006 11:11 am
by Benjamin
d11wtq wrote:What's the parentheses for? ;)
Habit, fixed
d11wtq wrote:Why are you using constants as function paramters in OOP? It makes a lot more sense to pass these to the object.
They are set in a config file, figured it would make them easier for the site admin to change that way..
d11wtq wrote:Where do you set the email body and the addresses etc?
Didn't include that code in the example, but they are sent through the send function as usual.
Ambush Commander wrote:I notice that all the logic is in the constructor and destructor. If that really is the case, as long as you register Swift into a registry, this wrapper class should be unnecessary. All calls to Swift would be calling a single instance of the mailer.
I don't know what a registry is, but I have about 15 pounds worth of books on there way here so I will shortly. I created this class so that I can easily send mail from other classes.

Posted: Sat Jul 29, 2006 11:13 am
by Ambush Commander
I don't know what a registry is, but I have about 15 pounds worth of books on there way here so I will shortly. I created this class so that I can easily send mail from other classes.
Okay, the registry may be overkill.

Can I see a standard use case?

Posted: Sat Jul 29, 2006 11:26 am
by Benjamin
Well, that's only for one mail, I could also do it like this..

Psuedo

Code: Select all

$message = 'Welcome to blah ... click here to activate your accout ...';
$this->mailer = new smtpMail();

while ($something)
{
     $this->mailer->send($toAddress, '"Welcome" <welcome@domain.com>', 'Activate Account', $message);
}

Posted: Sat Jul 29, 2006 11:28 am
by Ambush Commander
Yeah, that's not right. According to your code right here, you're authenticating every time you instantiate a new smtpMail() class (one authentication should be enough!).

Make smtpMail a Singleton.

Posted: Sat Jul 29, 2006 11:33 am
by Benjamin
Oops, I mean to post another post but I accidently edited the post above..

Posted: Sat Jul 29, 2006 11:42 am
by Chris Corbyn
Ambush Commander wrote:Make smtpMail a Singleton.
Yes, you could do this. You'd then have a connection open to the server and already authenticated persistently throughout the life of your singleton. Then every time you need to send an email you literally just shout at the server to send an email :)

Posted: Sat Jul 29, 2006 11:54 am
by Benjamin
I'm assuming the mailer will only be called once per page. When sending multiple mails, I can reuse the instantiated class. Anything wrong with that?

Posted: Sat Jul 29, 2006 12:14 pm
by Ambush Commander
Only that it would be real easy to forget you called the mailer previously and open it twice.

Posted: Sat Jul 29, 2006 12:15 pm
by Benjamin
Ambush Commander wrote:Only that it would be real easy to forget you called the mailer previously and open it twice.
Ok, I'll make it a singleton then.