Basic Fetch Database Records PHP Script

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

Post Reply
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Basic Fetch Database Records PHP Script

Post by Christopher »

I wrote this really quickly for another thread and Daedalus- noted that it might be good for Snippets. I figured that a round here in Critique would probably make it more useful and see if it was worthy of Snippets.

The script was my attempt to write what a minimum a PHP database script ought to look like. Some goals were: no die(), clear logic, thorough error checking, separation of connection and response (reporting an error is also a response), and basic security. I should also show good programming habits and practices.

I have some questions:
- should it be in three parts: connection, query/fetch, and display data? (so we could show fetching one or may rows for example)
- should it be database independent in some way or does that complicate it too much?
- should things be put in functions so they are more reusable (e.g. connect() and query(), fetch())

Code: Select all

<?php
// initialize variables
$errmsg = '';
$row = array();

// Connect to server
$con = mysql_connect("localhost","don't_post_usernames","don't_post_passwords");

if (mysql_errno()) {
     $errmsg = 'Could not connect:' . mysql_error();
} else {

     // Select database.
     mysql_select_db("database_name");

     if (mysql_errno()) {
          $errmsg = 'Could not select:' . mysql_error();
     } else {
          // Setup and do query

          // filter untrusted post variable
          $username = preg_replace('/[^a-zA-Z0-9]/', '', $_POST['username']);

          // escape untrusted post variable
          $username = mysql_real_escape_string($username);     // changed - thanks Mordred

          $sql1 = "SELECT * FROM user WHERE username='$username'";
          $result1 = mysql_query($sql1);

          if (mysql_errno()) {
               $errmsg = 'Query failed:' . mysql_error();
          } else {
               $row = mysql_fetch_assoc($result);
          }
     }
}

// now the response
if ($errmsg) {
     // error goes here
     echo "Error: $errmsg<br/>";
} else {
     // success goes here
     dump($row);
}

