Coaching on this code

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

Post Reply
pinehead
Forum Newbie
Posts: 14
Joined: Sat Oct 13, 2012 11:51 am

Coaching on this code

Post by pinehead »

I wrote some code which applies a db class I wrote. This feel all sorts of wrong and was hoping I could get some guidance on layout and method of writing this better I appreciate all you highly experienced coders and thanks for taking the time to look at mine.

Code: Select all

$db = new MySQL($config['mysql_host'],$config['mysql_user'],$config['mysql_password'],$config['mysql_db']);

	$sql = $db->query("SELECT * FROM users WHERE username='$username'");
	$result = $db->fetcharray($sql);
		$firstName = $result['first_name'];
		$lastName = $result['last_name'];
		$userid = $result['id'];


?>

<div class="left">
  <h3><span class="username"><?php echo $firstName." ".$lastName;?></span> Lists</h3>
  </div>
<div class="right">
  <h1>Completed lists</h1>
  <ul style="padding-left:10px;">
  <?php
  //List Competed list
	$sql = $db->query("select * from list_status where user_id = $userid AND completed = 1");
	while($result = $db->fetcharray($sql)) {
		$list_id = $result['list_id'];
		$sql1 = $db->query("select * from lists where id = $list_id");
		$result1 = $db->fetcharray($sql1);
		echo "<li>".$list_id = $result1['list_name']."</li>";

						
				
	}
?>
   	
   </ul>

</div>

<br class="clear" />
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Coaching on this code

Post by Christopher »

The query inside at loop is the main thing that stands out. If you can do a single query by JOINing lists and list_status that would be preferable. A minor thing, but why do you assign array elements to scalar variables for everything? Instead of "select * from lists where id = $list_id" you can just do "select * from lists where id = {$result['list_id']}".
(#10850)
User avatar
mecha_godzilla
Forum Contributor
Posts: 375
Joined: Wed Apr 14, 2010 4:45 pm
Location: UK

Re: Coaching on this code

Post by mecha_godzilla »

Hi,

I'd agree with the point about using JOIN in your query - this is the kind of thing that will matter when you're ready to start developing high-traffic applications. Including a JOIN in your query can obviously make things more critical because if the value you're JOINing doesn't exist for some reason then the query will fail.

I'll also just add that it's a bit redundant to assign values from your query into variables if you can access them directly from within the DB object and you're not going to change them in any way - compare

Code: Select all

$db = new MySQL($config['mysql_host'],$config['mysql_user'],$config['mysql_password'],$config['mysql_db']);

        $sql = $db->query("SELECT * FROM users WHERE username='$username'");
        $result = $db->fetcharray($sql);
                $firstName = $result['first_name'];
                $lastName = $result['last_name'];
                $userid = $result['id'];
?>

<div class="left">
  <h3><span class="username"><?php echo $firstName." ".$lastName;?></span> Lists</h3>
  </div>
with

Code: Select all

$db = new MySQL($config['mysql_host'],$config['mysql_user'],$config['mysql_password'],$config['mysql_db']);

$sql = $db->query("SELECT first_name,last_name,id AS user_id FROM users WHERE username='$username'");
$result = $db->fetcharray($sql);

?>

<div class="left">
<h3><span class="username"><?php echo $result['first_name']." ".$result['last_name'];?></span> Lists</h3>
</div>
What I do in my DB handler is include a function that lets me change the value retrieved by the result if (for example) I need to format the name in some way or pad an ID:

Code: Select all

class DB {
	
	public $number_of_results;
	public $result_array = array();
	public $row;
	
	function __construct($sql,$conn) {
		
		$this->result = mysql_query($sql,$conn) or die(mysql_error());
		
		$this->number_of_results = mysql_num_rows($this->result);
			
		for ($i = 0; $this->result_array[$i] = mysql_fetch_assoc($this->result); $i++);
			
		array_pop($this->result_array);
			
		$this->row = 0;
			
   	}
	
	function set_val($key,$value) {
		$this->result_array[$this->row][$key] = $value;
	}
	
   	function val($key) {
   		return $this->result_array[$this->row][$key];
   	}
   	
   	function reset_row() {
   		$this->row = 0;
   	}
   	
   	function get_row() {
   		return $this->result_array[$this->row];
   	}
		
}

// Example usage
$db = new DB("SELECT first_name, surname FROM tbl_users WHERE user_id = '$user_id' LIMIT 1",$this->get_conn());

if ($db->number_of_results == 1) {
	echo $db->val('first_name');
}
Please note that this is not exactly AAA code, but it does work :mrgreen:

On a final point, specifying the values individually rather than just using SELECT * is a good "aide-memoire" in your script to remind you of what data is supposed to be being retrieved, plus it's good design practice to only retrieve the values your script needs - your script will inadvertently retrieve the user's password and other sensitive data if you just use the wildcard.

HTH,

Mecha Godzilla
Eric!
DevNet Resident
Posts: 1146
Joined: Sun Jun 14, 2009 3:13 pm

Re: Coaching on this code

Post by Eric! »

Just curious, why aren't you using PDO or at least mysqli?
User avatar
mecha_godzilla
Forum Contributor
Posts: 375
Joined: Wed Apr 14, 2010 4:45 pm
Location: UK

Re: Coaching on this code

Post by mecha_godzilla »

I was trying to demonstrate some basic OOP concepts using code that I thought pinehead would already be familiar with - it can easily be rewritten to use mysqli or whatever else they want to use.

M_G
Eric!
DevNet Resident
Posts: 1146
Joined: Sun Jun 14, 2009 3:13 pm

Re: Coaching on this code

Post by Eric! »

That's what I get for skimming. I didn't notice the question wasn't yours....
Post Reply