PHP e-mail sending class design

Question

I'm working on a class, what will be able to send e-mails for our customers. I need to help with review of my classes design. I'm trying to do it to be easy extendable in future. I'm thinking about using dependency injection to inject InputReaderInterface. But is it worth doing it?

• InputReaderInterface - has method: getContent()
• WebReader - for reading source e-mail HTML (implements InputReaderInterface)
• ParamSetter - for setting different params for users (implements InputReaderInterface)
• $smtp - class for saving access attributes for SMTP
• Mailer - uses PHPMailer class for sending e-mails

In the future, params will be loaded from the database.

<?php
$reader1 = new WebReader($mailUrl);
$message = $reader1->getContent();
$params = array(
 "[email protected]" => array(
 "customer" => "Name Surname"
 )
);
$reader2 = new ParamSetter($message, $params);
$mailer = new Mailer($smtp, $from, $alias);
foreach($rows as $row) {
 $reader2->setParamsFor($row["email"]);
 $message = $reader2->getContent();
 $mailer->sendMail($row["email"], $subject, $message);
}
?>

Answer 1

Naming

Your names are... I miss the word to describe your naming overall... Either way let's start.

Some of your names are good. $smtp, $from, $alias, $mailer and so on. Some of your names need improvement. $reader1 for example..
That reader would be better off as $mailReader or $messageReader or even $webReader.

Whenever you number your variables something is wrong..

Then there's your $reader2.. And this is what's breaking my understanding..

You called the Class it has ParamSetter, but you make it implement a InputReaderInterface, and call the variable $reader...

Make up your mind. Is that thing a setter, a reader or something entirely different? It definitely looks like it's something different. ParameterSetter seems to reflect it's usage. Having it implement InputReaderInterface is definitely the wrong choice.

Concept

Actually your whole concept of OOP seems to be incorrect. I'd expect $message to be a Message and not a "String". That would allow you to restructure your code to something like this, which looks waay more OOP to me:

<?php
 $messageReader = new WebReader($mailUrl);
 $message = new Message(messageReader->getContent());
 $params /* how you get your params at all... */
 $paramReader = new ParamReader($params);
 $mailer = new Mailer($smtp, $from, $alias);
 foreach($rows as $row) {
 $message->applyParameters($paramReader->getParameters($row['email']));
 $mailer->sendMail($row['email'], $subject, $message->getContent());
 }
?>

Of course this means you'd have to change some underlying mechanics. Also I just dropped your InputReaderInterface. In that setup it's not really helpful, as you use your classes differently.

Your ParamSetter was a ParamReader and some Message-functionality. Keep concerns separate, the ParamSetter does not need to know how a message works, and what parameters it wants. He shouldn't even tamper with the message. That would be a violation of the "Principle of Least Knowledge", which is a very important principle for OOP designing.

Instead the only one altering the $message should be the $message itself. Thus your ParamSetter in fact just wants to be a Reader, who is used by the Message.