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.
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.
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.
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
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
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.
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.
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.
$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.
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!).
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