// handy for debugging
function dump($value) {
     echo '<pre>' . print_r($value, 1) . '</pre>';
}
Last edited by Christopher on Thu Jan 18, 2007 2:23 pm, edited 1 time in total.
(#10850)
User avatar
daedalus__
DevNet Resident
Posts: 1925
Joined: Thu Feb 09, 2006 4:52 pm

Post by daedalus__ »

New users normally only deal with MySQL, as far as I know, so I do not think it needs db abstraction. I do think that it should be chopped up into functions, though. Perhaps connect(), printError(), selectDb(), query(), disconnect() with a simple filter function or two to encourage security.



+1 for snippets!

EDIT:

I did bits and pieces of this through-out the course of the day. It might not even work rofl, I'll clean it up a bit when I get home.

Code: Select all

$link = '';
$res = '';
$err_msg = '';

function connect($host = 'localhost', $user = 'root', $pass = '')
{
	$link = mysql_connect($host, $user, $pass);
	if (! $link)
	{
		$err_msg = mysql_error();
		return false;
	} else return $link;
}

function disconnect()
{
	if (is_resource($link))
	{
		mysql_close($link);
	}
	else
	{
		mysql_close();
	}
}

function select_db($name)
{
	$bool = mysql_select_db($name);
	if (! $bool)
	{
		$err_msg = mysql_error();
		return false;
	} else return true;
}

function query($sql)
{
	$res = mysql_query($sql);
	if ($res != false)
	{
		if (is_resource($res))
		{
			if (rows($res) > 0)
			{
				return $res;
			}
			else
			{
				return false;
			}
		}
		else
		{
			return true;
		}
	}
	else
	{
		$err_msg = mysql_error();
		return false;
	}
}

function rows($res)
{
	$num = mysql_num_rows($res);
	if ($num != false)
	{
		return $num;
	}
	else
	{
		return mysql_affected_rows($res);
	}
}

function report_error($value)
{
	echo $err_msg.'<br />';
	echo '<pre>' . var_dump($value, 1) . '</pre>';
}

function filter_alphanumeric($string)
{
	return preg_replace('/[^a-zA-Z0-9]/', '', $string);
}


// connect
$link = connect('localhost', 'user', 'pass');
select_db('test');
$res = query('SELECT * FROM testycles');
disconnect();
if ($res != false)
{
	// do your thang, baby
}
else
{
	report_error();
}
Last edited by daedalus__ on Thu Jan 18, 2007 6:49 pm, edited 1 time in total.
User avatar
Nathaniel
Forum Contributor
Posts: 396
Joined: Wed Aug 31, 2005 5:58 pm
Location: Arkansas, USA

Post by Nathaniel »

In Code Complete, Steve Mcconnell says, if I remember correctly, that programming logic should have the wanted action first, and unexpected actions in the elses.

So that would make...

Code: Select all

if (mysql_errno()) {
     $errmsg = 'Could not connect:' . mysql_error();
} else { 
into

Code: Select all

if ( !mysql_errno() ) {
    // connection succeeded
} else { 
    $errmsg = 'Could not connect:' . mysql_error();
}
Perhaps a negligible critique, but as far as understanding the code goes, the latter method makes for an easy read of how the logic works in a successful case.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

The 'filter' step is meaningless, because the 'escape' step uses $_POST again.

You neglect the case where the SQL query succeeds, but returns zero rows. I would also add backticks to the query. Another good practice is to SELECT only the required fields, and to LIMIT 1, since you only use the first row from the result.

I don't think that outputting mysql_error() to the user is a good practice. It belongs to the error log, not to the output.

mysql_free_result() might be nice, although it's generally not needed, this is supposed to presend good practices ;)

Dividing the logical steps in separate funcitons is a good idea. Just don't turn them into a pseudo-db-abstraction-layer.

Also, it would be better if the sample is not about usernames, as not to tempt newbies into using it for their login scripts (which usually require one or two additional checks and features).

var_dump() may give more useful info than print_r(), so I'd use it instead.
User avatar
daedalus__
DevNet Resident
Posts: 1925
Joined: Thu Feb 09, 2006 4:52 pm

Post by daedalus__ »

Mordred wrote:I don't think that outputting mysql_error() to the user is a good practice. It belongs to the error log, not to the output.
I feel that most newbies are going to be outputting this information, with their mind far, far away from an error log, making it acceptable to use this method in the example.
Mordred wrote:mysql_free_result() might be nice, although it's generally not needed, this is supposed to presend good practices ;)
I'm still thinking about the target group for this script. I do not think that anyone who uses this is going to have a site that is big enough to need to free the results although I do agree it presents an example of good practice. +1 for a free() function.
Mordred wrote:Also, it would be better if the sample is not about usernames, as not to tempt newbies into using it for their login scripts (which usually require one or two additional checks and features).
I disagree, it is likely that this code will be better than whatever the newbie has written, at least it will by the time it is finished.
Mordred wrote:var_dump() may give more useful info than print_r(), so I'd use it instead.
+1
Nathaniel wrote:

Code: Select all

PHP (Brief):
if ( !mysql_errno() ) {
    // connection succeeded
} else {
    $errmsg = 'Could not connect:' . mysql_error();
}
+1


arborint, if you would be okay with it, I would like to help with this. Would you like to collaborate through pm?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

Daedalus- wrote:arborint, if you would be okay with it, I would like to help with this. Would you like to collaborate through pm?
I agree with your comments and those above. I am pretty busy right now ... why don't you have a go at taking the code to the next step as discussed. My only input would be that if it gets longer that maybe twice what I posted then it is too complicated.
(#10850)
User avatar
daedalus__
DevNet Resident
Posts: 1925
Joined: Thu Feb 09, 2006 4:52 pm

Post by daedalus__ »

I think it needs to be kept as simple as possible, as well.
Post Reply