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('[email protected]', 'Subject', '<html>Hello username</html>', 'Hello username' );
2 Answers 2
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
bodyandplainTextrepresent the same thing - just one with and one without HTML. It's generally best to name same things the same. So I would go withbody/bodyPlainorhtmlText/plainTextto avoid confusion. - I would rename
sendMailtomail, as the class is already named mail. - Your spacing is not consistent.
- if there is not reason not to, fields should be private.
-
\$\begingroup\$
if there is not reason not to, fields should be private.I would caution against that - it turns "I know, I'll just extend that class and change x" into "why doesn't it work when I ...". \$\endgroup\$AD7six– AD7six2015年11月06日 09:27:07 +00:00Commented Nov 6, 2015 at 9:27 -
\$\begingroup\$ @AD7six I don't think so. Then it's "oh, I can't do it this way, let's look for a different way". However, if the field is public, and someone does extend the class and relies on the field, then the mailing library again cannot be changed, as it would break the extending class. I would say that exposing inner workings of a class leads to more problems then possibilities. \$\endgroup\$tim– tim2015年11月06日 09:46:07 +00:00Commented Nov 6, 2015 at 9:46
-
\$\begingroup\$
protected $mail;- It's protected, not public. I'm commenting on the suggestion to change a protected property to private. \$\endgroup\$AD7six– AD7six2015年11月06日 09:46:48 +00:00Commented Nov 6, 2015 at 9:46 -
\$\begingroup\$ Example for clarity i.sstatic.net/bIvOX.png \$\endgroup\$AD7six– AD7six2015年11月06日 09:52:15 +00:00Commented Nov 6, 2015 at 9:52
-
\$\begingroup\$ One last question. How can I change the code not bind myself into one library? what design pattern used for this problem? \$\endgroup\$Erdem Ece– Erdem Ece2015年11月12日 20:10:47 +00:00Commented Nov 12, 2015 at 20:10
The backend mailer class is just an implementation detail of your own Mail class - it shouldn't be necessary to have intimate knowledge of such details, but because it's a mandatory constructor argument this is currently unavoidable. It should be possible to change the mailer implementation and make no changes to your application/usage code (except in tests).
Another disadvantage to everything-as-constructor-arguments is that all classes in use are created eagerly. If the Mail class were to be instanciated but is ultimately unused - the Mailer object would also have been instanciated and unused. In this case that class doesn't do anything - but if it were also creating dependencies on construction, and if there is any setup logic executed this can easily mean a tree of objects get created before they need to be used. A practical example of how this can be a problem would be if the config is modified after construction but before first use:
$mail = new Mail(new PHPMailer);
...
Config::write('mail.isHtml', false); // would have no effect
$mail->sendMail(...);
The above example probably does not match your usage here - but may be applicable to other scenarios.
So, rather than having the mailer a mandatory constructor argument, have a method to get/set the mailer instance, generally this is an easier pattern to work with, example:
class Mail {
protected $mail;
public function mailer($instance = null) {
if ($instance !== null) {
$this->mail = $instance;
}
if (!$this->mail) {
$this->mail = new PHPMailer();
$this->mail->isHTML(Config::read('mail.isHtml'));
$this->mail->setFrom(Config::read('mail.fromEmail'), Config::read('mail.fromName'));
}
return $this->mail;
}
public function sendMail($to, $subject, $body, $plainText) {
$mailer = $this->mailer();
$mailer->addAddress($to);
$mailer->Subject = $subject;
$mailer->Body = $body;
$mailer->AltBody = $plainText;
return $mailer->send();
}
}
As well as making using the mail class easier, this has the advantage of the backend mailer class only being constructed on first use, it also makes testing easier:
/**
* Verify the headers and body when sendMethod is called
**/
public function testExample() {
$mailer = $this->getMockBuilder('PHPMailer')
->setMethods(['mailSend'])
->getMock();
$mailer->expects($this->once())
->method('mailSend')
->with(['expected header', 'expected body']);
$mailer->isHtml(false);
$mailer->setFrom('[email protected]', 'Example Example');
$mail = new Mail();
$mail->mailer($mailer);
$mail->sendMail('[email protected]', 'Subject', '<p>Body</p>', 'Body');
}
There are other minor details which could change - but adopting this pattern will generally lead to more maintainable code.
$mailer = new PHPMailer;and$test = new lib\Mail($mailer);\$\endgroup\$