Event timers database interaction layer

I've just started with the basics of OOP. At first it looked complicated, but once I just started with it, it seemed so easy and everything seems to work like a charm, and that's why I'm unsure if I'm doing this right.

I'm creating a system to track actions:

This is the Timer class:

``
class Timer {

private $_db;
public $user_id;

public function __construct($db, $user_id) {

$this->_db = $db;
$this->user_id = $user_id;

}

public function checkActiveTimer() {

$db = $this->_db;

$query = $db->prepare("SELECT * FROM qrm_logs WHERE active = :active");
$query->execute(array(

':active' => 1

));

$result = $query->fetchAll(PDO::FETCH_OBJ);

if (count($result) > 0) {

return true;

}

return false;

}

public function getTimerTypes() {

$db = $this->_db;

$query = $db->prepare("SELECT * FROM qrm_types WHERE active = :active");
$query->execute(array(

':active' => 1

));

$types = $query->fetchAll(PDO::FETCH_OBJ);

if (count($types) > 0) {

return $types;

}

return false;

}

public function startTimer() {

$db = $this->_db;
$user_id = $this->user_id;

$date = new Datetime();

$query = $db->prepare("INSERT INTO qrm_logs (user_id, type_id, active, started_at) VALUES (:user_id, :type_id, :active, :started_at)");
$query->execute(array(

':user_id' => $user_id,
':type_id' => 6,
':active' => 1,
':started_at' => $date->format('Y-m-d H:i:s')

));

}

public function getTime() {

$db = $this->_db;
$user_id = $this->user_id;

$query = $db->prepare("SELECT started_at FROM qrm_logs WHERE active = :active AND user_id` = :user_id");
$query->execute(array(

':a

There's a number of issues with your code. I'll walk through your class pointing out several issues as I go along:

class Timer {

    private $_db;
    public $user_id;

I'll just mention the same point as chervand mentioned here, but it applies to the entire class: coding standards matter, Please follow the PSR standards as much as possible: brackets go on a new line, private or protected property names needn't start with an underscore (that's something that stems from the old PHP4 days).

Also try to add doc-blocks, documenting what arguments should be passed, and what the value/type of each property is:

class Timer
{
    /**
     * @var PDO
     */
    protected $db = null;//protected, in case you'll extend this class
    /**
     * @var int
     */
    public $userId = null;//I'm assuming int, and I prefer camelCase

Next: your constructor: you're expecting the user to pass a "db". You don't specify what that variable must be. Use type-hints to prevent users from passing invalid things to methods. Especially when you're dealing with dependencies.

The userId argument is also required to be passed to the constructor. If it is that important to have this value, then why is it public? There's nothing stopping me from doing something like this:

$timer = new Timer($pdo, 123);
$timer->userId = 'Some invalid value';

It's probably best to make the property protected here, or require the caller to pass the userId as an argument to the methods that require this value. Anyway...

Here's how I'd write the constructor:

public function __construct(PDO $db, $userId)
    {//PSR

        $this->db = $db;
        $this->userId = (int) $userId;//cast to int, so we're sure we're not dealing with a string
    }

Moving on:

public function checkActiveTimer()
    {//PSR

        $db = $this->db;

        $query = $db->prepare("SELECT * FROM qrm_logs WHERE `active` = :active");
        $query->execute(array(

            ':active'   =>  1

        ));

        $result = $query->fetchAll(PDO::FETCH_OBJ);

        if (count($result) > 0) {

            return true;

        }

        return false;

    }

your methods, almost all of them, have the same problem: they don't take any arguments, they contain hard-coded (and sometimes bad) queries. They don't handle possible exceptions, you're using prepared statements (which generally is a good thing), even if you don't (in which case, prepared statements are just overhead).

The biggest problem with your using prepared statements, though, is that you're missing out on the best part about them. Prepared statements allow you to re-use a query. You're preparing the same query string over and over again... pointlessly. Here's what my method would look like:

public function getActiveTimerCount()
{
    $stmt = $this->db->query(
        "SELECT COUNT(`active`) as cnt FROM qrm_logs WHERE `active` = 1"
    );
    $row = $stmt->fetch(PDO::FETCH_ASSOC);
    return (int) $row['cnt']
}

Now this method is a lot shorter. I know that active will be 1, so I'm not at risk of SQL injection attacks here. Instead of fetching all rows, I'm asking the DB to return me the row count, and I'm returning the count. This is more informative to the caller. 0 is still a fasly value, other values are truthy. The caller can still use this method as though it returned a boolean. However, because I'm returning the actual count, the caller can use the same method to work out how many active timers there are.

I also got rid of that reassignment ($db = $this->_db). It's personal preference mainly, but in this case, I think $this->db->query is just as easy to read.

The same basic comments apply for the getTimerTypes method

First, slightly late update

Ok, I promised to revisit this review when I had the time, it took some patience, but I found a free 15min, which I'll spend reviewing this code:

public function getTime() {

    $db = $this->_db;
    $user_id = $this->user_id;

    $query = $db->prepare("SELECT `started_at` FROM qrm_logs WHERE `active` = :active AND `user_id` = :user_id");
    $query->execute(array(

        ':active'   =>  1,
        ':user_id'  =>  $user_id

    ));

    $result = $query->fetchAll(PDO::FETCH_OBJ);

    if (count($result) > 0) {

        foreach ($result as $time) {

            $date = new Datetime($time->started_at);
            $date2 = new Datetime();

            $difference = $date2->getTimestamp() - $date->getTimestamp();

            return $difference;

        }

    }

    return false;

}

As before, the comments about PSR coding standards, and there being no real need to reassign the properties to local variables still apply.

You are using prepared statements, and you're using them to use the userId value (which is provided by the caller, and therefore not to be trusted) safely. This of course makes perfect sense. You're also binding a hard-coded 1 which really doesn't make that much sense

StackExchange Code Review Q#119487, answer score: 5