HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Wrapper class for PHPMailer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
wrapperclassphpmailerfor

Problem

Is this a right way to use/implement an external library into a project? If it's not how do you do it?

class Mail {
    protected $mail;

    public function __construct(PHPMailer $mail) {
        $this->mail = $mail;
        $this->mail->isHTML(Config::read('mail.isHtml'));
        $this->mail->setFrom(Config::read('mail.fromEmail'), Config::read('mail.fromName'));
    }

    public function sendMail ( $to, $subject, $body, $plainText ) {
        $this->mail->addAddress($to);
        $this->mail->Subject = $subject;
        $this->mail->Body = $body;
        $this->mail->AltBody = $plainText;

        $this->mail->send();
    }
}


and use it in anywhere in your app like this?

$test = new lib\Mail(new PHPMailer);

$test->sendMail('test@domain.com', 'Subject', 'Hello username', 'Hello username' );

Solution

As @ChoiZ already said in a comment, I would not pass the PHPMailer object in the constructor.

It doesn't really have any benefits (it's not like you can - or would want to - just pass a different mail class), and it's additional work for the caller.

It also means that it will be hard to change the library you are using. With your Mail class, you created a nice interface for sending emails, but by passing the class that actually sends mails via constructor, you are binding yourself to that library.

Misc

  • The names body and plainText represent the same thing - just one with and one without HTML. It's generally best to name same things the same. So I would go with body/bodyPlain or htmlText/plainText to avoid confusion.



  • I would rename sendMail to mail, as the class is already named mail.



  • Your spacing is not consistent.



  • if there is not reason not to, fields should be private.

Context

StackExchange Code Review Q#109860, answer score: 4

Revisions (0)

No revisions yet.