3
\$\begingroup\$

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)
    {
        $sql  = 'UPDATE users';
        $sql .= '   SET firstname = :firstname,';
        $sql .= '       lastname  = :lastname,';
        $sql .= '       email     = :email,';
        $sql .= '       password  = :password';
        $sql .= ' WHERE id = :id';

        $sth = $this->pdo->prepare($sql);
        $sth->bindValue(':id',        $id,        PDO::PARAM_INT);
        $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);
        $sth->execute();

        return $sth->rowCount();
    }

    public function delete($id)
    {
        $sql  = 'DELETE FROM users ';
        $sql .= '      WHERE id = :id';

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

        return $sth->rowCount();
    }
}

Database

class Database extends PDO
{
    private $options = [];

    private $dsn;
    private $username;
    private $password;
    private $connected;

    public function __construct($dsn, $username, $password, Array $options = [])
    {
        $this->dsn      = $dsn;
        $this->username = $username;
        $this->password = $password;
        $this->options  = $options;
    }

    public function prepare($sql, Array $driverOptions = [])
    {
        $this->connect();

        return parent::prepare($sql, $driverOptions);        
    }

    public function execute(Array $inputParameters = [])
    {
        $this->connect();

        return parent::execute($inputParameters);
    }

    private function connect()
    {
        if (!$this->connected) {
            parent::__construct($this->dsn, $this->username, $this->password, $this->options);

            $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $this->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            $this->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);

            $this->connected = true;
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I've concatenated my SQL queries like that pure for better readability, taught myself. Something wrong? Yes, I probably will be using transactions somewhere when I need more functionality. sth->fetch() ?: null, isn't it actually logical to return void if nothing was able to be fetched? For update() I guess I could return the affected rows and just return false if none? Because if the query didn't execute there probably would be some exception thrown with it as well. For delete(), ditto. My db class extends PDO, mainly for incorporating lazy connections and will get bigger as I require. \$\endgroup\$ – Kid Diamond Jun 28 '14 at 5:37
  • \$\begingroup\$ Transactions are sweet if you have to perform multiple actions against the DB because it allows for rollbacks, which are pretty nifty. All else is almost pure semantics \$\endgroup\$ – Alex L Jun 28 '14 at 5:42
  • \$\begingroup\$ I'm sorry what do you mean by semantics? Contextual meaning? (I'm not native English). \$\endgroup\$ – Kid Diamond Jun 28 '14 at 5:51
  • 1
    \$\begingroup\$ Many of the other points are simply just alternatives :) \$\endgroup\$ – Alex L Jun 28 '14 at 5:58
  • \$\begingroup\$ Alrighty. Thank you for your nice review as always. \$\endgroup\$ – Kid Diamond Jun 28 '14 at 6:05

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.