PHP code review please

Ask about general coding issues or problems here.

Moderators: egami, macek, gesf

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

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>
Image

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

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: 2257
Joined: Sat Jul 05, 2003 1:18 pm
Location: A bunk in the back
Contact:

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

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

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.

Post Reply
  • Information
  • Who is online

    Users browsing this forum: No registered users and 3 guests