2
\$\begingroup\$

I'm playing around with trying to do things in a better way, and in a class I have for sending emails, I have the concept of a recipient. This gets passed into my other classes essentially as a struct that is a little more descriptive than an associative array.

I'd like to know if the Recipient is going to be valid to pass to the rest of my code, and hence want a way to determine if the email address has any basic errors in it. (I'm aware that validating emails can only be done by sending them an email, for the purposes of this, just ignore this fact!)

Anyway, I have this class that looks like this:

class Recipient
{
    public $email_address, $name;

    public function __construct($email_address, $name) {
        $this->email_address = $email_address;
        $this->name = $name;
    }

    function emailIsValid() {
        return filter_var($this->email_address, FILTER_VALIDATE_EMAIL);
    }

    public function isValid() {
        return ($this->emailIsValid() && $this->name != '');
    }
}

And I'm calling it like:

$recipient = new Recipient('test@example.com', 'Example Name');

then, later when I'm about to send an email to something I can run:

if ($recipient->isValid()) {
    // add to the list of recipients
}

I'm interested on all and any feedback on this - I'm not that confident that this is a 'good' way to do things because:

  1. This class doesn't really do anything, and is really a pretty struct
  2. The validation isn't specific to this place, I may want to check emails in other places
  3. Should the validation happen in the constructor? Should it be a separate class? What would that look like?
  4. Should there be an interface for every class? Would that live at the top of this file?
  5. Should I throw exceptions when it's not valid? Should they be an object as well? How?
\$\endgroup\$
  • \$\begingroup\$ I saw this talk on PHP UK and think it answers your questions almost literally: youtube.com/watch?v=ZJ63ltuwMaE It's not short, but I really think you should take the time to watch it. \$\endgroup\$ – Pevara Aug 4 '14 at 20:20
7
\$\begingroup\$

Let's look at all of your questions, one by one:

This class doesn't really do anything, and is really a pretty struct

Yes and no. This class has a clear task within the application: it is passed around, carrying data. It represents a number of values that belong together. It's a model. Having data containers isn't just OK, it's good practice. You can use type-hinting, and doc-blocks to make your code easier to use.
You call it a glorified struct, well: in C, a FILE * is just a struct, too, but you rarely use it as such: There's an entire API that handles the nitty-gritty for you, in the same way that you can use this model to validate the data you are setting.

Code that requires this data, either to store it in a DB, or present it to the client side in a view, can then be written with a clear contract:

public function showRecipient(Recipient $recipient)
{
    //if an instance is passed, its email address is expected to be set and valid
}

But how do you best make sure when, and where to validate the email address:

The validation isn't specific to this place, I may want to check emails in other places

Don't. If you are processing data that belongs to, or is part of the dataset that makes up a Recipient, the thing to do is to wrap this data in the appropriate container: a Recipient instance.
If you validate the email elsewhere in your code, you are repeating yourself. Simply pour it into an object, that makes sure the data is valid, and pass that object around. Whatever values you extract from it (through getters), is guaranteed to be validated.

Should the validation happen in the constructor? Should it be a separate class? What would that look like?

The validation should Not be done in the constructor. I can construct an instance, and re-assign/reset the properties to a particular value, bypassing the constructor's validation.
A separate class, then? No. Really... why would you centralize all data validation/sanitation? Such a class would be either an all-static monster, or you'd initialize all validation code, every time you want to validate a single value.

I'd make my properties protected, and use setters. These setter methods are tailored to the type of property they are setting, and can perform the necessary sanitation and validation. Simple.

Should there be an interface for every class? Would that live at the top of this file?

Depends. Is it worth while? Are there similarities between other models (database mapping: getId() and setId methods)? Is there a certain type of functionality you want to add (for example implementing one of the Traversable interfaces).
Hard to say from what code you shared, but if this is the only data-class then: no. An interface is probably not required.

Should I throw exceptions when it's not valid? Should they be an object as well? How?

Yes: a class has just one job. This class' job is to contain valid data. Raw data is passed to its setters, which validate/sanitize this data. If the data is malformed/invalid, the Recipient class is not responsible for it: the code that supplied the data is. The Recipient should throw an exception (it encountered exceptionally formed data), and the calling code should handle the problem.
Should these exceptions be objects: Of course, they are. There's no way around them. If you mean: should I throw or set a property, the answer is: throw. Your class encountered a problem. Setting a property or returning false assumes the calling code is checking return values or flags. This is not a given (return values are often ignored, and error flags seldom checked).
Throwing an exception forces the calling code to respond

To answer the how question, here's how I'd write your class:

