3
\$\begingroup\$

I'm new to web development, and I'm constantly torn between good code and bad code. I was wondering if you could help me improve my following code and see if I'm doing it right in terms of how I use it on the line by line basis: - t code checks to see if a cookie exists if it doesnt exist it sets a cookie to last for 23 hours and 30 minutes after the cookie is set it pulls a random poem from a database using and and sets it on page to be displayed if the user visits the page more than once and the cookie is set no data will be retrieved and leave old data till the cookie expires -

<?php

if (@!$_COOKIE['true']){
    setcookie('true','true',time()+86100);

    $timeCheck = date('H:i', strtotime('23:30:00'));

    $currentTime = date('H:i', time());
    if ($currentTime >= $timeCheck){
            require_once 'upload/mysqlserver/login.php';

    $query = "SELECT * FROM poems";
    $queryRun = mysql_query($query);
     if ($queryRun = mysql_query($query)){
                // getting data
                if (mysql_num_rows($queryRun)== NULL){
                   echo "No poems in DataBase";
                }else{
                    $randNum = mysql_num_rows($queryRun);
                    $maxNum = rand(1, $randNum);

                    $query2 = "SELECT poem_name, poem, author, time_posted FROM poems WHERE id=$maxNum";
                    $queryRun2 = mysql_query($query2);

                        $row = mysql_fetch_assoc($queryRun2);
                        $poemName = $row['poem_name'];
                        $poem = $row['poem'];
                        $author = $row['author'];
                        $time_posted = $row['time_posted'];

                        $PoTDset = $poemSet = "<br />
                        <div class='allContent'>
                        <div class='poem'>
                        <h1>Poem name:</h1><h2>".$poemName ."</h2>
                        <br />"
                        ."<h2>Poem:</h2><br />"."<p class='poemScroll'>".$poem."</p>
                        <br /><br />"
                        ."<h3>Author:</h3> ".$author."<br />"
                        ."<h3>Posted on: </h3>".$time_posted."</div></div>";

                        $handle = fopen('navigation/PoTD/setPoTD.php', 'w');
                        fwrite($handle, "<?php"."\n"."return ".'"'.$PoTDset.'"'."\n".";\n?>");
                        @fclose($handle);    

                 }
            }
        }
}

$pageTemp->content .= include_once "navigation/PoTD/setPoTD.php";
?>
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Could you explain what your code is trying to do? If we know what you are trying to do specifically, we can improve your code more. Also, can you change your title to state what your code is doing as well? \$\endgroup\$
    – user34073
    Jan 30 '15 at 21:08
  • \$\begingroup\$ You're using mysql_* functions, and you're vulnerable to SQL injection, so no this is not done right! \$\endgroup\$ Jan 30 '15 at 21:10
1
\$\begingroup\$
if (@!$_COOKIE['true']){

Well that's one bad name for a cookie. This doesn't say anything about what this cookie is used for.


    if ($currentTime >= $timeCheck){
            require_once 'upload/mysqlserver/login.php';


    $query = "SELECT * FROM poems";
    $queryRun = mysql_query($query);
     if ($queryRun = mysql_query($query)){

Your indentation goes a bit off on these lines, correcting that makes it:

    if ($currentTime >= $timeCheck) {
        require_once 'upload/mysqlserver/login.php';

        $query = "SELECT * FROM poems";
        $queryRun = mysql_query($query);
        if ($queryRun = mysql_query($query)) {
            ...

Additionally, you are setting the value of $queryRun twice. You can change that too:

$queryRun = mysql_query($query);
if ($queryRun) {

Also, YOU ARE USING THE DEPRECATED MYSQL_ FUNCTIONS*. You should really use the mysqli_ functions instead. Apparently you have not read the documentation for mysql_query where there is a big red box essentially saying: Don't use this.


if (mysql_num_rows($queryRun)== NULL) {

This method does not return NULL, it can return false though, which when comparing with == is equal to null. However, I would recommend comparing with ===. So that would become:

if (mysqli_num_rows($queryRun) === false) {

$query2 = "SELECT poem_name, poem, author, time_posted FROM poems WHERE id=$maxNum";

This is vulnerable to SQL Injection if somehow $maxNum would somehow get the value 0; DROP TABLE poems;

Always use parametrized SQL queries!


$PoTDset = $poemSet = "<br />

Do you really need both $PoTDset and $poemSet ?

\$\endgroup\$
1
  • \$\begingroup\$ thanks this code is back ground code in which the cookie test to see if its set if it isnt it will be and a page update will be performed when page is visited ill fix the name later. Its more like background code for retrieving data and not sending data to a database. Thank you i'll add in your suggestions \$\endgroup\$
    – Prince
    Jan 31 '15 at 0:26
1
\$\begingroup\$

In addition to the depreciation and injection issues pointed out in the comments, I would just add that SELECT * FROM poems will fetch all columns for all rows, which is terribly inefficient. SELECT COUNT(*) FROM poems will return the number of records in the table, and you won't have to call mysql_num_rows.

\$\endgroup\$

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.