snippetphpMinor
Accept an object and generate a email
Viewed 0 times
emailacceptgenerateandobject
Problem
I have a question regarding optimizing a php email class. It's basically a wrapper class than needs to be able to accept different objects and always output an email. I'm trying to make it generic, in it's current form it's working but I still feel like it may be to tightly coupled. I was hoping someone here could make some suggestions on how to improve it. I would really appreciate it.
Different php file, auto load the Email class:
Take the Bill of Lading object that was created and email it
Take the tender object and email it
$tender = new Email();
```
$tender->body="Tender body";
$tender->displayName='Jon jonhnson';
$tender->fromEmailAddress = 'jraynor@aol.com';
$tender->toEmailaddress = 'jraxxor@xxxxxxx.com';
$tende
require_once 'PHPMailerAutoload.php';
class Email{
public $fromEmailAddress;
public $displayName;
public $body;
public $toEmailaddress;
public function sendEmail(Email $email){
$mail = new PHPMailer;
$mail->isSMTP(); // Set mailer to use SMTP
$mail->Host = 'exploud-xxxxx.ny.xxxxxx.com'; // Specify main and backup server
$mail->setFrom($this->fromEmailAddress, $this->displayName);
//$mail->addAddress($email);
$mail->addAddress($this->toEmailaddress); // Add a recipient
$mail->addCC($this->fromEmailAddress);
$mail->WordWrap = 50; // Set word wrap to 50 characters
$mail->isHTML(true); // Set email format to HTML
$mail->Subject = "Load Tender " . " - " . $this->displayName;
$mail->Body = $this->body;
if (!$mail->send()) {
echo 'Message could not be sent.';
echo 'Mailer Error: ' . $mail->ErrorInfo;
exit;
}
return true;
}
}Different php file, auto load the Email class:
Take the Bill of Lading object that was created and email it
$bol = new Email();
$bol->body="Bill of Lading body";
$bol->displayName='Jon jonhnson';
$bol->fromEmailAddress = 'jraxxxxor@xxxxxx.com';
$bol->toEmailaddress = 'jraxxxxor@axxxxxandxxxs.com';
$bol->sendEmail($bol);
/**
*
*
*/Take the tender object and email it
$tender = new Email();
```
$tender->body="Tender body";
$tender->displayName='Jon jonhnson';
$tender->fromEmailAddress = 'jraynor@aol.com';
$tender->toEmailaddress = 'jraxxor@xxxxxxx.com';
$tende
Solution
The code is written in a nice, clean style.
It's easy to read like that, which helps the reviewer a lot, keep it up!
This sort of thing doesn't make much sense:
It's strange to pass an object of its own type as a method parameter.
Normally you would do such thing in a copy-constructor or some kind of clone or factory method, where you want to build a copy from the parameter.
But that's not the case here.
And in any case, the
Another big problem with this method is that it makes assumptions that are not enforced programmatically: before the method is called,
Here's a safer alternative:
There are no hidden assumptions here: the only way to call this method is by adding all parameters. You cannot forget any of them, the code won't compile.
Note that with this implementation the class becomes a utility class with no member data.
At which point it can be just a standalone function,
it doesn't really need to belong to a class.
Another issue is error handling:
So if email sending fails, you
Most probably you want to
and the caller should handle gracefully.
Even if this script runs as a batch program without a user friendly web interface,
the exit points of programs are best controlled in one place,
the caller of this method, or even higher in the call stack,
not buried inside utility methods.
It's easy to read like that, which helps the reviewer a lot, keep it up!
This sort of thing doesn't make much sense:
class Email {
// ...
public function sendEmail(Email $email) {It's strange to pass an object of its own type as a method parameter.
Normally you would do such thing in a copy-constructor or some kind of clone or factory method, where you want to build a copy from the parameter.
But that's not the case here.
And in any case, the
sendEmail implementation as it is doesn't use the $email parameter for anything: you can safely remove it.Another big problem with this method is that it makes assumptions that are not enforced programmatically: before the method is called,
body, displayName and other fields must be set. If you forget to set some of them, the program may still work and might not raise obvious errors, but you can experience nasty bugs. Here's a safer alternative:
public function sendEmail($fromEmailAddress, $displayName, $body, $toEmailaddress) {
// ...
}There are no hidden assumptions here: the only way to call this method is by adding all parameters. You cannot forget any of them, the code won't compile.
Note that with this implementation the class becomes a utility class with no member data.
At which point it can be just a standalone function,
it doesn't really need to belong to a class.
Another issue is error handling:
if (!$mail->send()) {
echo 'Message could not be sent.';
echo 'Mailer Error: ' . $mail->ErrorInfo;
exit;
}So if email sending fails, you
exit, effectively crashing the program.Most probably you want to
return false instead,and the caller should handle gracefully.
Even if this script runs as a batch program without a user friendly web interface,
the exit points of programs are best controlled in one place,
the caller of this method, or even higher in the call stack,
not buried inside utility methods.
Code Snippets
class Email {
// ...
public function sendEmail(Email $email) {public function sendEmail($fromEmailAddress, $displayName, $body, $toEmailaddress) {
// ...
}if (!$mail->send()) {
echo 'Message could not be sent.';
echo 'Mailer Error: ' . $mail->ErrorInfo;
exit;
}Context
StackExchange Code Review Q#64651, answer score: 4
Revisions (0)
No revisions yet.