Is it time to move away from procedural?

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Is it time to move away from procedural?

Post by psurrena »

This page pulls a specific project from the databse

Code: Select all

<?php
	# * WORK item * #
	
	$pid=$_GET['id'];
	require_once(DB);
	
	$query="SELECT P.project_id, P.project_name, P.project_des, P.category_id, P.location_id, C.category_full FROM project P, category C WHERE project_id='$pid' AND P.category_id=C.category_id";
	$result=mysql_query($query) OR DIE (mysql_error());
	$row=mysql_fetch_assoc($result);
	
	//Save for Later!
	$location=$row['location_id'];
	
	if (mysql_num_rows($result)){
		echo "<h3>".$row['project_name']."</h3>\n";
		echo "<p>".$row['project_des']."</p>\n";
		
		/*Check for Images */
		$path="images/work/".$row['project_id'].'/';
		$dir=opendir($path);
		while($name=readdir($dir)){
			if (preg_match("/t+([0-9]+\.jpg)$/",$name,$sp)){
				$nameb=ereg_replace('t','', $name);
				echo '<a href="'.BASE_URL.$path.$sp[1].'" rel="lightbox" title="'.$row['project_name'].'">';
				echo '<img class="work" src="'.BASE_URL.$path.$name.'" alt="'.$row['project_name'].'" width="140" />';
				echo "</a>&nbsp;\r";
			}
		}
		
		//PDF
		echo '<p><img src="'.BASE_URL.'images/pdf-icon.gif" /> <a class="workU" href="'.BASE_URL.'modules/pdf.php?pid='.$pid.'"> View Project Brief.</a></p>';
		
		//DEATAILS Section
		echo "<p><strong>Details</strong><br />\n";
		echo "Category: ";
		
		//Categories the project is listed under
		$a=explode(' ',$row['category_id']);
		foreach ($a as $key=>$value){
			$query="SELECT category_full, category_id FROM category where $value=category_id";
			$result=mysql_query($query) OR DIE (mysql_error());
			$row=mysql_fetch_assoc($result);
			$cat=$row['category_id'];
			if(mysql_num_rows($result)){
				echo '<a class="workU" href="'.BASE_URL.'category-'.$cat.'.html" class="underline">'.$row['category_full']."</a>\n";
			} else {
				echo "The project is not categorized.";
			}
		}
		echo "<br />\n";
		// Locaton of the project
		echo "Location: ";
		$query="SELECT location_id, location_ct, location_st, location_st_full, location_co FROM location WHERE location_id='$location'";
		$result=mysql_query($query) OR DIE (mysql_error());
		$row=mysql_fetch_assoc($result);
		$city=ereg_replace(' ','-', trim($row['location_ct']));
		$state=ereg_replace(' ','-', trim($row['location_st']));
		echo '<a class="workU" href="'.BASE_URL.'location-'.$city.'-'.$row['location_id'].'.html">'.$row['location_ct']."</a>";
		
		//IN PROGRESS
		if($row['location_st']){
			echo ', <a class="workU" href="'.BASE_URL.'location-'.$state.'-'.$row['location_id'].'.html">'.$row['location_st_full']."</a>\n";
		}
		
		if ($row['location_co'] != "USA"){
			echo ', <a class="workU" href="'.BASE_URL.'work.php?mode=loc&co='.$row['location_id'].'">'.$row['location_co'].'</a>';
		}
		echo "</p>\r";
		
		$query="SELECT project_id, people_fname, people_lname, people_mem, people_id FROM people WHERE project_id LIKE '%$pid%'";
		$result=mysql_query($query) or die (mysql_error());		
		if(!mysql_num_rows($result)){
			echo'';
		}else{
			echo "<p><strong>People</strong><br />";
			while($row=mysql_fetch_assoc($result)){
			$membership=ereg_replace(' ',', ', $row['people_mem']);	
				echo '<a class="workU" href="'.BASE_URL.'bio-'.$row['people_fname'].'-'.$row['people_lname'].'-'.$row['people_id'].'.html">'.$row['people_fname'].' '.$row['people_lname'].', '.$membership.'</a><br />';
			}
			echo "</p>\n";
		}

		echo '<p><strong><a class="work" href="'.BASE_URL.'category-'.$cat.'.html">< Project List</a></strong></p>';

	} else {
		echo 'There are no additional details available.';
	}
