coding best practise

[knobby2k]

Hi,

 

I am looking to pick your brains on coding best practises.

 

I currently authenticate a user and then check that a user is found; only if a user is found does it run the rest of the code for that page (maybe a users account section). Please see below for an example.

 

// search for user
$query = mysql_query("SELECT * FROM users WHERE username='$username' AND password='$password' ");
$rows=@mysql_num_rows($query);
$result = mysql_fetch_array($query);

if(!$rows)
{
echo "no user found";
}
else
{
echo "user found";

//include the rest of the pages coding. This way if the user doesn't exist nothing relevant to that user is displayed.

}

 

...so what i want to know is... am i doing it correctly??

 

Cheers

[per1os]

Don't suppress errors (@ sign). This makes debugging a bitch. Why do you need rows, if you are pulling the information needed out anyways? Just an extra step, not needed.

 

// make sure you mysql_real_escape_string  $username, if it is coming from an untrusted source.
$query = mysql_query("SELECT count(*) as cnt FROM users WHERE username = '$username'") or trigger_error('MySQL Query Failed: ' . mysql_error());

// If the query is bad no need to fetch anything. 
if (!empty($query)) {
    $result = mysql_fetch_assoc($query);
    if (!empty($result) && $result['cnt'] == 1) {
        // Yep a user.
   }else {
       // Nope.
   }
}else {
    // no info returned, error
}

 

Should get you started.

 

// Side Note

It is practice, not practise

[redixx]

Don't suppress errors (@ sign). This makes debugging a bitch.

 

And to add to this, you should be developing with ALL errors displayed, no exceptions. Put this at the top of your code:

error_reporting(-1);

 

If you get errors or notices, fix them. Never suppress errors, always code to expect and handle them.

[AbraCadaver]

A lot of things are preference, however you have an issue, you call mysql_fetch_array() even if there are no rows!  I prefer not to embed the bulk of my page in an if.  Just die or something.  Also, you're only looking for one user.  Depends on the rest of your page, but just an example:

 

// search for user
$result = mysql_query("SELECT * FROM users WHERE username='$username' LIMIT 1");

if(!mysql_num_rows($result)) {
die('No user found');
}
//include the rest of the pages coding. This way if the user doesn't exist nothing relevant to that user is displayed.

 

This is a lot on many pages, so you can simply add this to a is_auth.php page that you include or you can include a page with a lot of functions and this could be a function called is_auth() or something.  Just some ideas.

[Maq]

// Side Note

It is practice, not practise

Not if you spell it like this @practise.

[AbraCadaver]

Don't suppress errors (@ sign). This makes debugging a bitch. Why do you need rows, if you are pulling the information needed out anyways? Just an extra step, not needed.

 

// make sure you mysql_real_escape_string  $username, if it is coming from an untrusted source.
$query = mysql_query("SELECT count(*) as cnt FROM users WHERE username = '$username'") or trigger_error('MySQL Query Failed: ' . mysql_error());

// If the query is bad no need to fetch anything. 
if (!empty($query)) {
    $result = mysql_fetch_assoc($query);
    if (!empty($result) && $result['cnt'] == 1) {
        // Yep a user.
   }else {
       // Nope.
   }
}else {
    // no info returned, error
}

 

Should get you started.

 

// Side Note

It is practice, not practise

 

The return of the query will NOT be empty unless false for an error is returned.  If there is NO error, but there are also NO rows because the user was NOT found, it will NOT be empty.

 

//side not:  not if you're speaking English opposed to American:  http://dictionary.cambridge.org/dictionary/british/practise_3

[redixx]

I prefer not to embed the bulk of my page in an if.  Just die or something.

 

Sometimes die() or exit() will break the flow of your application if there are necessary things beneath it. For example, maybe only half of your template will load, etc.

 

Don't suppress errors (@ sign). This makes debugging a bitch. Why do you need rows, if you are pulling the information needed out anyways? Just an extra step, not needed.

 

// make sure you mysql_real_escape_string  $username, if it is coming from an untrusted source.
$query = mysql_query("SELECT count(*) as cnt FROM users WHERE username = '$username'") or trigger_error('MySQL Query Failed: ' . mysql_error());

// If the query is bad no need to fetch anything. 
if (!empty($query)) {
    $result = mysql_fetch_assoc($query);
    if (!empty($result) && $result['cnt'] == 1) {
        // Yep a user.
   }else {
       // Nope.
   }
}else {
    // no info returned, error
}

 

Should get you started.

 

// Side Note

It is practice, not practise

 

The return of the query will NOT be empty unless false for an error is returned.  If there is NO error, but there are also NO rows because the user was NOT found, it will NOT be empty.

 

But if the query DOES have an error, there is no point continuing...you'll just make more errors.

[AbraCadaver]

I prefer not to embed the bulk of my page in an if.  Just die or something.

 

Sometimes die() or exit() will break the flow of your application if there are necessary things beneath it. For example, maybe only half of your template will load, etc.

 

Yes, that's why I said, "I prefer" and "Depends on the rest of your page"

 

The return of the query will NOT be empty unless false for an error is returned.  If there is NO error, but there are also NO rows because the user was NOT found, it will NOT be empty.

 

But if the query DOES have an error, there is no point continuing...you'll just make more errors.

 

