4
\$\begingroup\$

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(

            ':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;

    }

    public function stopActiveTimer() {

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

        $date = new Datetime();

        $query = $db->prepare("UPDATE `qrm_logs` SET `active` = :inactive, `ended_at` = :ended_at WHERE `user_id` = :user_id AND `active` = :active");
        $query->execute(array(

            ':inactive' =>  0,
            ':ended_at' =>  $date->format("Y-m-d H:i:s"),
            ':user_id'  =>  $user_id,
            ':active'   =>  1

        ));

    }

    public function startWhiteSpaceTimer($type_id) {

        $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'      =>  $type_id,
            ':active'       =>  1,
            ':started_at'   =>  $date->format('Y-m-d H:i:s')

        ));

    }

}

This is a piece of code I use in an AJAX request:

$timer = new Timer($db, $_SESSION['id']);

if ($timer->checkActiveTimer() !== false) {

    $timer->stopActiveTimer();

}

$timer->startWhitespaceTimer($type_id);

Can you give me some feedback on how I'm currently approaching it (right or wrong), and eventually correct some errors/mistakes?

\$\endgroup\$
7
  • 2
    \$\begingroup\$ Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. \$\endgroup\$ Feb 10 '16 at 12:17
  • \$\begingroup\$ I've rolled your question back. Please read What should I do when someone answers my question? \$\endgroup\$
    – Quill
    Feb 11 '16 at 21:43
  • \$\begingroup\$ So I do can answer my question with the example to show how I would improve it? \$\endgroup\$ Feb 11 '16 at 21:46
  • \$\begingroup\$ If it's written like a review (i.e. not like a code dump), sure! \$\endgroup\$ Feb 11 '16 at 21:47
  • 1
    \$\begingroup\$ You asked if you could answer your question with the example to show how you would improve it - the answer to that is yes, if it's written like a review. Bullet 3 specifically says "If you want to show everyone how you improved your code, but don't want to ask another question, then post an answer to your own question." , and the two bullets under it specify exactly how to do that: your answer must meet the standards of a Code Review answer, just like any other answer. - I said "yes" to question asking if you could answer your own post, not whether a new question is ok as an answer. \$\endgroup\$ Feb 11 '16 at 22:26
5
\$\begingroup\$

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 at all. For starters, if this value is something only of interest to your class, you'd probably want to define it as a constant. If the value ever needs to change, it's easier to change the value assigned to a constant once, then going through the code and changing all occurrences of 1 to their new value. Especially if the value, like 1, can be used for things other than the active value.
More importantly here, though, is that there is no real reason to bind a known integer value. You can just make that part of the WHERE clause part of the prepared statement. It's an immutable part anyway. Here's what I'd do:

//at the top of the class:
const TIMER_ACTIVE = 1;

public function getTime()
{
    //makes query easier to read IMO
    $stmt = $this->db->prepare(
        'SELECT started_at
        FROM qrm_logs
        WHERE `active` = ' . self::TIMER_ACTIVE .
        ' AND `user_id` = :uid'
    );
    $stmt->execute([':uid' => $this->userId]);

The next thing I notice about your code is that you're using fetchAll, then iterate over the results, and return inside the loop. This suggests that all you're really after is a single result. Using LIMT 1 in your query would make this clearer to whoever ends up maintaining this code. If a query can return multiple results, but I'm only after one row, I'd never ever use fetchAll. Personally, I'd use fetch, to get a single row. Your call to fetchAll implies that all results are being fetched and loaded into memory. That's extremely wasteful considering you're never ever processing more than a single row.
You're also using count to check whether or not there are any results to process. There's no reason for that function call, though. If you use fetch, the return value is null if there aren't any results, or the row (as an object, or array, or whichever fetch mode you use).
even if you use fetchAll, though, an empty array in PHP is a falsy value, so you could just as well write if (count($result) > 0) { like so:

if ($result) {
    //$result is NOT empty
}
//or:
if (!$result) {//in PHP, !array() is true
    return false;//
}
//process $results here

So what does that mean for the code we've discussed so far, and how would I write it?

public function getTime()
{
    //makes query easier to read IMO
    $stmt = $this->db->prepare(
        'SELECT started_at
        FROM qrm_logs
        WHERE `active` = ' . self::TIMER_ACTIVE .
        ' AND `user_id` = :uid'
    );
    $stmt->execute([':uid' => $this->userId]);

    $row = $stmt->fetch(PDO::FETCH_OBJ);
    if (!$row) {
        return false;//no active records found for user
    }

Now all that's left for us to do is to return the interval in seconds. I personally advocate the use of the DateTimeInterface objects as much as possible. However, in this particular case, I don't think it's worth it. You want to subtract a date (in timestamp format) from the current timestamp. You can do that as a one-liner by calling 2 simple functions: time() and strtotime(). The first just returns the current timestamp, the second returns the timestamp for the string representation of a date. Seeing as started_at is coming from the DB, I'm assuming it's a MySQL DATETIME or TIMESTAMP field, and its values can be handled by strtotime just fine. Here's how I'd write your function in full:

public function getTime()
{
    $stmt = $this->db->prepare(
        'SELECT started_at
        FROM qrm_logs
        WHERE `active` = ' . self::TIMER_ACTIVE .
        ' AND `user_id` = :uid'
    );
    $stmt->execute([':uid' => $this->userId]);

    $row = $stmt->fetch(PDO::FETCH_OBJ);
    if (!$row) {
        return false;
    }

    return time() - strtotime($row->started_at);
}

This looks simple, clean and easy.
A Final not, instead of passing those PDO::FETCH_* constant every time you're fetching, you might want to consider setting the default fetch mode on the PDO instance ($this->db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);), provided no other piece of code has access to the same PDO instance, or that other piece of code doesn't change the default fetch mode, you don't have to bother passing the fetch mode every time again...


I'll look into the rest of the code a bit later on, and update this answer as I go along. This is all I have time for for now.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for this great feedback. You have my upvote, but not yet accepted as I'll await further feedback. Indeed, my approach regarding query's isn't good. I will update my answer, with an example of how I think I should handle a specific query. Example: getting the number of rows for each table. \$\endgroup\$ Feb 11 '16 at 21:33
  • \$\begingroup\$ Update on my preview comment. Please view the answer that I've posted. I think the method I provided in it is way more reusable and dynamic. Could you add the review of it in your answer? Thanks! \$\endgroup\$ Feb 11 '16 at 21:59
  • \$\begingroup\$ Thanks for the updated answer. Realy useful as it gets cleaner & forces me to look deeper into the docs to write cleaner code! Thanks :) \$\endgroup\$ Apr 11 '16 at 8:23
1
\$\begingroup\$

It seems OK regarding to OOP. I can only suggest you to create some PDO operations implementing pattern like ActiveRecord, since every method contains quite the same DB querying logic. Additionally, you may want to use magic methods like __get() and __set() to make your class API look more intuitive.

PS: following some code style standard like PSR is also a good thing to consider about.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the PSR standards. Didn't know of their existance. Thanks! \$\endgroup\$ Feb 10 '16 at 12:20
  • \$\begingroup\$ "It seems OK regarding to OOP": I'm sorry, but it doesn't. Also: I'd strongly advise against implementing __get and __set (or any other magic method) unless you have a really, really good reason to do so. They are slow, make code more error-prone, harder to test, and make the API more messy, not more intuitive. If you want to allow the user to change a property, define a setter. Don't let them access protected/private properties via __set, it's not a public property, don't make it behave like one \$\endgroup\$ Feb 11 '16 at 15:31

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.