PHP code review please

Ask about general coding issues or problems here.

Moderators: macek, egami, gesf

Hendrix
New php-forum User
New php-forum User
Posts: 4
Joined: Wed Sep 24, 2003 11:14 am

PHP code review please

Postby Hendrix » Wed Sep 24, 2003 11:26 am

Hi, I am very new to php and I wanted to see if you could review my included code and let me know of any problems or things I should do differently for speed. The code works as it should, I am just not sure if there are security risks or code that lags, etc. etc. Any help would be great!

Here is the code:

Code: Select all

<?
error_reporting(E_ALL);
$PHP_SELF = $_SERVER['PHP_SELF'];
include "settings1.php";
$connection = mysql_connect($host, $user, $pass) or die(mysql_error());
mysql_select_db($dbase, $connection) or die(mysql_error());

$id = (empty($_REQUEST['id'])) ? 1 : $_REQUEST['id'];
$pid = (empty($_REQUEST['pid'])) ? 0 : $_REQUEST['pid'];
$pageData = mysql_query("SELECT title, header, content FROM $table WHERE pageid='$id'") or die(mysql_error());
$catsData = mysql_query("SELECT pageid, parent, name FROM $table WHERE visible='1'") or die(mysql_error());

$title = "404 Not Found";
$header = "404 Not Found";
$content = "The server can not find the requested page.";

while ($pageRow = mysql_fetch_array($pageData))
{
   $title = $pageRow["title"];
   $header = $pageRow["header"];
   $content = $pageRow["content"];
}

include "header.php";
echo "<table border=\"0\" cellspacing=\"0\" cellpadding=\"0\" summary=\"Main table\" width=\"100%\">
<colgroup span=\"4\"><col width=\"112\"><col width=\"7\"><col width=\"15\"><col></colgroup>
<tr><td rowspan=\"2\" valign=\"top\" class=\"bluebg\">
<table border=\"0\" cellspacing=\"0\" cellpadding=\"0\" summary=\"Buttons\">
<colgroup><col width=\"23\"><col width=\"79\"><col width=\"10\"></colgroup>";

$catsCount = 0;
$catsArray[] = "";

while ($catsRow = mysql_fetch_array($catsData)) // this populates the menu with $catsData
{
   $pageid = $catsRow["pageid"];
   $parent = $catsRow["parent"];
   $name = $catsRow["name"];

   if ($pageid == $id)
      $btn = ($parent == '1') ? "btnsubon.gif" : "btnon.gif";
   else
      $btn = ($pageid == $pid) ? "btnsubon.gif" : "btnoff.gif";
      
   $catsArray[] = $name;

   if ($pageid == $id || $pageid == $pid) // when $pageid == id this is the page we are on if it equals pid this is the parent
   {
      $subsData = mysql_query("SELECT pageid, parentid, name FROM $table WHERE parentid=$pageid") or die(mysql_error());
      
      echo "\n<tr><td>\n<a href=\"$PHP_SELF?id=$pageid\">
      <img src=\"images/$btn\" alt=\"$name\" width=\"23\" height=\"25\" border=\"0\"></a></td>
      <td class=\"btn\"><a href=\"$PHP_SELF?id=$pageid\">$name</a></td>
      <td><img src=\"images/btnendm.gif\" width=\"10\" height=\"25\" border=\"0\"></td></tr>";
      
      if ($parent == '1') // indicates that this node is a parent
      {
         echo "<tr><td class=\"btnbtm\"><img src=\"images/space.gif\" width=\"23\" height=\"4\" border=\"0\"></td>
         <td class=\"btnbtm\"><img src=\"images/space.gif\" width=\"79\" height=\"4\" border=\"0\"></td>
         <td><img src=\"images/btnbtmends.gif\" width=\"10\" height=\"4\" border=\"0\"></td></tr>";
   
         while ($subsRow = mysql_fetch_array($subsData)) // here we populate the submenu items
         {
            $pageid = $subsRow["pageid"];
            $parentid = $subsRow["parentid"];
            $name = $subsRow["name"];
            $sub = ($pageid == $id) ? "subon.gif" : "suboff.gif";
   
            if ($pageid == $id)
            {
            echo "\n<tr><td>\n<a href=\"$PHP_SELF?id=$pageid&pid=$parentid\">
            <img class=\"subimg\"  src=\"images/$sub\" alt=\"$name\" width=\"23\" height=\"18\" border=\"0\"></a></td>
            <td class=\"sub\"><a href=\"$PHP_SELF?id=$pageid&pid=$parentid\">$name</a></td>
            <td><img src=\"images/space.gif\" width=\"1\" height=\"18\" border=\"0\"></td></tr>";
            }
            else
            {
            echo "\n\n<tr><td>
            <a href=\"$PHP_SELF?id=$pageid&pid=$parentid\" onMouseOver=\"sHighLight('sub00$pageid');\" onMouseOut=\"sUnHighLight('sub00$pageid');\">
            <img class=\"subimg\" src=\"images/suboff.gif\" alt=\"$name\" name=\"sub00$pageid\" width=\"23\" height=\"18\" border=\"0\"></a></td>
            <td class=\"sub\">
            <a href=\"$PHP_SELF?id=$pageid&pid=$parentid\" onMouseOver=\"sHighLight('sub00$pageid');\" onMouseOut=\"sUnHighLight('sub00$pageid');\">$name</a></td>
            <td><img src=\"images/space.gif\" width=\"1\" height=\"18\" border=\"0\"></td></tr>";
            }
         }
      }
      
      mysql_free_result($subsData);
   }
   else
   {   
      echo "\n\n<tr><td>
      <a href=\"$PHP_SELF?id=$pageid\" onMouseOver=\"HighLight('btn00$pageid');\" onMouseOut=\"UnHighLight('btn00$pageid');\">
      <img src=\"images/btnoff.gif\" alt=\"$name\" name=\"btn00$pageid\" width=\"23\" height=\"25\" border=\"0\"></a></td>
      <td class=\"btn\">
      <a href=\"$PHP_SELF?id=$pageid\" onMouseOver=\"HighLight('btn00$pageid');\" onMouseOut=\"UnHighLight('btn00$pageid');\">$name</a></td>
      <td><img src=\"images/btnendm.gif\" width=\"10\" height=\"25\" border=\"0\"></td></tr>";
   }
   
   $catsCount++;
}

