Critigue please

[austinderrick2]

<?
include("myfiles/myfolder/etc/etc/Includes/mysqlconnection.php");
$query = 'SELECT * FROM `futurepolls` LIMIT 0, 30';
$result = mysql_query ($query,$connection);
while ($row = mysql_fetch_array($result)){
     if($is != "yes"){
        $question = $row['question'];
        $oid = $row['oid'];
        $is = "yes";
     }
}
     $sql1 = "DELETE FROM polloftheweek";
     $result1 = mysql_query($sql1);
     $sql2 = "DELETE FROM pollresults";
     $result2 = mysql_query($sql2);
     $sql3 = "INSERT INTO polloftheweek (`oid`, `question`) VALUES (' ', \"$question\");";
     // The "" instead of '' is to avoid problems with ' in the question. Still has problems with " though
     $result3 = mysql_query($sql3);
     $sql4 = "DELETE FROM futurepolls WHERE oid = '$oid'";
     $result4 = mysql_query($sql4);
?>

This is the cron.php that runs to switch the polls on my site from the "futurepolls" table to the "polloftheweek" table. Any better organization of my code would be appriciated!

[Weedpacket]

"SELECT question, oid" would be nicer than "SELECT *".

What's the idea behind the loop? Apart from the E_NOTICE that's thrown because $is doesn't start out being defined, $question and $oid are set because $is is not equal to "yes". After that, $is is equal to "yes" and so every subsequent iteration through the loop becomes "fetch a row but don't do anything with it".

[Shrike]

Doesn't the query simply mean that the second poll in data set? Selecting all rows only to retrieve the second seems a little inefficient.

I also dislike things like $row = mysql_fetch_array() ... $var = $row['key']. Why not just use $row[] wherever necessary?

Finally, addslashes() should take care of most of your quoting problems.

If I were to write a poll I think I would have one polls table (although having a separate results table is really the best way to go), and the ability to mark a row as the current poll. If you wanted to rotate through them using cron *you would simply need to shift the marker with one SQL query.

[austinderrick2]

<?
    $is = "no";
    include("/home/etc/etc/etc/Includes/mysqlconnection.php");
    $query = 'SELECT * FROM `futurepolls` LIMIT 1;';
    $result = mysql_query ($query,$connection);
    while ($row = mysql_fetch_array($result)){
        $question = $row['question'];
        $oid = $row['oid'];
        $is = "yes";
    }
    if($is == "yes"){
        $question = htmlspecialchars("$question", ENT_QUOTES);
        $sql = "DELETE FROM polloftheweek";
        $result = mysql_query($sql);
        $sql = "DELETE FROM pollresults";
        $result = mysql_query($sql);
        $sql = "INSERT INTO polloftheweek (`oid`, `question`) VALUES (' ', '$question');";
        $result = mysql_query($sql);
        $sql = "DELETE FROM futurepolls WHERE oid = '$oid'";
        $result = mysql_query($sql);
    }
?>

How's this?

[Weedpacket]

How is that different?

[Moonglobe]

why are you using "yes" and "no" instead of the keywords true and false? i'd think boolean checks would be a LOT faster than checking on a string.