1
\$\begingroup\$

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 = "abuse@google.com";
        $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("k123@google.com", "k123@google.com"),
            'valid_2' => array("abuse@google.com", "abuse@google.com"),
            'valid_3' => array(null,null),
            'valid_4' => array("doesnotexist@google.com", "doesnotexist@google.com"),
        );
    }

    /**
     * @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@example.com"),
            "invalid_8" => array("email\@example.com"),
            "invalid_9" => array("email@example.com."),
            "invalid_10" => array("nospam@example.com"),
            'integer' => array(123, null),
            'array_0' => array(array(), null),
            'array_1' => array(array('email@example.com'), 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 = "abuse@google.com";
        $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("k123@google.com", "k123@google.com"),
            'valid_2' => array("abuse@google.com", "abuse@google.com"),
            'valid_3' => array(null,null),
            'valid_4' => array("doesnotexist@google.com", "doesnotexist@google.com"),
        );
    }

    /**
     * @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@example.com"),
            "invalid_8" => array("email\@example.com"),
            "invalid_9" => array("email@example.com."),
            "invalid_10" => array("nospam@example.com"),
            'integer' => array(123, null),
            'array_0' => array(array(), null),
            'array_1' => array(array('email@example.com'), 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?
\$\endgroup\$
  • \$\begingroup\$ I've removed your question about the unit tests, since you haven't included that code in the question itself. \$\endgroup\$ – 200_success Mar 27 '16 at 6:14
  • \$\begingroup\$ If you want your unit tests to be reviewed, then include the code in the question too. \$\endgroup\$ – 200_success Mar 27 '16 at 9:15
  • \$\begingroup\$ sorry forgot to paste the links!! \$\endgroup\$ – wwr Mar 27 '16 at 9:15
  • \$\begingroup\$ We don't review code in links, only code that is embedded directly in the question itself. \$\endgroup\$ – 200_success Mar 27 '16 at 9:16
  • \$\begingroup\$ oh now I see! I did that to keep the question shorter. I'm going to edit. \$\endgroup\$ – wwr Mar 27 '16 at 9:20
3
\$\begingroup\$

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 ...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I started almost everything a couple of weeks ago :-) Later I will spend some time on what you have wrote and brb \$\endgroup\$ – wwr Mar 27 '16 at 9:12
  • \$\begingroup\$ about testing \EmailValidator\Validator . It's a package from packagist with its own unit tests, that's why I didn't write tests on it. But in my unit tests I have tested several cases of validation: are they complete garbage? \$\endgroup\$ – wwr Mar 27 '16 at 9:28
1
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ both the classes are just a proof of concept I'm not really trying to do anything. I'm studying and writing code upon what I learn. I admit, though, that Gtoken is supposed to be a class handling Google Tokens (request new / revoke / refresh) \$\endgroup\$ – wwr Mar 27 '16 at 9:31
  • \$\begingroup\$ @wwr In that case, GoogleToken would be a better name :) If you want to keep it generic for now, Token might be acceptable as well. The main thing is to not keep a reader wondering what g might stand for. \$\endgroup\$ – tim Mar 27 '16 at 9:35
  • \$\begingroup\$ why do you say "indentation is off"? I even ran phpcbf (PSR2) before posting. Could you please point out what's wrong with it? I really don't see it. \$\endgroup\$ – wwr Mar 27 '16 at 20:35
  • \$\begingroup\$ @wwr sorry, it's just that part of GtokenB:setEmail is indented too much (it's a very minor thing and doesn't really hurt readability that much, but I thought I'd mention it anyways). \$\endgroup\$ – tim Mar 27 '16 at 20:38

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.