How to improve

[TheCase]

Hi,

Im a self taught programmer and have picked up bad habbits this is my code please tell me how I can improve on this as I dont think its perfect.

<?php
  include_once('header.htm');
  require_once('functions.php');
  logincheck();
  $userid = $_COOKIE['userid'];


  $result = mysql_query("SELECT admin FROM users_info WHERE id='$userid' AND admin='0'");
  $num_rows = mysql_num_rows($result);

  if ($num_rows == '0')
      {
      echo '<h1>Add Rota</h1>';

  echo '
   <form action ="" method="post">
     <p><label>Presenter 1:</label>
  <select name="presenter1">
  <option value="0">Pick presenter</option>
';

  $query = mysql_query("SELECT id, username, lastname FROM users WHERE id !='31' ORDER BY username ASC ");
  while ($row = mysql_fetch_array($query, MYSQL_NUM))
      {
      $options[] = "<option value=\"{$row['0']}\">{$row['1']} {$row['2']} </option>\n";
      }


  foreach ($options as $v)
      {
      echo $v;
      }

  echo '  </p></select>
  <p><label>Presenter 2:</label>
  <select name="presenter2">
  <option value="31">No presenter</option>';

  reset($options);

  foreach ($options as $v)
      {
      echo $v;
      }

  echo '</p></select>

 <p><label>Engineer:</label>
  <select name="engineer">
  <option value="31">Self Op</option>';

  reset($options);

  foreach ($options as $v)
      {
      echo $v;
      }

  echo '</p></select>

  <p><label>Time:</label>
  <select name="time">
  <option value="19:00">19:00</option>
  <option value="20:00">20:00</option>
  <option value="20:30">20:30</option>
  <option value="21:00">21:00</option>
</select></label>
  <p><label>Date:</label><select name="datedropdown">';



  for ($i = 1; $i <= 10; ++$i)
      {
      $list[] = date("l d F", strtotime('+' . $i . ' Sunday')) . '';
      }



  foreach ($list as $ii)
      {
      $iii = str_replace("Sunday", "", "$ii");
      echo "<option value='$iii'>$ii</option>";
      }


  echo '  </p></select>
   <p><input type="submit" value="Add" name="submitted" class="button" /></p>
</form>

';
      }

  echo '<h1>Current Rota</h1><BR><center><p>Click on your name to approve or decline, also to add a comment.</p></center> <b><p>Key:</b>&nbsp;<font color="#df8700">Awaiting</font>&nbsp;<font color="#006700">Approved</font>&nbsp;<font color="#cf0000">Declined</font></p>&nbsp;&nbsp;&nbsp;';

  if (isset($_POST['yourshowsfilter']))
      {
      $result2 = mysql_query("SELECT id FROM users WHERE username = '$_COOKIE[username]' ");
      while ($row2 = mysql_fetch_array($result2, MYSQL_ASSOC))
          {
          $userid = $row2['id'];
          }

  $query = mysql_query("SELECT shows.id, time, date, presenter1status, presenter2status, engineerstatus, presenter1comment, presenter2comment, egineercomment,
   u1.username AS presenter1,
   u2.username AS presenter2,
   u3.username AS engineer
FROM shows
   INNER JOIN users AS u1 ON u1.id = shows.presenter1id
   INNER JOIN users AS u2 ON u2.id = shows.presenter2id
   INNER JOIN users AS u3 ON u3.id = shows.engineerid
WHERE presenter1id='$userid' OR presenter2id='$userid' OR engineerid='$userid' ORDER BY date, time ASC  ");


  $whatfilter = '2';
  }
  elseif (isset($_POST['commentsonlyfilter']))
      {
      $query = mysql_query("SELECT shows.id, time, date, presenter1status, presenter2status, engineerstatus, presenter1comment, presenter2comment, egineercomment,
   u1.username AS presenter1,
   u2.username AS presenter2,
   u3.username AS engineer
FROM shows
   INNER JOIN users AS u1 ON u1.id = shows.presenter1id
   INNER JOIN users AS u2 ON u2.id = shows.presenter2id
   INNER JOIN users AS u3 ON u3.id = shows.engineerid
WHERE presenter1comment!='No Comment' OR presenter2comment!='No Comment' OR egineercomment!='No Comment' ORDER BY date, time ASC  ");

  $whatfilter = '2';
  }
  elseif (isset($_POST['awaitingfilter']))
      {
      $query = mysql_query("SELECT shows.id, time, date, presenter1status, presenter2status, engineerstatus, presenter1comment, presenter2comment, egineercomment,
   u1.username AS presenter1,
   u2.username AS presenter2,
   u3.username AS engineer
FROM shows
   INNER JOIN users AS u1 ON u1.id = shows.presenter1id
   INNER JOIN users AS u2 ON u2.id = shows.presenter2id
   INNER JOIN users AS u3 ON u3.id = shows.engineerid
WHERE presenter1status='0' OR presenter2status='0' OR engineerstatus='0' ORDER BY date, time ASC  ");

  $whatfilter = '2';
  }
  elseif (isset($_POST['declinefilter']))
      {
      $query = mysql_query("SELECT shows.id, time, date, presenter1status, presenter2status, engineerstatus, presenter1comment, presenter2comment, egineercomment,
   u1.username AS presenter1,
   u2.username AS presenter2,
   u3.username AS engineer
FROM shows
   INNER JOIN users AS u1 ON u1.id = shows.presenter1id
   INNER JOIN users AS u2 ON u2.id = shows.presenter2id
   INNER JOIN users AS u3 ON u3.id = shows.engineerid
WHERE presenter1status='2' OR presenter2status='2' OR engineerstatus='2' ORDER BY date, time ASC  ");

  $whatfilter = '2';
  }
  else
      {
      $query = mysql_query("SELECT shows.id, time, date, presenter1id, presenter2id, engineerid, presenter1status, presenter2status, engineerstatus, presenter1comment, presenter2comment, egineercomment,
   u1.username AS presenter1,
   u2.username AS presenter2,
   u3.username AS engineer
FROM shows
   INNER JOIN users AS u1 ON u1.id = shows.presenter1id
   INNER JOIN users AS u2 ON u2.id = shows.presenter2id
   INNER JOIN users AS u3 ON u3.id = shows.engineerid
ORDER BY date, time ASC  ");

  $whatfilter = '1';
  }

  if ($whatfilter == '1')
      {
      echo '<form action="" method="post"><div align="right"><input type="submit" value="Filter to comments only" name="commentsonlyfilter" class="button" />&nbsp;&nbsp;<input type="submit" value="Filter to only your shows" name="yourshowsfilter" class="button" /><BR><BR><input type="submit" value="Show Awaiting" name="awaitingfilter" class="button" />&nbsp;&nbsp;<input type="submit" value="Show Declined" name="declinefilter" class="button" /></div></form><BR>';
      }
  elseif ($whatfilter == '2')
      {
      echo '<BR>
<FORM><div align="right"><INPUT TYPE="BUTTON" VALUE="Go Back" 
ONCLICK="history.go(-1)"></FORM></div><BR><BR>';
      }

  $bg = '#708090';

  echo "<table width='70%' cellspacing='0' cellpadding='0' border='1' align='center'>

<tr>
<td align='center' width='12%'><b><p>Date / Time</p></b></td>
<td align='center' width='30%'><b><p>Presenter 1</p></b></td>
<td align='center' width='30%'><b><p>Presenter 2</p></b></td>
<td align='center' width='30%'><b><p>Engineer</p></b></td>
<td align='center' width='30%'><b><p>Admin</p></b></td>
</tr>";

  $colors = array("#df8700", "#006700", "#cf0000");

  while ($row = mysql_fetch_array($query, MYSQL_ASSOC))
      {
      $bg = ($bg == '#708090' ? '#bcbcbc' : '#708090');

  echo "<tr>

  <td align=\"center\">";
      echo "</td> ";
      $id = $row['id'];

  if ($row['presenter1id'] == $userid)
      {
      echo "<td bgcolor=\"$bg\" align=\"center\ ><a style=\"color: {$colors[$row['presenter1status']]}\" href='./rotastatus.php?id=$id'>{$row['presenter1']}</a></td>";
      }
  else
      {
      echo " <td bgcolor=\"$bg\" align=\"center\"><font color=\"{$colors[$row['presenter1status']]}\"><p class='table'>{$row['presenter1']}</p> </font></td>";
      }

  if ($row['presenter2id'] == $userid)
      {
      echo "<td bgcolor=\"$bg\" align=\"center\"><a style=\"color: {$colors[$row['presenter2status']]}\" href='./rotastatus.php?id=$id'>{$row['presenter2']}</a></td>";
      }
  elseif ($row['presenter2'] == 'SYSTEM')
      {
      echo " <td  bgcolor=\"$bg\"align=\"center\"><font color='green'><p class='table'>No Presenter</p> </font></td> ";
      }
  else
      {
      echo " <td bgcolor=\"$bg\" align=\"center\"><font color=\"{$colors[$row['presenter2status']]}\"><p class='table'>{$row['presenter2']} </p></font></td>";
      }
  if ($row['engineerid'] == $userid)
      {
      echo "<td bgcolor=\"$bg\" align=\"center\"><a style=\"color: {$colors[$row['engineerstatus']]}\" href='./rotastatus.php?id=$id'>{$row['engineer']}</a></td>";
      }
  elseif ($row['engineer'] == 'SYSTEM')
      {
      echo " <td bgcolor=\"$bg\" align=\"center\"><font color='green'><p class='table'>Self-Op</p> </font></td> ";
      }
  else
      {
      echo " <td bgcolor=\"$bg\" align=\"center\"><font color=\"{$colors[$row['engineerstatus']]}\"><p class='table'>{$row['engineer']} </p> </font></td>";
      }


  echo "  <td bgcolor=\"$bg\" align=\"center\"><input type='button' value='Edit' onClick='window.location.href=\"rota.php?id=$id\" '></form></td> 

</tr>

  <tr>
  <td bgcolor=\"$bg\" align=\"center\"><p>{$row['time']} {$row['date']}</p><BR></td> 
  <td bgcolor=\"$bg\" align=\"center\"><p class='table'>{$row['presenter1comment']}</p></td>
  <td bgcolor=\"$bg\" align=\"center\"><p class='table'>{$row['presenter2comment']}</p></td> 
  <td bgcolor=\"$bg\" align=\"center\"><p class='table'>{$row['egineercomment']}</p></td>  

  <td bgcolor=\"$bg\" align=\"center\"><BR></td>  

  </tr>";
      }
  unset($colors, $row);
  mysql_close($dbc);

  echo '</table><BR><BR><BR>';


?>

Thanks

[bradgrafelman]

No one's code is ever "perfect" (and if it is, they need to stop writing a one-line "Hello World" program and move on to something useful :p), but here are my thoughts/comments/suggestions/etc.:

1. Since we can't see functions.php (where I presume the 'logincheck' function is defined), there isn't much critiquing we can do.

2. In general, you should avoid accessing external data without first verifying that it exists. For example, this line:

   $userid = $_COOKIE['userid'];

   will generate E_NOTICE level error messages every page load where a 'userid' cookie wasn't sent. If your logincheck() function ensures that this will never happen, that's fine, but I just wanted to point that out in case it didn't.

3. User-supplied data should never be placed directly into a SQL query string, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors. Instead, you must first sanitize the data with a function such as [man]mysql_real_escape_string/man (for string data).

4. If you're just performing a SELECT query to test for existence, note that you can simply say 'SELECT 1' or 'SELECT TRUE' (or any other arbitrary, small, fixed value) instead of actually selecting a column. I try to emphasize SELECT'ing only columns from which you actually need data, so a 'SELECT 1' or 'SELECT TRUE' type of query stands out (in my eyes, at least) as one of these "existence-only" tests.

5. While PHP is loosely-typed and won't complain, note that this:

   if ($num_rows == '0') 

   is comparing the return value of [man]mysql_num_rows/man, an integer, to the string '0' (instead of the number or integer 0).

   Also note that this applies to SQL queries as well; numeric data shouldn't be delimited with quotes (which makes it string data instead).

6. In smaller projects this suggestion might create more overhead than its worth, but I always try to separate display markup from server-side code. In other words, there is very little HTML code in my PHP scripts and very little (if any) PHP in my HTML documents/templates.

   It makes working on one or the other a lot easier.

7. Any particular reason why you're using numerical indexes when fetching data from MySQL, e.g. $row['1'] (which, again, should instead be written as $row[1] since the indexes are numbers and not strings)? To me, $row[1] doesn't really mean anything, but $row['username'] certainly does, hence why I almost always use [man]mysql_fetch_assoc/man instead.

8. If you simply want to output all entities in an array, note that you could use [man]implode/man instead of a foreach() loop.

9. Regardless of whether you implode() or use a foreach() loop, note that there is no need to call [man]reset/man to reset the internal array pointer (which will be done automatically in either scenario).

10. Any reason why you have both of these loops:

          for ($i = 1; $i <= 10; ++$i) 
              { 
              $list[] = date("l d F", strtotime('+' . $i . ' Sunday')) . ''; 
              } 
    
    
    
      foreach ($list as $ii) 
          { 
          $iii = str_replace("Sunday", "", "$ii"); 
          echo "<option value='$iii'>$ii</option>"; 
          } 
    

    ? Why not just output the necessary HTML in the for() loop?

11. I won't critique your HTML, but I will offer two pieces of advise. First, learn CSS (and get rid of deprecated things such as <font> tags) as it makes formatting much, much easier and portable (across different browsers). Second, make sure your HTML is valid.

12. Here:

          $result2 = mysql_query("SELECT id FROM users WHERE username = '$_COOKIE[username]' "); 
          while ($row2 = mysql_fetch_array($result2, MYSQL_ASSOC)) 
              { 
              $userid = $row2['id']; 
              } 

    Why do you have a while() loop there? Shouldn't the username be a unique value, meaning you only expect at most one row? If so, then there's no point in looping 1 time only.

13. If the username is indeed a unique value, then why have the separate SELECT query at all? Get rid of it and instead just modify the WHERE condition of the second query to look for the username value.

14. Any reason why 'date' and 'time' are two separate columns in your DB? Seems like they could/should instead be a single column of type DATETIME.

15. There is a lot of repetition (which is always a red flag for possibly unoptimized code) in regards to the MySQL queries, i.e. the only thing that changes from one if() section to the next is the WHERE clause. Why not write the query once and use the if/elseif/etc. to add the appropriate WHERE condition instead?

[TheCase]

Thank for your reply it was very helpful.

1 and 2. logincheck() checks in functions if the $userid exsits

3. I only implemented basic security because this is a private script over a LAN so its is unlilkey someone to breach the security that needs the system

4. I did not know this and has been changed to

     mysql_query("SELECT TRUE FROM users_info WHERE id='$userid' AND admin='0'"); 

5. I also did not know this, although I do not get any errors I will start to use this.

6. A habbit I guess I like to keep everything on one page

7. Very bad habbit, I agree it will be hard to find certain code so I changed all queries to

         $query = mysql_query("SELECT id, username, lastname FROM users WHERE id !='1' ORDER BY username ASC ");
         while ($row = mysql_fetch_assoc($query))

8. Not sure how to do this, can you give any pointers

9. Didnt know this thanks

10. Fix this with the following code

          for ($i = 1; $i <= 10; ++$i)
              {
              $datelist = date("l d F", strtotime('+' . $i . ' Sunday')) . '';
    		  $iii = str_replace("Sunday", "", "$datelist");
              echo "<option value='$iii'>$datelist</option>";
              }
    

11. Yes there is some CSS but it is not all completly CSS I will fix this

12. Your correct this was a mistake

13. Sorry where is the seperate SELECT query

14. I will change this im not sure why I did this

15. To tidy up the duplication I done the following

    if (isset($_POST['yourshowsfilter'])){
    $whatfilter = '2';
    $where = "WHERE presenter1id='$userid' OR presenter2id='$userid' OR engineerid='$userid'";
    }elseif (isset($_POST['commentsonlyfilter'])){
    $whatfilter = '2';
    $where = "WHERE presenter1comment!='No Comment' OR presenter2comment!='No Comment' OR egineercomment!='No Comment'";
    }elseif (isset($_POST['awaitingfilter'])){
    $whatfilter = '2';
    $where = "WHERE presenter1status='0' OR presenter2status='0' OR engineerstatus='0'"; 	
    }elseif (isset($_POST['declinefilter'])){
    $whatfilter = '2';
    $where = "WHERE presenter1status='2' OR presenter2status='2' OR engineerstatus='2'";
    }else{
    $whatfilter = '1';
    $where = "";
    }
    
          $query = mysql_query("SELECT shows.id, time, date, presenter1id, presenter2id, engineerid, presenter1status, presenter2status, engineerstatus, presenter1comment, presenter2comment, egineercomment,
       u1.username AS presenter1,
       u2.username AS presenter2,
       u3.username AS engineer
    FROM shows
       INNER JOIN users AS u1 ON u1.id = shows.presenter1id
       INNER JOIN users AS u2 ON u2.id = shows.presenter2id
       INNER JOIN users AS u3 ON u3.id = shows.engineerid
       $where
    ORDER BY date, time ASC 
    
     ");
    

I would be greatful if you let me know what you think. As some gauge of how well the optimiziation went using the memory_get_usage() function it started at 131520 now is at 123760, so uses roughly 7760 less memory.

Thank you for your time and feedback