Yes, but there is a difference between an error in the query and no error but no rows were returned.  One is an error in the application and one means that the user is not authorized to view the page and they should be handled differently.

[per1os]

I prefer to use the trigger_error and if combinations to have "proper" error handling, as appose to a blatant and retarded, yes retarded DIE statement. The trigger_error, allows me to log an error in my error log, so I can look at it later if necessary on Production, and shows me the error on development. Using the if statements, I can pass that information along to an error log of my own if I want with different dumps of information, IE what caused the error. This is also handy in finding people trying to exploit your code and what not.

 

Die has it's purposes,  but in a production app, one that may be resold or should be professional, DIE does not have a purpose in this instance. Handle your errors properly and not with a stupid ass die statement. This way you can handle them right and not just have a blank white page, you can actually make it look like your site and give more information then the one liner.

 

So yea, before you give out bad advice, think about what you are typing.

 

@ the spelling of practise, oh well. I am American, so that is correct for me either or. 

 

As to the error in the query and no rows returned, you will see in my code example, both cases are handled. If there is an error in the query, $query will be false, which is handled with the first if statement. The second inner if statement, checks if there was a row returned, and if it was allows you to do something, if not, it allows you to handle that properly as well. So look at my code before you start to diss it and make false statements about it.

[redixx]

The return of the query will NOT be empty unless false for an error is returned.  If there is NO error, but there are also NO rows because the user was NOT found, it will NOT be empty.

 

But if the query DOES have an error, there is no point continuing...you'll just make more errors.

 

Yes, but there is a difference between an error in the query and no error but no rows were returned.  One is an error in the application and one means that the user is not authorized to view the page and they should be handled differently.

 

I'm not sure what you're getting at; the purpose isn't to see if no rows were returned but rather if there is an error with the query.

[knobby2k]

hey, well great advice... although i didn't mean to start a fight with my spelling! 

 

I couldn't go with die because as mentioned the rest of my page would not display.

 

I do need to start handling my errors more efficiently so I will work on that and the advice given is very helpful.

 

Good to see the different ways that people do things!!

 

 

[knobby2k]

Don't suppress errors (@ sign). This makes debugging a bitch. Why do you need rows, if you are pulling the information needed out anyways? Just an extra step, not needed.

 

// make sure you mysql_real_escape_string  $username, if it is coming from an untrusted source.
$query = mysql_query("SELECT count(*) as cnt FROM users WHERE username = '$username'") or trigger_error('MySQL Query Failed: ' . mysql_error());

// If the query is bad no need to fetch anything. 
if (!empty($query)) {
    $result = mysql_fetch_assoc($query);
    if (!empty($result) && $result['cnt'] == 1) {
        // Yep a user. 
        echo "username taken";
   }else {
       // Nope.
       echo "username available";
   }
}else {
    // no info returned, error
}

 

Should get you started.

 

 

I'm having trouble with this. I'm checking to see if a username exists in my table using the method above. Basically checking to see if the username entered is taken or not. However it is always returning the result for 'username taken' even when I enter a username that is not in the table. Any ideas?

 

thanks

[redixx]

Don't suppress errors (@ sign). This makes debugging a bitch. Why do you need rows, if you are pulling the information needed out anyways? Just an extra step, not needed.

 

// make sure you mysql_real_escape_string  $username, if it is coming from an untrusted source.
$query = mysql_query("SELECT count(*) as cnt FROM users WHERE username = '$username'") or trigger_error('MySQL Query Failed: ' . mysql_error());

// If the query is bad no need to fetch anything. 
if (!empty($query)) {
    $result = mysql_fetch_assoc($query);
    if (!empty($result) && $result['cnt'] == 1) {
        // Yep a user. 
        echo "username taken";
   }else {
       // Nope.
       echo "username available";
   }
}else {
    // no info returned, error
}

 

Should get you started.

 

 

I'm having trouble with this. I'm checking to see if a username exists in my table using the method above. Basically checking to see if the username entered is taken or not. However it is always returning the result for 'username taken' even when I enter a username that is not in the table. Any ideas?

 

thanks

 

Hmm, it works for me. Make sure $username is what you think it is.

[per1os]

@knobby2k, I have no clue. I just ran that exact code on my test setup, and it worked just fine returning the right information. So yea, there is either that username in the database, or something else is going on. Where is `$username` coming from?

[jcbones]

If there is more than 1 matching username in the database, it will echo 'username available', since the script checks to see if there is exactly 1 username returned.  Might want to change that:

$result['cnt'] == 1 
//to
$result['cnt'] > 0

[redixx]

If there is more than 1 matching username in the database, it will echo 'username available', since the script checks to see if there is exactly 1 username returned.  Might want to change that:

$result['cnt'] == 1 
//to
$result['cnt'] > 0

 

True, but technically there shouldn't ever be more than one username if you used this script.

[knobby2k]

Hi Guys,

 

I was pressing the back button and entering a different username, submitting that to the query and it was always finding a user even if the username was not in my database.

 

Very random but I then reloaded by browser, tried again and now it works every time without fail.

 

Just want to say i really appreciate your help as it's great for people like me that can make code work... but not necessarily the correct or best way of doing it!

 

Cheers