5
\$\begingroup\$

The script is fully working, but I ask is there a better way I can do this rather than name all of the query variables, $q, $q1 etc, and then use if else statements to execute the code.

<?php 
// open the mysql connection

require_once('mysqli.php');

// successful login variables

$q = $dbc -> prepare("SELECT * FROM accounts WHERE email = ? && password = ? && logcount != ''");
$q -> bind_param('ss', ($_POST['email']), ($_POST['password'])); 
$q -> execute();
$q -> store_result();

// wrong password variables

$q1 = $dbc -> prepare("SELECT * FROM accounts WHERE email = ? && password != ? && logcount != ''");
$q1 -> bind_param('ss', ($_POST['email']), ($_POST['password']));
$q1 -> execute();
$q1 -> store_result();  

// not active variables

$q2 = $dbc -> prepare("SELECT * FROM accounts WHERE email = ? && logcount = ''");
$q2 -> bind_param('s', ($_POST['email']));
$q2 -> execute();
$q2 -> store_result();  

// successful login query

if ($q -> num_rows == 1) {
    $q = $dbc -> prepare("UPDATE accounts SET logcount = logcount+1 WHERE email = ?");
    $q -> bind_param('s', ($_POST['email']));
    $q -> execute();
    $dbc -> close();
    header('location: shack');
    exit();
}

// wrong password query

elseif ($q1 -> num_rows == 1) {
    echo '<p>Your password is incorrect.</p>';
}

// not activated query

elseif ($q2 -> num_rows == 1) {
    echo '<p>Your account is not activated.</p>';
}

// blank email form entry

elseif (empty($_POST['email'])) {
    echo '<p>You did not fill in an email on the login form.</p>';
}

// account not found

else {
    echo '<p>An account could not be found.</p>';
}

// close the mysql connection

$dbc -> close();

?>
\$\endgroup\$

migrated from stackoverflow.com Apr 15 '11 at 19:54

This question came from our site for professional and enthusiast programmers.

5
\$\begingroup\$

Appears you are storing cleartext passwords and doing a 1:1 comparison. This is bad.

You can clean the code up by doing the following and executing 1 query to MySQL:

require_once('mysqli.php');  
// successful login variables  
$q = $dbc -> prepare("SELECT email,dbClearTextPassword,accountActive FROM accounts WHERE email = ?"); 
$q -> bind_param('s', ($_POST['email']));  
$q -> execute(); 
$q -> store_result();

// check to see if you returned any information from your query
if ($q->num_rows == 0) { 
  // email doesn't exist in db .. #fail
  exit;
}

if ($q['accountActive'] != '1') { 
  // email account is not active .. #fail
  exit;
}

// check the password matches 
if ($_POST['clearTextPassword'] == $q['dbClearTextPassword']) { 
  // do some of your fancy code to process the successful login
} else { 
  // do some other fancy code to process a login failure
}

$dbc->close();
\$\endgroup\$
  • \$\begingroup\$ Ye I haven't got round to hashing the passwords yet, just still trying to get the cleanest code possible, I haven't seen this method yet, I will try implementing it and see what happens :) Thanks \$\endgroup\$ – Basic Apr 15 '11 at 12:12
  • \$\begingroup\$ will you donate 10% of your contract earnings to a non-profit organization that supports depressed coders if this answers your question? \$\endgroup\$ – sdolgy Apr 15 '11 at 12:16
  • \$\begingroup\$ Haha this is a non profit website, and it seems that I cannot just use simply $q['value'] as it returns an object error! \$\endgroup\$ – Basic Apr 15 '11 at 12:17
  • 3
    \$\begingroup\$ you need to spend more time reading about how to work with mysqli results. my code was a quick example to show you the concepts of how to clean up your code ... answering your question. it was not meant to be a production example. \$\endgroup\$ – sdolgy Apr 15 '11 at 12:37
  • 2
    \$\begingroup\$ @Basic 1982301 Programmers nodded in disappointment after reading that comment \$\endgroup\$ – Abe Petrillo Apr 15 '11 at 13:12
1
\$\begingroup\$

About efficiency: I suggest do not select data which do not needed.

For example, intead of

SELECT * FROM accounts WHERE email = ? && password = ? && logcount != ''

use

SELECT COUNT(*) FROM accounts WHERE email = ? && password = ? && logcount != ''

Also I fully agree with @sdolgy about not storing password as plain text.

Another point, not about efficiency, but about OOP design and readability: split code to few classes:

  • All SQL queries go to DAO layer
  • All checks move to service layer (which calls DAO-methods and throw exception when error occurs)
  • All other actions leave as is

See also: DAO pattern

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.