Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

<?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"; 
}
}
}
?>
share|improve this question

1 Answer 1

up vote 3 down vote accepted
  1. Proper and consistent indentation would improve the readability a lot. Here is mine:

    <?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"; 
                }
            }
        }
    ?>
    
  2. 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

  3. 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.)

  4. 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.

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

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

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

    $Result = $Start - $Stop;
    
  8. 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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.