echo "<tr><td class=\"btnbtm\"><img src=\"images/space.gif\" width=\"23\" height=\"4\" border=\"0\"></td>
<td class=\"btnbtm\"><img src=\"images/space.gif\" width=\"79\" height=\"4\" border=\"0\"></td>
<td><img src=\"images/btnbtmend.gif\" width=\"10\" height=\"4\" border=\"0\"></td></tr>
</table></td>

<td rowspan=\"2\" class=\"bluebg\"><img src=\"images/space.gif\" width=\"7\" height=\"1\" border=\"0\"></td>
<td valign=\"top\" class=\"roundcnr\"><img src=\"images/cnrtop.gif\" width=\"15\" height=\"15\" border=\"0\"></td>
<td colspan=\"3\" rowspan=\"2\" valign=\"top\" class=\"main\">$content";

include "footer.php";

mysql_free_result($pageData);
mysql_free_result($catsData);

mysql_close($connection);
?>


Thanks for your help! :D

Hendrix

User avatar
swirlee
Moderator
Moderator
Posts: 2272
Joined: Sat Jul 05, 2003 1:18 pm
Location: A bunk in the back
Contact:

Re: PHP code review please

Postby swirlee » Wed Sep 24, 2003 12:55 pm

Your code is actually pretty good. I'll take a stab at the bits I see.

Code: Select all

<?php
$PHP_SELF = $_SERVER['PHP_SELF'];
// Why is this necessary? Just refer to $_SERVER['PHP_SELF']

$title = "404 Not Found";
$header = "404 Not Found";
$content = "The server can not find the requested page.";
// This isn't the correct way to return a 404 error -- neither your browser or server will interpret it as such. Since this isn't really a 404 situation (e.g. the file itself exists, just not a particular set of data in the database), you should return a more meaningful error. But if you insist on returning an actual 404, use header() (see the documentation).

while ($pageRow = mysql_fetch_array($pageData))
{
   $title = $pageRow["title"];
   $header = $pageRow["header"];
   $content = $pageRow["content"];
}
// If you only want to fetch one row, you can skip the while loop. In fact, it's wise to check that only one row is returned, as more than one may indicate a problem.