class Recipient
{
    protected $email = null;
    protected $name = null;
    public function __construct(array $data = array())
    {
        foreach ($data as $key => $val)
        {
            //transform key to method
            $method = 'set'.ucfirst(strtolower($key));
            if (method_exists($this, $method))
                $this->{$method}($val);
        }
    }
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL))
        {//not a valid email address, throw exception
            throw new InvalidArgumentException(
                sprintf(
                    '%s expects valid email address, %s is not',
                    __METHOD__,
                    $email
                )
            );
        }
        $this->email = $email;
        return $this;
    }
    public function setName($name)
    {
        //add checks/sanitation you need: check length, check format, check for numeric chars... 
        $this->name = trim($name);
        return $this;
    }
    public function getEmail()
    {
        return $this->email;
    }
    public function getName()
    {
        return $this->name;
    }
}

That should do it. Usage examples:

$data = array('email' => 'foo@bar.com', 'name' => 'foo');
$recipient = new Recipient($data);
echo 'From instance - email: ', $recipient->getEmail();
$recipient->setName('    Foo');
echo 'After setter: ', $recipient->getName();//leading spaces are removed

Now, why did I default the constructor argument to an empty array? Simply because it allows you to do this:

$blank = new Recipient();
$obj->EmailGetter($queryObject, $blank);//gets email, sets it on instance
$blank->setName('Unknonwn');

I can create an object, then fetch the data as I go along, and set the properties accordingly.
As far as the exception business is concerned:

$error = new Recipient(
    array(
        'email' => 'invalid'
    )
);//error
//or
$test = new Recipient();
$value = 'invalidaddress';
try
{
    $test->setEmail($value);
    echo 'If $value is valid, this will echo its value again here: ', $test->getEmail();
}
catch (InvalidArgumentException $e)
{//echos "invalidaddress was refused:  Recipient::setEmail expects valid email address invalidaddress is not"
    echo $value, ' was refused: ', $e->getMessage();
}

I've reviewed a couple of validation + data-model related questions, so I thought I'd add some of my (lengthy) reviews here, too. For completeness' sake:

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks - this is really helpful, thanks taking the time to explain this :) \$\endgroup\$ – Rich Bradshaw Aug 5 '14 at 10:26
  • 1
    \$\begingroup\$ @RichBradshaw: Was just updating my review, to be more specific, and added some more explanation + links you might find useful, too \$\endgroup\$ – Elias Van Ootegem Aug 5 '14 at 10:36
  • \$\begingroup\$ @RichBradshaw: Could you perhaps clarify what you meant by the second part of "Should there be an interface for every class? Would that live at the top of this file"? Did you mean to ask: "Should this interface be defined in the same file, just above the class definition" (to which the answer is a resounding NO), or did you mean something else? If not: separate files, with a logic name + spl_autoload_register autoloader is standard procedure \$\endgroup\$ – Elias Van Ootegem Aug 5 '14 at 13:11
  • \$\begingroup\$ That is what I meant and I agree - now I know that was a stupid question! \$\endgroup\$ – Rich Bradshaw Aug 5 '14 at 13:13
  • 1
    \$\begingroup\$ @RichBradshaw: No such thing as a stupid question (cliché, I know). Questions stem from ignorance and laziness, or from curiosity and being in need of assistance. I wouldn't have bothered reviewing code if your question lead me to believe you were lazy or ignorant... (here endeth the lesson) \$\endgroup\$ – Elias Van Ootegem Aug 5 '14 at 13:23
1
\$\begingroup\$

I think that having a class representing a recipient is good. It makes it much easier to understand what your code does that, for instance, having just a string representing the value.

I'd take a step further and create the class such that it only represents a valid recipient. If you do that, you'll always be sure that your object contains valid data and you won't have to check for its validity all the time.

You can do that quite easily with a builder method, that takes as input the email address and the name, validates them and either creates an instance of Recipient or throws an exception if the data is not valid. This has also the advantage of letting you detecting bad data as soon as the user enters it instead of when you need to actually use it.

\$\endgroup\$
  • 1
    \$\begingroup\$ with 'builder method' you mean __constructor right..? Factory pattern is bit to much here ;) \$\endgroup\$ – Pinoniq Aug 4 '14 at 21:58
  • \$\begingroup\$ Yes, a factory is definitely too much. Actually I was thinking at something that could be a bit overkill and I ended up writing a mixed answer. I was thinking at having an Email class, which does email validation. Recipient should compose it with an user name. The function I was thinking at should be in charge of taking a pair of strings, building the email object, validating the name and feeding them to the Recipient constructor \$\endgroup\$ – mariosangiorgio Aug 4 '14 at 22:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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