Two classes to validate and store an e-mail address

Question

I'm trying to improve my limited knowledge in PHP (7) and I wrote these two simple classes as a proof of concept.

They both have just one (private) property ->email and they run validation before setting a value for it (unless it's null).

They are both supposed to run at business-tier

GtokenA uses magic methods and is more strict than GtokenB. It does not allow to set any property that is not declared.

<?php

class GtokenA
{
    /**
     * @var string  $email
     */
    protected $email;

    public function __construct($email = null)
    {
        $this->__set('email', $email);
    }

    public function __get($property)
    {
        if(property_exists($this,$property)){
            return $this->$property;
        }
    }

    public function __set($property, $value)
    {

        if (!property_exists($this, $property)) {
            throw new InvalidArgumentException("The class " . get_class($this) . " does not have the property " . $property);
        }

        switch ($property) {
            case 'email':
                // a null value is acceptable
                // otherwise I need the email to be a valid recipient
                $validator = new \EmailValidator\Validator();
                try {
                    if(!is_null($value) && !$validator->isSendable($value)) {
                        throw new UnexpectedValueException("Wrong value for ->$property");
                    }
                } catch (Exception $e) {
                    throw new UnexpectedValueException("Wrong value for ->$property : " . $e->getMessage());
                }

                break;
        }

        $this->$property = $value;

        return $this;
    }
}

GtokenB uses setters and getters:

<?php

class GtokenB
{
    /**
     * @var string  $email
     */
    private $email;

    public function __construct($email = null)
    {
        $this->setEmail($email);
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function setEmail($email = null)
    {
            // a null email is acceptable
            // otherwise I need the email to be a valid recipient
            $validator = new \EmailValidator\Validator();
            try {
                if(!is_null($email) && !$validator->isSendable($email)) {
                    throw new UnexpectedValueException("Wrong email for ->email");
                }
            } catch (Exception $e) {
                throw new UnexpectedValueException("Wrong email for ->email : " . $e->getMessage());
            }

        $this->email = $email;

        return $this;
    }
}

I also wrote some (hopefully meaningful) tests for my first time ever.

GtokenA tests:

<?php
require_once 'app/src/models/gtokenB.php';
/**
 * GtokenA test case.
 */
class GtokenBTest extends PHPUnit_Framework_TestCase
{
    /**
     * @covers __construct()
     */
    public function test__construct_withArguments()
    {
        $email = "[email protected]";
        $gtoken = new GtokenB($email);
        $this->assertEquals($email, $gtoken->getEmail());
    }

    /**
    * @covers __construct()
    **/
    public function test_construct_noArguments()
    {
        $gtoken = new GtokenB();
        $gtoken->setEmail(null);
        $this->assertEquals(null, $gtoken->getEmail());
    }

    public function validEmailStringProvider(){
        return array(
            'valid_1' => array("[email protected]", "[email protected]"),
            'valid_2' => array("[email protected]", "[email protected]"),
            'valid_3' => array(null,null),
            'valid_4' => array("[email protected]", "[email protected]"),
        );
    }

    /**
     * @covers Gtoken::__set()
     * @dataProvider validEmailStringProvider
     */
    public function test_setter_Email($email, $expected)
    {
        $gtoken = new GtokenB();
        $gtoken->setEmail($email);
        $this->assertEquals($expected, $gtoken->getEmail());
    }

    public function invalidEmailString()
    {
        return array(
            "invalid_1" => array("email-examplecom"),
            "invalid_2" => array("email-example.com"),
            "invalid_3" => array("email\\\"@example.com"),
            "invalid_4" => array("email@example!.com"),
            "invalid_6" => array("email@example$.com"),
            "invalid_7" => array("[email protected]"),
            "invalid_8" => array("email\@example.com"),
            "invalid_9" => array("[email protected]."),
            "invalid_10" => array("[email protected]"),
            'integer' => array(123, null),
            'array_0' => array(array(), null),
            'array_1' => array(array('[email protected]'), null),
            );
    }

    /**
    * @covers Gtoken::__set()
    * @expectedException UnexpectedValueException
    * @dataProvider invalidEmailString
    **/
    public function test_setter_Email_throwsUnexpectedValueException($email)
    {
        $gtoken = new GtokenB();
        $gtoken->setEmail($email);
    }
}

GtokenB tests:

<?php
require_once 'app/src/models/gtokenA.php';
/**
 * GtokenA test case.
 */
class GtokenATest extends PHPUnit_Framework_TestCase
{
    /**
     * @covers __construct()
     */
    public function test__construct_withArguments()
    {
        $email = "[email protected]";
        $gtoken = new GtokenA($email);
        $this->assertEquals($email, $gtoken->email);
    }

    /**
    * @covers __construct()
    **/
    public function test_construct_noArguments()
    {
        $gtoken = new GtokenA();
        $gtoken->email = null;
        $this->assertEquals(null, $gtoken->email);
    }

    /**
    * @covers __set()
    * @expectedException InvalidArgumentException
    **/
    public function test_set_notExistentProperty()
    {
        $gtoken = new GtokenA();
        $gtoken->kk = null;
    }

    public function validEmailStringProvider(){
        return array(
            'valid_1' => array("[email protected]", "[email protected]"),
            'valid_2' => array("[email protected]", "[email protected]"),
            'valid_3' => array(null,null),
            'valid_4' => array("[email protected]", "[email protected]"),
        );
    }

    /**
     * @covers Gtoken::__set()
     * @dataProvider validEmailStringProvider
     */
    public function test_setter_Email($email, $expected)
    {
        $gtoken = new GtokenA();
        $gtoken->email = $email;
        $this->assertEquals($expected, $gtoken->email);
    }

    public function invalidEmailString()
    {
        return array(
            "invalid_1" => array("email-examplecom"),
            "invalid_2" => array("email-example.com"),
            "invalid_3" => array("email\\\"@example.com"),
            "invalid_4" => array("email@example!.com"),
            "invalid_6" => array("email@example$.com"),
            "invalid_7" => array("[email protected]"),
            "invalid_8" => array("email\@example.com"),
            "invalid_9" => array("[email protected]."),
            "invalid_10" => array("[email protected]"),
            'integer' => array(123, null),
            'array_0' => array(array(), null),
            'array_1' => array(array('[email protected]'), null),
            );
    }

    /**
    * @covers Gtoken::__set()
    * @expectedException UnexpectedValueException
    * @dataProvider invalidEmailString
    **/
    public function test_setter_Email_throwsUnexpectedValueException($email)
    {
        $gtoken = new GtokenA();
        $gtoken->email = $email;
    }
}

---

I prefer GtokenA by far. It feels more elegant in the usage ( $obj->email = "value" instead of $obj->setEmail("value") ) but, most important, it's strict and I could write and run this test:

/**
* @covers __set()
* @expectedException InvalidArgumentException
**/
public function test_set_notExistentProperty()
{
    $gtoken = new GtokenA();
    $gtoken->kk = null;
}

But maybe this is just a personal preference that brings no value to the code.

Questions:

• How do you rate my code?
• Would you suggest any improvement?
• Do I use Exceptions in a meaningful way?

Answer 1

The first example (you called it "GtokenA") is a complete garbage. In a real world situation, your class would have more than 1 parameter, which means that your magic method would grow in complexity and, when you return to that code in 3 month for maintenance, you will have forgotten all about how that magic worked.

As for second approach .. well ...

• From the constructor it is clear, that "email" is an optional parameter, but, if I don't pass it a valid email, then it blows up in my face. That doesn't really makes sense.
• Why are you hard-coding the validator? Instead of constructor accepting an email, it should instead be expecting a validator to be passed in. What if you have *gasp* other classes, that also need a validator?
• The constructors should never contain any computational logic. It become almost completely untestable.
• What about testing the validator? How do you intent to isolate it from "Gtoken" classes?

So .. my rating would be: "you started to learn about unit tests like 2 days ago, right?"

About improving this mess:

There are actually two approaches for validating an entity:

• you can validate it on each setter - which is kinda nice, when you can always be sure, that the entity at all time is in a valid state
• have a separate isValid() method - which comes in handy, when you have to validate more complex conditions .. like country-city combinations, where you need one to validate the other and vice versa

In either case, when you have a codebase, which would expect a "full valid instance" to be produced, it's preferable to have a separate Builder class, that becomes responsible for assembling the dependencies, initializing the cl;as, passing the values and making sure that all is valid, before returning a prepared instance to the rest of your codebase ...

Answer 2

It feels more elegant in the usage ( $obj->email = "value" instead of $obj->setEmail("value") )

It's mostly a matter of preference, but I would strongly disagree with this. The normal setEmail approach makes it very clear that this is a method that is applied to a specific instance.

This is logically different than your preferred approach, which looks as if it were only setting a value (which it actually isn't! It's also validating).

Additionally, your prefered approach looks as if it would break encapsulation (even though it doesn't).

Other than that, I would mostly agree with @tereško. Additionally:

• what's a gtoken? Write the g part out to make it clearer.
• What Exception are you catching? It's very unclear when this might happen or why it might happen. At the very least you should catch the specific exceptions, not the general exception. I could also imagine that exceptions are thrown which do not mean that an email is invalid (such as wrong server email config, timeouts, and who knows what else).
• your indentation is off, making your code harder to read.
• your comment is not really needed. I can tell from the code that a null email is ok. What a comment should explain is why this is the case (it's rather unexpected). As it is, the comment would be better as a PHPDoc method comment instead of an in-code comment.
• you could make it more explicit that null is an allowed email by extracting that check to the beginning, eg: if (is_null($email)) { return $this; // valid email because X }. Now your if is also simplified.