HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP Check user and delete if expired

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
deletephpuserexpiredandcheck

Problem

Currenty coding a PHP script that will have a CRON to auto run it.
The point is that it checks the user in Mysql and if it has expired, it will delete it from the admin table and set status to expired.
I have used time() and then when it gets INSERT When his trial starts, it starts for example "1347054850" in the "Start" and then i add "900" on it using +, and the result is "1347055750", So the trial is for 15 minutes.
But the point, will this scipt work propery or does it have issues?

connect_error) { die("Sorry, Could not connect (".$Sql->connect_errno.") ".$Sql->connect_error);}
$Data = $Sql->query("SELECT * FROM trial");
while ($q = $Data->fetch_assoc()) {
$Start = $q['Start'];
$Stop = $q['Stop'];
$Steam = $q['Steam'];
$Result = $Start - $Stop;
$Time = time();
if ($q['Expired'] == "True") {echo "All expired \n";} else {
if ($Stop Query("DELETE FROM `sm_admins` WHERE `identity` = '".$Steam."'");
$Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '".$Steam."'");
} 
else 
{ 
echo "All ok, 0 deleted"; 
}
}
}
?>

Solution

-
Proper and consistent indentation would improve the readability a lot. Here is mine:

connect_error) { 
        die("Sorry, Could not connect (" . $Sql->connect_errno . ") " . $Sql->connect_error);
    }
    $Data = $Sql->query("SELECT * FROM trial");
    while ($q = $Data->fetch_assoc()) {
        $Start = $q['Start'];
        $Stop = $q['Stop'];
        $Steam = $q['Steam'];
        $Result = $Start - $Stop;
        $Time = time();
        if ($q['Expired'] == "True") {
            echo "All expired \n";
        } else {
            if ($Stop Query("DELETE FROM `sm_admins` WHERE `identity` = '" . $Steam . "'");
                $Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '" . $Steam . "'");
            } else { 
                echo "All ok, 0 deleted"; 
            }
        }
    }
?>


-
These conditions are good candidates for guard clauses:

if ($q['Expired'] == "True") {
    echo "All expired \n";
} else {


and

if ($Stop <= $Time) {


Here is the modified version:

while ($q = $Data->fetch_assoc()) {
    $Start = $q['Start'];
    $Stop = $q['Stop'];
    $Steam = $q['Steam'];
    $Result = $Start - $Stop;
    $Time = time();
    if ($q['Expired'] == "True") {
        echo "All expired \n";
        continue;
    }
    if ($Stop > $Time) { 
        echo "All ok, 0 deleted"; 
        continue;
    }
    echo "Deleted " . $Steam . " From trial Reason: Expired"; 
    $Sql->Query("DELETE FROM `sm_admins` WHERE `identity` = '" . $Steam . "'");
    $Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '" . $Steam . "'");
}


References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

-
Furthermore, if you are not interested in those records whose Expired flag is True and Stop attribute is bigger than the current time you could modify the SQL query to filter those records, for example:

$Data = $Sql->query("SELECT * FROM trial WHERE Expired != `True` AND Stop > " . $Time);


(If you are using string concatenation to create SQL commands be careful about SQL injections attacks. The value of $Time comes from the time() function so it can't contain any malicious data here.)

-
I'd consider using MySQL's datetime attributes for storing the date informations and using MySQL's date and time functions for the comparison. I'd improve the database structure and make the raw data readable. You might want to prepare for situations when the current time is different between the webserver and the database server.

-
$mysql->query has a return value. You might want to check whether it's FALSE and print a proper error message.

-
PHP functions are not case sensitive, but according to this answer you might want to modify ->Query(...) to ->query(...).

-
This line seems unnecessary, since the $Result variable is not used:

$Result = $Start - $Stop;


-
The delete and update statement uses the $Steam variable as the part of the query without escaping it. I think it could cause 2nd order SQL attacks.

Code Snippets

<?php
    $Sql = new mysqli("localhost", "root", "*******", "serveradmin");
    if ($Sql->connect_error) { 
        die("Sorry, Could not connect (" . $Sql->connect_errno . ") " . $Sql->connect_error);
    }
    $Data = $Sql->query("SELECT * FROM trial");
    while ($q = $Data->fetch_assoc()) {
        $Start = $q['Start'];
        $Stop = $q['Stop'];
        $Steam = $q['Steam'];
        $Result = $Start - $Stop;
        $Time = time();
        if ($q['Expired'] == "True") {
            echo "All expired \n";
        } else {
            if ($Stop <= $Time) { 
                echo "Deleted " . $Steam . " From trial Reason: Expired"; 
                $Sql->Query("DELETE FROM `sm_admins` WHERE `identity` = '" . $Steam . "'");
                $Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '" . $Steam . "'");
            } else { 
                echo "All ok, 0 deleted"; 
            }
        }
    }
?>
if ($q['Expired'] == "True") {
    echo "All expired \n";
} else {
if ($Stop <= $Time) {
while ($q = $Data->fetch_assoc()) {
    $Start = $q['Start'];
    $Stop = $q['Stop'];
    $Steam = $q['Steam'];
    $Result = $Start - $Stop;
    $Time = time();
    if ($q['Expired'] == "True") {
        echo "All expired \n";
        continue;
    }
    if ($Stop > $Time) { 
        echo "All ok, 0 deleted"; 
        continue;
    }
    echo "Deleted " . $Steam . " From trial Reason: Expired"; 
    $Sql->Query("DELETE FROM `sm_admins` WHERE `identity` = '" . $Steam . "'");
    $Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '" . $Steam . "'");
}
$Data = $Sql->query("SELECT * FROM trial WHERE Expired != `True` AND Stop > " . $Time);

Context

StackExchange Code Review Q#15425, answer score: 5

Revisions (0)

No revisions yet.