Problem
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?
Solution
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.
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.