Help with makeing the code beter, faster, and find stupid code lops!!

[Chrysanthus]

Hi

it woulde be great if some one coulde take a look att the code and se if i can make some improvments and if there are any botnecks i the code!!

<?php
$username=$_POST["username"];
$password=$_POST["password"];

include ("../managment/mysqlcondbrr.php");


$mysqlcon = "select * from reg_info where username = '$username' and password = '$password';";
$result = mysql_query($mysqlcon) or die("Query failed");

if (mysql_num_rows($result) == '1') {

print "<p><center><big><big>Welcome $username Please click <a href=\"isloggedin.php\">here to continue to flameline.com</big></big></center></p>";

$_SESSION["username"] = $username;
$_SESSION["password"] = $password;
$_SESSION["validuser"] = "1";
$_SESSION["loggins"] = "1";
}
else
{
$_SESSION["unvaliduser"] = "0";
print "<p><center><big><big>Sorry, incorrect Password $password. or Nickname $username.</big></big></center></p>";
print "<p><center><big>if not a member <a href=\"register.php\">click here to register</big></center></p>";
}

?>

[HalfaBee]

When you post php use the php button just above or put the code in [ PHP] [/ PHP] tags ( without the space.
Like this.

<?php
$username = $_POST["username"];
$password = $_POST["password"];

include ("../managment/mysqlcondbrr.php");

$mysqlcon = "select * from reg_info where username = '$username' and password = '$password';";
$result = mysql_query($mysqlcon) or die("Query failed");

if (mysql_num_rows($result) == '1') {
print "<p><center><big><big>Welcome $username Please click <a href=\"isloggedin.php\">here to continue to flameline.com</big></big></center></p>";

$_SESSION["username"] = $username;
$_SESSION["password"] = $password;
$_SESSION["validuser"] = "1";
$_SESSION["loggins"] = "1";
} else {
$_SESSION["unvaliduser"] = "0";
print "<p><center><big><big>Sorry, incorrect Password $password. or Nickname $username.</big></big></center></p>";
print "<p><center><big>if not a member <a href=\"register.php\">click here to register</big></center></p>";
}

?>


Your code has no loops, unless they are in the include file.

HalfaBee

[shareaweb]

include_once() is marginally faster. Any reason why your selecting all instead of just the columns you need? Just a thought...
Any do you have output bufferring enabled?

[Chrysanthus]

Your code has no loops, unless they are in the include file.

HalfaBee

Nop that's only the db connection !!

///

shareaweb


include_once() is marginally faster. Any reason why your selecting all instead of just the columns you need? Just a thought...

Answer: That's a good question i shoulde use only password and username columns?? :p



Any do you have output bufferring enabled?

Whats tha't what do that Improve

[BuzzLY]

Originally posted by Chrysanthus
$_SESSION["loggins"] = "1";
session_destroy();
if ($_SESSION['loggins'] != "1") {
echo "Oh my God, they killed Kenny!!!";
}

:D:D:D:D

BTW... use single quotes in your $_SESSION array, not double quotes.

[Chrysanthus]

okay but i have one stupid question that i wanna ask what makes the diffrence with use single quotes insted of dubbel ???


PHP:
--------------------------------------------------------------------------------

session_destroy();
if ($_SESSION['loggins'] != "1") {
echo "Oh my God, they killed Kenny!!!";
}

[goldbug]

IIRC, one reason is because PHP evaluates everything in double quotes (which is why you can throw variable names in there), and just treats single-quoted text as a scalar value, throwing it around as-is. Single quotes thusly provides you with a very minor performance increase.

my 2c.

[goldbug]

Also, you're setting a bunch of session variables to 0 or 1.

Is this supposed to indicate a true/false state of some kind? (Are numbers really required?)

If possible consider something like:


$_SESSION['foo'] = TRUE;


then it makes your code a little cleaner:

if ($_SESSION['foo']) {
...
}


Of course, you could still use the if statement above with your current values (0/1 evaluate to false/true).

[HalfaBee]

TRUE & FALSE are just numbers.

It could take longer to put a defined number rather than an explicit number.

But I prefer TRUE/FALSE also. :)


Also I would not use
$_SESSION["unvaliduser"] = "0";

If they are not valid set
$_SESSION["validuser"] = 0;

Halfabee

[Chrysanthus]

okay sow this is the best way! my ide was that give the user a login level per specified user but that can be a later brainstorm.


<?php
$username = $_POST["username"];
$password = $_POST["password"];

include ("../managment/mysqlcondbrr.php");

$mysqlcon = "select * from reg_info where username = '$username' and password = '$password';";
$result = mysql_query($mysqlcon) or die("Query failed");

if (mysql_num_rows($result) == '1') {
print "<p><center><big><big>Welcome $username Please click <a href=\"isloggedin.php\">here to continue to flameline.com</big></big></center></p>";

$_SESSION["username"] = $username;
$_SESSION["password"] = $password;
$_SESSION['validuser'] = TRUE;
$_SESSION["loggins"] = "1";
} else {
$_SESSION['unvaliduser'] = FALSE;
print "<p><center><big><big>Sorry, incorrect Password $password. or Nickname $username.</big></big></center></p>";
print "<p><center><big>if not a member <a href=\"register.php\">click here to register</big></center></p>";
}

?>
BuzzLY

did you mean thate code shoulde look like this

$_SESSION['loggins'] = "1";

[HalfaBee]

There is no need for the 'unvaliduser' session variable.
Just set $_SESSION['validuser']=FALSE;

HalfaBee

[Chrysanthus]

Thats rigtht way do i need that if there are now value in the session file tha's not TRUE The user will be kicked!

[Chrysanthus]

ahha PHP button thanks i wounder how all you user gott color now i know that to!

[rythmic]

Hi!

I think that the logincheck that you use is not safe. It can easily be passed using sql injection where you would just limit your answer to return one line.

You should check the password so that it actually equals the password in the database.. like this example below:


else{
/* Let's validate login */

$given_user = $_POST['username'];
$given_pass = md5($_POST['password']);

$query = "SELECT $tbl_users_uid,$tbl_users_pass,$tbl_users_isb FROM $tbl_users WHERE $tbl_users_uname='$given_user' LIMIT 1";
$result = mysql_query($query,$connection);
if(mysql_num_rows($result) != 0){
$row = mysql_fetch_assoc($result)or die(mysql_error());

/* If password is correct and user is not blocked login should be approved */
if($row["$tbl_users_pass"] == $given_pass && $row["$tbl_users_isb"]!= 1){
$_SESSION['user_name'] = $given_user;
$_SESSION['user_id'] = $row["$tbl_users_uid"];
register_login($row["$tbl_users_uid"]);
unset($_SESSION['failed_attempts']);
header('Location: index_inside.php');
}


It is taken from a login code from one of my pages and as you can see there is little hope of getting through without providing a correct password for an existing user.

cheers :)