9

Let's say I have this model. (I made it very simple for demonstration purposes.)

class User
{
    public $id;
    public $email;
    public $password;
    public $errors = [];

    public function isValid()
    {
        if (strpos($this->email, '@') === false) {
            $this->errors['email'] = 'Please enter an email address';
        }
        // ...

        return !$this->errors;
    }
}

And let's say I have this DAO for retrieving, adding, updating, and deleting users.

class UserDAO
{
    public function getUsers() { ... }

    public function getUserById($id) { ... }

    public function addUser(User $user) { ... }

    public function updateUser(User $user) { ... }

    public function deleteUser($id) { ... }

    public function isEmailUnique($email) { ... }
}

When I process a form, I typically do something like this:

$userDAO = new UserDAO();
$user = new User();
$user->email = filter_input(INPUT_POST, 'email', FILTER_VALIDATE_EMAIL);
$user->password = filter_input(INPUT_POST, 'password');

if ($user->isValid()) {
    if ($userDAO->addUser($user)) {
        // ...
    } else {
        // ...
    }
} else {
    // do something with $user->errors
}

Now, let's say part of my user validation should be to check whether email is unique, how do I make it part of the User model? So that, when $user->isValid() is called, it also checks whether the email is unique? Or am I doing this all wrong?

From my weak understanding of DAOs, DAOs are responsible for all interactions with the database. So how do I make the model work with database from within?

Mikey
  • 6,728
  • 4
  • 22
  • 45
  • 1
    The answer by S.Lott probably help you http://stackoverflow.com/a/198032/3904215. And here a description from tutorialspoint: http://www.tutorialspoint.com/design_pattern/data_access_object_pattern.htm – Alexander Feb 13 '16 at 23:30

7 Answers7

3

My recommendation is this: don't consider email address uniqueness when validating your User model. Uniqueness is a UserDAO problem, not a User problem.

If the User can validate itself, it should be able to do so in isolation; its validation should not be concerned with any external interactions.

The only time it matters whether or not the email address is unique is at the instant you attempt to insert it into the database. Considering the possibility of multiple concurrent users, it is theoretically possible to validate the uniqueness of the address and have it no longer be unique by the time you attempt to insert it.

I think the most straightforward and dependable way to do this is to add a unique constraint on email address in your database, then in your addUser() method, just try to add it. If your database tells you it is not unique, then you know it is not unique. You cannot really know beforehand.

Don't Panic
  • 41,125
  • 10
  • 61
  • 80
1

I think that validation, in this case, is a part of application logic as you require data that isn't stored in the model. So it will be better to implement validation logic within a different controller function.

Moreover, there is a similar question with the similar answer already: Best Place for Validation in Model/View/Controller Model?

Community
  • 1
  • 1
max
  • 2,757
  • 22
  • 19
1

Keep the User class as it is, it is a good citizen by itself.

I would make the method isEmailUnique private (iff it is used just for that) and check by the existence of a User with that e-mail inside addUser. On the other hand, this would pull the responsibility of the logic to the DAO. (See: Responsibilities and use of Service and DAO Layers)

So if you change the behavior of isValid to check if the user is already onto the database, you would be breaking your design.

Community
  • 1
  • 1
thyago stall
  • 1,654
  • 3
  • 16
  • 30
1

One way to go about this, would be to remove the method User::isValid completely, in favour of passing everything it needs in its constructor, running the validation from there:

class User
{
    public function __construct($email) {
        if (strpos($email, '@') === false) {
            throw new \InvalidArgumentException("Invalid email");
        }

        $this->email = $email;
    }
}

If you think about it, what makes a user valid? If that's a valid email address, make sure you pass one in when you construct the User object. That makes your User object always valid.

A better way for ensuring this, would be using a ValueObject which encapsulates this validation logic so you can use it in other objects, avoiding a lot of redundancy and boilerplate code:

class Email
{
    public function __construct($email)
    {
        if (strpos($email, '@') === false) {
            throw new \InvalidArgumentException("Invalid email");
        }

        $this->email = $email;
    }
}

class User
{
    public function __construct(Email $email)
    {
        $this->email = $email;
    }
}

class ProspectiveUser
{
    public function __construct(Email $email)
    {
        $this->email = $email;
    }   
}

Now, in terms of validating a user with the database, you can perfectly encapsulate that within your DAO. The DAO can perform the check of ensuring that the user is not in the database already, keeping the DAO consumer agnostic of it, except from the fact that it should know how to handle the case of an error when the user already exists in the DB:

class UserDAO
{
    public function recordNewUser(User $user)
    {
        if ($this->userExists()) {
            throw new UserAlreadyExistsException();
        }

        $this->persist($user);
        $this->flush($user);
    }

    private function userExists(User $user)
    {
        $user = $this->findBy(['email' => $user->getEmail()]);

        return !is_null($user);
    }
}

As you can see, the DAO provides you with an interface for saving a new user, but that operation might fail if the constraint of email uniqueness is not satisfied.

hasumedic
  • 2,139
  • 12
  • 17
1

I would take all the validation issues out of User class and move to Controller layer (which can e.g. call UserDAO to check email uniqueness). It is best to keep User class simply as an Entity class and put all the other stuff in other classes - otherwise it will grow and grow to the state that is not maintainable anymore :)

Check also: https://en.wikipedia.org/wiki/Single_responsibility_principle

Piotr Borek
  • 866
  • 1
  • 6
  • 6
0

The UserDAO class must implement a method called userExists. This method only checks if the email address already exists. It checks this in the BD so its place is in the UserDAO class. It must be a private method and addUser uses it to return a correct value or false/null

mark
  • 344
  • 3
  • 8
0

I think you can use DAO as argument for validation function.

public function isValid($dao)
{
    if (strpos($this->email, '@') === false) {
        $this->errors['email'] = 'Please enter an email address';
    }
    if ($dao->isEmailUnique($this->email) === false) {
        $this->errors['email'] = 'Email address should be unique';
    }
    // ...

    return !$this->errors;
}

But may be better way is use DAO inside your User model. Add to model private variable $dao and init it in constructor. And implement all methods for add/edit/delete operation in model class.

newman
  • 2,689
  • 15
  • 23