4
\$\begingroup\$

I am finally making the move to object oriented PHP. This has been something I have been quite reluctant to do, but I now feel that it could benefit me, and bring me up-to-date with current practices.

The main problem I have is how to start, and how to do things right. Now, I understand that with programming, there are many ways to do things, but I like to do things the correct, and efficient way.

The project

One of the reasons I wish to make the move to OOP is that myself and my colleagues at work wish to try coding this way to update an existing, and antiquated system that manages employee details. We have said that if we don't start using OOP, we never will!

It will eventually become a nice MVC and AJAX rich web-based app for our intranet (The MVC is a whole different story for another post!).

The code

The following code is my first attempt at writing a class to retrieve employee data from a MySQL database using the PDO class. I have tried to adhere to PSR-2 coding style where possible, but if you see anything out of place, then please say.

Employee.php

<?php

class Employee
{

    private $db;
    private $employee;

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

    public function fetchBy($value, $type = 'username')
    {
        $stmt = null;
        switch ($type) {
            case 'username':
                $stmt = $this->db->prepare("SELECT * FROM `employees` WHERE `Username` = :value");
                $stmt->bindValue(':username', $value, PDO::PARAM_STR);
                break;
            case 'id':
                $stmt = $this->db->prepare("SELECT * FROM `employees` WHERE `ID` = :id");
                $stmt->bindValue(':id', $value, PDO::PARAM_INT);
                break;
            case 'department':
                $stmt = $this->db->prepare("SELECT * FROM `employees` WHERE `Department` = :department");
                $stmt->bindValue(':department', $value, PDO::PARAM_INT);
        }
        $stmt->execute();
        if ($stmt->rowCount() > 1) {
            $this->employee = $stmt->fetchAll(PDO::FETCH_OBJ);
        } else {
            $this->employee = $stmt->fetch(PDO::FETCH_OBJ);
        }
    }

    public function fetchEmployee()
    {
        return $this->employee;
    }

}

index.php

<?php

spl_autoload_register(function ($class) {
    include 'class/'.$class.'.php';
});

try {
    $db = new PDO('mysql:host=localhost;dbname=employees','user','password');
} catch (PDOException $e) {
    // For now, print the message during dev, but later, log this!
    print $e->getMessage();
    die();
}

$employee = new Employee($db);
$employee->fetchBy("1", "department");

$fetchEmployee = $employee->fetchEmployee();
if (is_array($fetchEmployee)) {
    foreach ($fetchEmployee as $feItem) {
        echo $feItem->FirstName." ".$feItem->Surname."<br />";
    }
} else {
    echo $fetchEmployee->FirstName." ".$fetchEmployee->Surname;
}

$db = null;

Questions

  1. What do you think of my code, honestly? Does it look like I am on the right tracks?
  2. In regards to the PDO class, I understand that with it being a class itself. Should I just try and use it as-is without trying to wrap it, or should I actually wrap it? Would that make it easier to perform some tasks?
  3. As you will see, I am using a SELECT * FROM for querying the table. The idea behind this is to load up all the fields into an object that I can pick data out to display. Usually I prefer specifying each column, but is there any point to that? An alternative was to specify a LOT of getter methods for each field. Would that be a good idea, or overkill?
  4. Is the fetchBy method a good idea or, am I on the path to ending up with a query builder? Would that be a bad thing?
  5. I've read about dependency injection, so I am keeping the database object separate and loading this into the Employee object. Is the way I am doing this using the __construct method the right way or should I do this with another method like this?

    Employee.php

    public function setDb(PDO $db)
    {
        $this->db = $db;
    }
    

    index.php

    $employee = new Employee;
    $employee->setDB($db);
    

Feel free to pick my code apart and be constructive with your criticism. I can take it. I would rather pick up good habits now, than go full steam ahead doing things badly, and end up having to fix them later!

\$\endgroup\$
1
\$\begingroup\$

In regards to the PDO class, I understand that with it being a class itself, I should just try and use it as-is without trying to wrap it, or should I actually wrap it? Would that make it easier to perform some tasks?

If you have a real need to wrap it, feel free to do so. But make sure that you aren't just wrapping it for the sake of wrapping it. A lot of wrappers I see do not offer any new functionality, and just slightly change the method names and minor details of how PDO works, making it actually harder to use.

As you will see, I am using a SELECT * FROM for querying the table. [...] Usually I prefer specifying each column, but is there any point to that?

Yes, I think specifying it is better. It is more readable, and if you change your DB layout later, you don't load columns you might not even need.

Is the fetchBy method a good idea or, am I on the path to ending up with a query builder? Would that be a bad thing?

No, it's not on the path to a query loader, and generally, it's a good approach. But I would get rid of the $type variable. It's confusing and hard to use without reading the documentation. Just create the specific functions instead: fetchById, fetchByUsername, fetchByDepartment.

And as jdog said, you really don't need the fetchEmployee function, it's unnecessary and confusing.

What I would consider is creating a real Employee object, instead of the pseudo object PDO creates. You may not necessarily need it, but I think it's nicer to handle.

Is the way I am doing this using the __construct method the right way or should I do this with another method

Your alternative method of having a setDb class is definitely not the correct approach. Your Employee class doesn't work without a db, so if a caller doesn't call it, it will cause problems.

Either pass the db object in the constructor, or pass it on to the various fetch methods.

\$\endgroup\$
1
\$\begingroup\$

Here are a couple of comments, by no means trying to be complete:

I think you made a good start with your Employee class. If this is going to be grown beyond this one table, then you may want to inherit from a Table class, which at least has the constructor and probably the $db in it

Recommend you combine fetchBy and fetchEmployee into 1 function for the following reasons:

  • $employee->fetchEmployee unnecessary duplication of what you are fetching
  • unnecessary employee attribute can be eliminated

Why is $type defaulted to username. It seems not obvious to me. Some frameworks use id as a default, I think that is a better option

The switch statement can be simplified, see how you repeat the code for each parameter type? You only need one block of code for PDO::PARAM_STR, PARAM_INT etc

Recommend you always use fetchAll() and create another indirection through a new function called fetchOneBy().

Using * in queries has no direct downside at this level of complexity, however once you start using joins, it will get you into trouble. However you'll quickly find that listing the columns will be tedious. The natural tendency is to then build a data dictionary, for example by using private variables (see Joomla) or having a getFields() function in each class. Don't go to far down this rabbit hole, as this problem has been solved many times over: https://packagist.org/packages/doctrine/orm

\$\endgroup\$

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.