Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

    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 = '[email protected]';
$bol->toEmailaddress = '[email protected]';

$bol->sendEmail($bol);
/**
 *
 *
 */

Take the tender object and email it $tender = new Email();

$tender->body="Tender body";
$tender->displayName='Jon jonhnson';
$tender->fromEmailAddress = '[email protected]';
$tender->toEmailaddress = '[email protected]';

$tender->sendEmail($tender);
share|improve this question

2 Answers 2

Essentially you're reinventing a very large wheel. You "wrapper" class has done no more than add configuration, which could easily be applied without a single functioned class. I think the best way to optimize this, would be to either scratch the class aspect, or implement one of the many other stable and supported email utilities available.

However, it's up to you. So I'll comment on the code you do have for now!

  • the class keyword has one too many indentations, and there should be a space before that opening bracket. It's always best to follow standards.
  • Those variables have no reason to be public. Use setters and getters to accomplish that. A simple Google as to why will give you thorough explanations.
  • Do you always want to CC the sender? I think this could have a potential negative affect at some point.
  • A word wrap of 50 seems very narrow. Are you sure you want to have it this small? On a smaller screen this may act okay, but on a large screen device this would display awkwardly.
  • Your subject line is very "coupled". That line just took your class from somewhat localized to very localized! Anytime you want to send an email with a different subject line, you have to change the class! Bad.
  • You've mixed your presentation with your business. Having echos is limiting your program. I suggest just having

    return $mail-send();
    

    This way the code calling the class can decide how to present the response, whether it be a success or a failure.

  • You do have too much white space in the last few lines. It's quite distracting and therefore decreases readability.

Overall, it seems like nice looking code (not nice functioning code, though). The biggest issue is that it's very localized and in fact quite error prone. Developing with this in a year would be a huge pain in the ass! :)

share|improve this answer

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:

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.