Swift Mailer wrapper class - critique

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Swift Mailer wrapper class - critique

Post 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.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

:arrow: Moved to Coding Critique
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Is this a new category?
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post 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 :)
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post 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
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post 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.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post 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?
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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);
}
Last edited by Benjamin on Sat Jul 29, 2006 11:33 am, edited 1 time in total.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post 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.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Oops, I mean to post another post but I accidently edited the post above..
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post 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 :)
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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?
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Only that it would be real easy to forget you called the mailer previously and open it twice.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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.
Post Reply