UserDAO with CRUD functionality for my UserRepository

This is my first attempt at creating a DAO.

I would like to get some feedback regarding the following aspects if possible:

• Code readability

• Efficiency

• Usability

I also would appreciate any other suggestions for improvement as well.

UserDAO

```
class UserDAO
{
private $pdo;

public function __construct(PDO $pdo)
{
$this->pdo = $pdo;
}

public function fetchById($id)
{
$sql = 'SELECT id,';
$sql .= ' firstname,';
$sql .= ' lastname,';
$sql .= ' email,';
$sql .= ' password';
$sql .= ' FROM users';
$sql .= ' WHERE id = :id';
$sql .= ' LIMIT 1';

$sth = $this->pdo->prepare($sql);
$sth->bindValue(':id', $id, PDO::PARAM_INT);
$sth->execute();

return $sth->fetch() ?: null;
}

public function fetchByEmail($email)
{
$sql = 'SELECT id,';
$sql .= ' firstname,';
$sql .= ' lastname,';
$sql .= ' email,';
$sql .= ' password';
$sql .= ' FROM users';
$sql .= ' WHERE email = :email';
$sql .= ' LIMIT 1';

$sth = $this->pdo->prepare($sql);
$sth->bindValue(':email', $email, PDO::PARAM_STR);
$sth->execute();

return $sth->fetch() ?: null;
}

public function insert($firstname, $lastname, $email, $password)
{
$sql = 'INSERT INTO users (firstname, lastname, email, password) ';
$sql .= 'VALUES(:firstname, :lastname, :email, :password)';

$sth = $this->pdo->prepare($sql);
$sth->bindValue(':firstname', $firstname, PDO::PARAM_STR);
$sth->bindValue(':lastname', $lastname, PDO::PARAM_STR);
$sth->bindValue(':email', $email, PDO::PARAM_STR);
$sth->bindValue(':password', $password, PDO::PARAM_STR);

return $sth->execute();
}

public function update($id, $firstname, $lastname, $email, $password)
{

Well it looks incredibly clean, the amount of effort to read it is almost minimal. Everything seems very straight forward too! Nice.

However, that leaves little to actually be critiqued!
UserDAO

-
It's common to see private variables prefixed with an underscore. It's up to you to decide if you'd like to follow this style. Some (including me) find that it makes it easier to differentiate between variable types.

-
I've never seen SQL queries built/concatenated like you have done it. I wonder where you picked up this technique? I see nothing wrong with simply having it in one string. Another option you have is...

-
SQL procedures, which provide a way to create the query before you use it, and then call it. I suggest you look into them. Benefits include improved performance and cleaner PHP code.

-
On the subject of SQL, perhaps you'd be interested in utilizing PDO Transactions.

-
At first glance, $sth->fetch() ?: null tricked me. Sure it works, but it seems somewhat hack-ish.

-
In update(), your return statement, $sth->rowCount(); will return either 0 or 1 (hopefully). This seems like a strange response. You could return the execute call if you're looking to make sure the query happened.

-
Same deal with the delete() method. Are you expecting more than one user to be deleted or something?

Database

Does this class do something extrodinary that PDO doesn't do? I just can't find the point behind this, as it's basically a replica of PDO with less functionality.

Anyways...

-
$dsn is a clunky string to have in the constructor. I don't suppose you could have the parameter an associative array, and then build the DSN on the fly? That would leave for a cleaner calling of the class, and an easier way to add and remove parts from the DSN string.

-
Purely choice, but hard coding in PDO::FETCH_OBJ could cause difficulties if you wanted to port this class to another project where you might need the array or another format in response. Maybe add get/set methods for this.

Overall, very nice code.

StackExchange Code Review Q#55439, answer score: 2