echo "<table border=\"0\" cellspacing=\"0\" cellpadding=\"0\" summary=\"Main table\" width=\"100%\">
<colgroup span=\"4\"><col width=\"112\"><col width=\"7\"><col width=\"15\"><col></colgroup>
<tr><td rowspan=\"2\" valign=\"top\" class=\"bluebg\">
<table border=\"0\" cellspacing=\"0\" cellpadding=\"0\" summary=\"Buttons\">
<colgroup><col width=\"23\"><col width=\"79\"><col width=\"10\"></colgroup>";
// You should exit PHP when outputting a lot of HTML. It'll save you a lot of trouble.

$catsCount = 0;
$catsArray[] = "";
// You just created a blank element in $catsArray. This might cause problems later. If you want to create an empty array (no elements), do this instead:
// $catsArray = array();

while ($catsRow = mysql_fetch_array($catsData)) // this populates the menu with $catsData
{

// As above, for most of the HTML in this section, you should exit PHP instead of using echo, and then re-enter PHP briefly to output values, like so:
?>
<em><?php echo $var; ?></em>

Hendrix
New php-forum User
New php-forum User
Posts: 4
Joined: Wed Sep 24, 2003 11:14 am

Postby Hendrix » Wed Sep 24, 2003 1:32 pm

Hi swirlee,

Thanks for the reply, your suggestions helped quite a bit. I was unable to use your suggestion for the $catsArray[] = ""; being changed to $catsArray = array(); because I end up with undefined offsets for some reason. Would I need to initialize the array somehow before using it?

When exiting and entering php is it faster to do this or is it just better for code readabilty? Thanks again for your input, it has helped a great deal.

Hendrix

User avatar
Redcircle
Moderator
Moderator
Posts: 830
Joined: Tue Jan 21, 2003 10:42 pm
Location: Michigan USA
Contact:

Postby Redcircle » Wed Sep 24, 2003 10:51 pm

Code: Select all

while ($pageRow = mysql_fetch_array($pageData))
{
   $title = $pageRow["title"];
   $header = $pageRow["header"];
   $content = $pageRow["content"];
}


This can only return one result so it is not needed to be within a while statement. you could just use

Code: Select all

if(mysql_num_rows($pageData))
{
$pageData = mysql_query("SELECT title, header, content FROM $table WHERE pageid='$id' limit 1") or die(mysql_error()); //becasue you will only have one result use limit 1 so it will stop searching the db once it finds one result.
$pageRow = mysql_fetch_array($pageData)
$title = $pageRow["title"];
$header = $pageRow["header"];
$content = $pageRow["content"];
}


also using single ' oposed to double quote " will increase execution time. example

Code: Select all

echo '<tr><td class="btnbtm"><img src="images/space.gif" width="23" height="4" border="0"></td>
<td class="btnbtm"><img src="images/space.gif" width="79" height="4" border="0"></td>
<td><img src="images/btnbtmend.gif" width="10" height="4" border="0"></td></tr>
</table></td>

<td rowspan="2" class="bluebg"><img src="images/space.gif" width="7" height="1" border="0"></td>
<td valign="top" class="roundcnr"><img src="images/cnrtop.gif" width="15" height="15" border="0"></td>
<td colspan="3" rowspan="2" valign="top" class="main">'.$content;


it's not only easier to read but there are much fewer \ to type.

User avatar
swirlee
Moderator
Moderator
Posts: 2272
Joined: Sat Jul 05, 2003 1:18 pm
Location: A bunk in the back
Contact:

Postby swirlee » Thu Sep 25, 2003 2:08 pm

Hendrix wrote:I was unable to use your suggestion for the $catsArray[] = ""; being changed to $catsArray = array(); because I end up with undefined offsets for some reason.


Remember that array indexes start at 0, so if you've added two elements to the array, the first will have index 0 and the second will have index 1.

Hendrix wrote:When exiting and entering php is it faster to do this or is it just better for code readabilty?


Both, in fact.

psychomantum
New php-forum User
New php-forum User
Posts: 10
Joined: Sat Dec 07, 2002 8:04 pm

Postby psychomantum » Thu Sep 25, 2003 9:36 pm

I would think you would want to seperate your PHP from your HTML a little more, like using more functions or seperating and including the files in one.

Another thing you could try is ending your PHP tags when you write a lot of HTML, like this:

Code: Select all

<?php

if ($something = avalue)
{
     $code...
     $code...
?>
<htmltags>data</htmltags>
<?php
     $code...
     $code...
}

?>


Just a suggestion. I'm fairly new myself, so there's a lot about the organization and formatting that I sill need to learn.


Return to “PHP coding => General”

Who is online

Users browsing this forum: No registered users and 1 guest