Page 1 of 1
Coaching on this code
Posted: Thu Oct 25, 2012 10:15 pm
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" />
Re: Coaching on this code
Posted: Thu Oct 25, 2012 10:35 pm
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']}".
Re: Coaching on this code
Posted: Wed Nov 21, 2012 4:55 pm
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
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
Re: Coaching on this code
Posted: Wed Nov 21, 2012 8:25 pm
by Eric!
Just curious, why aren't you using PDO or at least mysqli?
Re: Coaching on this code
Posted: Thu Nov 22, 2012 3:59 pm
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
Re: Coaching on this code
Posted: Thu Nov 22, 2012 7:36 pm
by Eric!
That's what I get for skimming. I didn't notice the question wasn't yours....