?>
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

It doesn't strictly require that you move away from procedural. You certainly need to learn how to keep your presentation separate from your business logic though.

OOP helps in that respect, but it's not a requirement.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

What do yo mean by presentation separate from business logic?
sebastianrs
Forum Newbie
Posts: 1
Joined: Thu Jul 26, 2007 8:55 am

Post by sebastianrs »

That simply means that you should separate your presentation elements like the HTML output from any behavioral elements--the code logic. Retrieve the information using a separate PHP file called something.inc.php or similar.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

Don't take that too literally, however. If something needs to be displayed, it needs to be displayed, but you shouldn't have any particularly long statements or any need for deep nested if-statements in your display file. Clean code means less clutter and more organization, and the cleaner your code, the easier it is to modify.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

So the code below really does not need to be displayed / could easily be an include. Is that what you're saying? Also, would this be a good piece of code to start learning oop with? I feel it's pretty straight forward.

Thanks for all the help!

Code: Select all

/*Check for Images */
$path="images/work/".$row['project_id'].'/';
$dir=opendir($path);
while($name=readdir($dir)){
		if (preg_match("/t+([0-9]+\.jpg)$/",$name,$sp)){
				$nameb=ereg_replace('t','', $name);
				echo '<a href="'.BASE_URL.$path.$sp[1].'" rel="lightbox" title="'.$row['project_name'].'">';
				echo '<img class="work" src="'.BASE_URL.$path.$name.'" alt="'.$row['project_name'].'" width="140" />';
				echo "</a>&nbsp;\r";
		}
}
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

That would be an.. okay place to start. But then there's issues of what class(es) it would become. One for handling your images, one for handling your files, or one of each working together?
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

Ideally it would be one of each working together.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

psurrena wrote:Ideally it would be one of each working together.
Any idea how much control you'd give to either? The issue I was addressing was that either of them could be structured to be able to handle that entire function, so either you choose one, or somehow try to split it up.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

If I understand correctly, almost total control. The only variable would be the path.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

psurrena wrote:If I understand correctly, almost total control. The only variable would be the path.
I mean that particular function if broken up into being performed by two classes would need each class to perform certain parts of it. Which one would do more? Would the image class only define the image path and filename and the file class handle the rest, or would the file class only handle retrieving the file and the image class do the rest?
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

For my purposes now, naming of the file is specific as to control the order. So the images are 1.jpg, 1t.jpg, 2.jpg 2t.jpg, etc. What makes sense to me is, the file class would only to the retrieving and image would do the rest.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

psurrena wrote:For my purposes now, naming of the file is specific as to control the order. So the images are 1.jpg, 1t.jpg, 2.jpg 2t.jpg, etc. What makes sense to me is, the file class would only to the retrieving and image would do the rest.
Makes sense. I'd get started on it. And try to leave some elbow room for later on, as you'll likely re-factor as you discover new things that you'll want to be able to do.
User avatar
psurrena
Forum Contributor
Posts: 355
Joined: Thu Nov 10, 2005 12:31 pm
Location: Broolyn, NY

Post by psurrena »

One last question, and I don't mean to drag this on. Why would I use two classes over one class with two functions?
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

psurrena wrote:One last question, and I don't mean to drag this on. Why would I use two classes over one class with two functions?
No one said you have to use two classes, you said you'd use two.

The whole purpose of classes is a separation of duties, so to speak. Every object is a worker on an assembly line to serve your data, and each of them has a task to perform and the section to handle.
Post Reply