PHP best practices?

[zander1983]

Hi

I've developed a website myself over the last 6 months. I've gotten some investment to develop it further and will now have 2 develoeprs working with me. Its a market place style website so high hits are expected and speed is a must. I was wondering, is my code up to scratch? what exactly are best practices in php? Here's an example of how i get all products on the index page:

 

		$sql = "SELECT si.Name, si.ShopItemID, si.Active, si.InStock, si.DateAdded, si.Price, s.CountyID, si.Url as ItemUrl, s.Url as ShopUrl, si.Approved, ii.Url, ii.Active, ii.Front, ii.Thumb, ii.OrderImage, s.CategoryID, s.ShopID, s.Active ";
		$sql = $sql."FROM shopitem si ";  
		$sql = $sql."Inner Join shop s On (s.ShopID = si.ShopID) ";
		$sql = $sql."left Join itemimage ii on (ii.ShopItemID = si.ShopItemID And ii.OrderImage = 1) Where 1=1 and si.Active = 1 and si.InStock > 0 and s.Active = 1 and si.Approved = 1 ";
		if(!$categoryId==0 and !$categoryId==""){
			$sql = $sql."And CategoryID = ".mysql_real_escape_string($categoryId);	
		}
		$sql .= " Group By si.ShopItemID ";

 

and to display it:

 

			 while ($row = mysql_fetch_array($result)) {
			echo "<div class='shopItems'>";
			echo "<div id='itemIndex'>".$x.".</div>";
				if(!$row['Url']==NULL){
					echo "<div id='centered'><a href='".WEBSITE_DOMAIN."shops/".$row['ShopUrl']."/".$row['ItemUrl']."'><img alt='".htmlspecialchars($row['Name'])."' title='".htmlspecialchars($row['Name'])."' height='135' width='170' src='images/".htmlspecialchars($row['Front'])."'/></a></div>";
				}
				else{
					echo "<div id='centered'><a href='".WEBSITE_DOMAIN."shops/".$row['ShopUrl']."/".$row['ItemUrl']."'><img alt='".htmlspecialchars($row['Name'])."' title='".htmlspecialchars($row['Name'])."' height='135' width='170' src='images/no-img.jpg'/></a></div>";
				}
				echo "<div id='centered'><a href='".WEBSITE_DOMAIN."shops/".$row['ShopUrl']."/".$row['ItemUrl']."'>".htmlspecialchars($row['Name'])."</a></div>";
				echo "<p id='centered'>€".htmlspecialchars($row['Price'])."</p>";
			echo "</div>";

			If($x%4==0){
			echo "<div id='clear'></div>";
			echo "<div id='divider'></div>";

			}
			$x++;
		 }

 

Is there anything wrong with this approach? Is it best practice and/or faster to use object oriented programming? So I would have a ShopItems class, with members which reflect the attributes of the ShopItems table, and call a function of it to run the sql and set the vales of the members. Am I going in the right direction, towards better practices? Or is what I have done fine? I have used the top approach throught the website, but worry that its a bit of an old-fashioned approach...any advice?

 

thanks

Mark

 

[Psycho]

The first code block is simply the building of a query. What will be more important is database normalization, use of indexes, etc. But, I do have a few comments on that one.

 

1. What is the purpose of "1=1" in your WHERE clause:

Where 1=1 and si.Active = 1 

 

2. In this code, there is no need to do TWO comparisons when only one will do the trick using empty() (but there's no need for that - see next comment). Also, you should almost always trim user input. Otherwise an input with just spaces can cause problems.

	if(!$categoryId==0 and !$categoryId==""){
    $sql = $sql."And CategoryID = ".mysql_real_escape_string($categoryId);	
}

 

3. Also on that bit of code, why are you using mysql_real_escape_string() on the category ID? Just convert it to an int. Since 0 is not a valid value to use you can enforce the value to an integer (ensure it is SQL safe) and use that for the validation.

$categoryID = (int) $categoryId;
if($categoryID!=0){
    $sql = $sql."And CategoryID = $categoryID";
}

 

4. Also, I'm not sure, but I think that using $sql = $sql . "new text" would be less efficient than using $sql .= "new text"

 

As for your display code, the one thing I would change are the two lines to display the div with the image. Those two lines are EXACTLY the same except for the image. So instead of rewriting both lines, just define a variable for the difference. This will make the code much more maintainable. If you have to change the output in the future you don't have to change both lines - only one.

if(!$row['Url']==NULL){
    $imageFile = htmlspecialchars($row['Front']);;
}
else{
    $imageFile = "no-img.jpg";
}
echo "<div id='centered'><a href='".WEBSITE_DOMAIN."shops/".$row['ShopUrl']."/".$row['ItemUrl']."'><img alt='".htmlspecialchars($row['Name'])."' title='".htmlspecialchars($row['Name'])."' height='135' width='170' src='images/$imageFile'/></a></div>";

[zander1983]

Hi mjdamato

 

Thanks for your response and you make some interesting points. What is your view on the MVC model? I've been reading this article http://www.phppatterns.com/docs/design/archive/model_view_controller_pattern

 

I have tended to write more procedural code, but the MVC model talks about layers of seperation within code. I dont even use object oriented programming when writing PHP code (I used to in .NET but have gotten out of the habit). The the MVC model and OOP the way to go when writing modern php code? As I expect this site to go live in a few months and have a fairly heavy traffic load, I want to get the structure and style of the code right...

Thanks

Mark

[Psycho]

I agree that separation of functions is a good idea. I don't know that there have ever been claims that it improves performance or not, but for sheer maintainability and future development it makes things much easier.

[zander1983]

ok thanks, im going to keep reading and try to follow these principles:) thanks for help