The User class is doing too much. It represents a "User" and also handles the data connection and mapping. You have a need for two classes in the very least. The Repository Pattern is a common design solution for this and is quite easy to set up.
You first need to split up your concerns. You have a "User" and a "place users are stored."
The User class would be called the Domain Model. This represents a row from the "users" table and contains the data and methods that operate on that data. It does not contain data access methods. Rather, it should only contain "business logic."
From Wikipedia:
A domain model is a system of abstractions that describes select aspects of a sphere of knowledge, influence, or activity (a domain [3]). The model can then be used to solve problems related to that domain. The domain model is a representation of meaningful real-world concepts pertinent to the domain that need to be modeled in software. The concepts include the data involved in the business and rules the business uses in relation to that data.
A domain model generally uses the vocabulary of the domain so that a representation of the model can be used to communicate with non-technical stakeholders.
(emphasis, mine)
class User
{
private $id;
private $username;
private $password;
private $first_name;
private $last_name;
public function __construct($username, $password, $id = 0) {
$this->setUsername($username);
$this->setPassword($password);
$this->setId($id);
}
// Getters
// Setters
public function getFullName() {
if (isset($this->first_name) && isset($this->last_name)) {
return $this->getFirstName() . ' ' . $this->getLastName();
} else {
return null;
}
}
public function canChangePassword($oldPassword, $newPassword) {
// Logic to check if the password can be changed
}
public function changePassword($oldPassword, $newPassword) {
if (!$this->canChangePassword($oldPassword, $newPassword)) {
throw new Exception("Cannot change password");
}
$this->password = $newPassword;
}
}
Not every property needs a setter method. In fact, a User is not even a valid object unless it has a username, and password. The constructor is the perfect place to supply those values because the username shouldn't change, and the password can change, but the change in value should be tightly controlled by the User object (hence the canChangePassword and changePassword methods).
The data access code would go into a UserRepository class that specializes in CRUD operations on the "users" table. You can optionally create an interface for the repository class in case you want to unit test your application and provide a mock user repository.
interface IUserRepository
{
public function add(User $user);
public function findAll();
public function find($id);
public function findByUsername($username);
public function update(User $user);
public function remove(User $user);
}
The interface sets up the main operations you can perform on Users in the database. The UserRepository class below is where we actually start the data access code:
class UserRepository implements IUserRepository
{
private $connection;
public function __construct(PDO $connection) {
$this->connection = $connection;
}
public function add(User $user) {
// Create prepared statement for INSERT
}
public function findAll() {
$results = // Query by prepared statement
$users = [];
foreach ($results as $data) {
$users[] = $this->createInstance($data);
}
return $users;
}
public function find($id) {
$data = // Query by prepared statement
return $this->createInstance($data);
}
public function findByUsername($username) {
$data = // Query by prepared statement
return $this->createInstance($data);
}
public function update(User $user) {
// Create prepared statement for UPDATE
}
public function remove(User $user) {
// Create prepared statement for DELETE
}
private function createInstance($data) {
return $this->map($data, new User($data['username'], $data['password'], $data['id']));
}
private function map($data, User $user) {
foreach ($record as $attribute => $value) {
$method = 'set' . ucfirst($attribute);
if (method_exists($user, $method)) {
call_user_func_array(array($user, $method), array($value));
} else {
throw new Exception("No setter method exists for '$attribute'");
}
}
return $user;
}
}
The instantiating and mapping of User objects is kept internal to the repository class. You could split this concern out into its own "mapper" or "factory" class if you wanted, but I generally start out with the mapping code right inside the repository class. Later on if I need to split out the mapping code, doing so is a simple refactoring job because these methods are private.
Now let's explore a sample use case where we change the password of a user:
// Create the "repostory" object
$users = new UserRepository($pdo);
// Fetch the user from the database
$user = $users->findByUsername('jdoe');
// Modify the User
$user->changePassword('password1', 'password2');
// Persist the changes back to the database
$users->update($user);
Clean and simple. That's the goal of splitting out your data access from your domain model. Furthermore, since your User class is devoid of data access code, you can now write unit tests validating the business logic of its methods.
Unit Testing
Earlier I mentioned that defining an interface for the repository can assist unit testing. We'll take a simple "MVC" example to illustrate this point. We have a "users controller" class that handles an HTTP request. It has two arguments in its constructor. The first argument has a Type Hint of IUserRepository and the second argument is the $_GET or $_POST array.
class UsersController
{
private $params;
private $users;
public function __construct(IUserRepository $users, $params) {
$this->users = $users;
$this->params = $params;
}
public function changePassword() {
$user = $this->users->findByUsername($this->params['username']);
$user->changePassword($this->params['password1'], $this->params['password2']);
$this->users->update($user);
}
}
The changePassword method queries the repository for the user and calls the changePassword method on the User object. Lastly, it updates the user. Since the $this->users property was injected into the UsersController object in the constructor with a Type Hint, we can provide our own mock object to unit test the controller with.
class MockUserRepository implements IUserRepository
{
private $data;
public function __construct($existingUsers = array()) {
$this->data = array(
'all' => $existingUsers,
'added' => array(),
'updated' => array(),
'removed' => array()
);
}
public function add(User $user) {
$this->data['added'][] = $user;
$this->data['all'][] = $user;
}
public function findAll() {
return $this->data['all'];
}
public function find($id) {
// Find in $this->data['all']
}
public function findByUsername($username) {
// Find in $this->data['all']
}
public function update(User $user) {
$this->data['updated'][] = $user;
}
public function remove(User $user) {
// remove from $this->data['all']
$this->data['removed'][] = $user;
}
public function getData() {
return $this->data;
}
}
Now, the simple unit test:
$userToFind = new User('jdoe', 'password1', 123);
$mockUsers = new MockUserRepository(array($userToFind));
$params = array(
'username' => 'jdoe',
'password1' => 'password1',
'password2' => 'abc123'
);
$controller = new UsersController($mockUsers, $params);
$controller->changePassword();
// Assert that $userToFind has a different password
// Assert that $userToFind was "updated"
Now we can unit test something more complex like a "controller" in an MVC application without requiring a connection to the database.
@PeraMika said:
I hope that it is OK to just have class User and class UserRepository without interface and or other classes that you have created here.
The interface is not a requirement. The most important thing is to separate the Domain Model from the Data Access.
@PeraMika said:
Is it good to have two classes for every table in DB, for example - if we have a table "Comments" - class Comment and class CommentRepository? Or, if we have table categories - class Category and class CategoryRepository?
The answer is two parts: "Yes" and "It depends".
If you have two tables called "comments" and "categories" you should have two Domain Models: Comment and Category. Yes, you should create one Domain Model class per table in the database.
When it comes to creating the repository classes, it depends. It's not a requirement to create one repository class per database table. If you don't have a large number of tables, one class might suffice. As your application grows, the number of tables grow. As a result, your data access code grows in size and stuffing all of it into one file can become cumbersome to maintain.
Really the decision of how many repository classes you need comes down to good object oriented design. I always approach class design with these in mind first:
- Do one thing and do it well
- Don't Repeat Yourself (DRY)
Do you need one repository class per table? No. If you start out doing this then you are creating a system that is easily extensible, yet doesn't make any assumptions about future business requirements. This is why I advocate for one repository class per table.
@PeraMika asked:
In methods with SQL (PDO), for example findAll()... is it good to check if the query is executed successfully, and if not - return false; For example:
if (!($stmt->execute())) {
return false;
}
Also, in findById($id) is it good to have something like:
$record = $stmt->fetch(PDO::FETCH_ASSOC); $stmt-> closeCursor();
if (!$record) {
return false;
}
Many Object Relation/Mapping (ORM) libraries in multiple languages and platforms seem to implement these rules:
If multiple records can be returned, but the query does not return results, an empty array or "collection" is returned. This helps prevents null pointer exceptions in later code
If a single record should be returned, for instance querying by primary key, and the query does not return results, return null instead and let the calling code decide if this is an "exceptional" condition (and then throw an exception).
If a single record should be returned and multiple records are found, throw an exception. You are expecting something to be unique and it is not. Your repository cannot resolve this situation, so failing early and failing loudly is the best option.
If the execute method returns false because of a failure in the database or problem with the SQL, this is an "exceptional" condition. It is appropriate to throw an exception in this scenario with the message being the error message sent back from the database.
