Return to Answer

First off, when posting a question, try to give it some natural IDE formatting if you can. this improves readability and you're more likely to get more/better answers!

##Code##

Technically, you can call

$_SERVER["REQUEST_METHOD"] == "POST"

however, my personal preference is just

isset($_POST)

I find that's it's easier to understand and is more intuitive.

This first two conditions encompass basically the entire function. Why not just reduce them to be negative and return false?

Such as

if (!isset($_POST['action']) or $_POST['action'] != 'login') {
 return FALSE;
}

It's reduces unnecessary nesting. Apply the same for if ($_SERVER["REQUEST_METHOD"] == "POST").

if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '')

could be simplified to

if (!isset($_POST['user'], $_POST['password']) or empty($_POST['user']) or empty($_POST['password']))

It's just easier to read.

In your minimum time check between form submissions, you've added an else, but because the initial condition would return false, an else is completely unnecessary. It also nests your code one more layer, which isn't too great if it can be avoided.

$user = test_input($_POST['user']);

The variable here is very ambiguous. I suggest calling something more appropriate such as $username or something that describes what the value is.

hash('sha512', $passtemp . $user);

This not as secure as it could be! First off, if your PHP version supports it, I suggest you use bcrypt instead of hash. It take scares of the salting for you, so that you don't use bad salts such as you have. The salt should typically be a long string composed of random characters. Not just a username.

if ((isset($_SESSION['session_id'])) and (checkSessionID($user)))

Why so many parentheses! It just makes things unclear!

$_SESSION['password'] = $password;

It's generally bad practice to store a password in any place but the database.

This condition returns true, again the else after this is not needed. Remove it to reduce nesting.

Overall, you're on your way, but there's a lot that could be improved.

• Your code is strictly procedural which is becoming less and less neccessary.

• Your code is reinventing something that doesn't need to be made again.

• Your code is hard to read and understand. Partly because of all the nesting you have (which is easy to avoid with an OO design). Here's an article about arrow code written by StackExchange's very own Jeff Atwood.

There're hundreds of frameworks and community run scripts that do the logging in/out for you. Don't think that using one of these is limiting you or making your coding skills worse.

If you insist on using this code, it will suffice, but it will be hard to maintain and expand...

First off, when posting a question, try to give it some natural IDE formatting if you can. this improves readability and you're more likely to get more/better answers!

Code

Technically, you can call

$_SERVER["REQUEST_METHOD"] == "POST"

however, my personal preference is just

isset($_POST)

I find that's it's easier to understand and is more intuitive.

This first two conditions encompass basically the entire function. Why not just reduce them to be negative and return false?

Such as

if (!isset($_POST['action']) or $_POST['action'] != 'login') {
 return FALSE;
}

It's reduces unnecessary nesting. Apply the same for if ($_SERVER["REQUEST_METHOD"] == "POST").

if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '')

could be simplified to

if (!isset($_POST['user'], $_POST['password']) or empty($_POST['user']) or empty($_POST['password']))

It's just easier to read.

In your minimum time check between form submissions, you've added an else, but because the initial condition would return false, an else is completely unnecessary. It also nests your code one more layer, which isn't too great if it can be avoided.

$user = test_input($_POST['user']);

The variable here is very ambiguous. I suggest calling something more appropriate such as $username or something that describes what the value is.

hash('sha512', $passtemp . $user);

This not as secure as it could be! First off, if your PHP version supports it, I suggest you use bcrypt instead of hash. It take scares of the salting for you, so that you don't use bad salts such as you have. The salt should typically be a long string composed of random characters. Not just a username.

if ((isset($_SESSION['session_id'])) and (checkSessionID($user)))

Why so many parentheses! It just makes things unclear!

$_SESSION['password'] = $password;

It's generally bad practice to store a password in any place but the database.

This condition returns true, again the else after this is not needed. Remove it to reduce nesting.

Overall, you're on your way, but there's a lot that could be improved.

• Your code is strictly procedural which is becoming less and less neccessary.

• Your code is reinventing something that doesn't need to be made again.

• Your code is hard to read and understand. Partly because of all the nesting you have (which is easy to avoid with an OO design). Here's an article about arrow code written by StackExchange's very own Jeff Atwood.

There're hundreds of frameworks and community run scripts that do the logging in/out for you. Don't think that using one of these is limiting you or making your coding skills worse.

If you insist on using this code, it will suffice, but it will be hard to maintain and expand...

First off, when posting a question, try to give it some natural IDE formatting if you can. this improves readability and you're more likely to get more/better answers!

##Code##

Technically, you can call

$_SERVER["REQUEST_METHOD"] == "POST"

however, my personal preference is just

isset($_POST)

I find that's it's easier to understand and is more intuitive.

This first two conditions encompass basically the entire function. Why not just reduce them to be negative and return false?

Such as

if (!isset($_POST['action']) or $_POST['action'] != 'login') {
 return FALSE;
}

It's reduces unnecessary nesting. Apply the same for if ($_SERVER["REQUEST_METHOD"] == "POST").

if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '')

could be simplified to

if (!isset($_POST['user'], $_POST['password']) or empty($_POST['user']) or empty($_POST['password']))

It's just easier to read.

In your minimum time check between form submissions, you've added an else, but because the initial condition would return false, an else is completely unnecessary. It also nests your code one more layer, which isn't too great if it can be avoided.

$user = test_input($_POST['user']);

The variable here is very ambiguous. I suggest calling something more appropriate such as $username or something that describes what the value is.

hash('sha512', $passtemp . $user);

This not as secure as it could be! First off, if your PHP version supports it, I suggest you use bcrypt instead of hash. It take scares of the salting for you, so that you don't use bad salts such as you have. The salt should typically be a long string composed of random characters. Not just a username.

if ((isset($_SESSION['session_id'])) and (checkSessionID($user)))

Why so many parentheses! It just makes things unclear!

$_SESSION['password'] = $password;

It's generally bad practice to store a password in any place but the database.

This condition returns true, again the else after this is not needed. Remove it to reduce nesting.

Overall, you're on your way, but there's a lot that could be improved.

• Your code is strictly procedural which is becoming less and less neccessary.

• Your code is reinventing something that doesn't need to be made again.

• Your code is hard to read and understand. Partly because of all the nesting you have (which is easy to avoid with an OO design). Here's an article about arrow code written by StackExchange's very own Jeff Atwood.

There're hundreds of frameworks and community run scripts that do the logging in/out for you. Don't think that using one of these is limiting you or making your coding skills worse.

If you insist on using this code, it will suffice, but it will be hard to maintain and expand...

First off, when posting a question, try to give it some natural IDE formatting if you can. this improves readability and you're more likely to get more/better answers!

##Code##

Technically, you can call

$_SERVER["REQUEST_METHOD"] == "POST"

however, my personal preference is just

isset($_POST)

I find that's it's easier to understand and is more intuitive.

This first two conditions encompass basically the entire function. Why not just reduce them to be negative and return false?

Such as

if (!isset($_POST['action']) or $_POST['action'] != 'login') {
 return FALSE;
}

It's reduces unnecessary nesting. Apply the same for if ($_SERVER["REQUEST_METHOD"] == "POST").

if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '')

could be simplified to

if (!isset($_POST['user'], $_POST['password']) or empty($_POST['user']) or empty($_POST['password']))

It's just easier to read.

In your minimum time check between form submissions, you've added an else, but because the initial condition would return false, an else is completely unnecessary. It also nests your code one more layer, which isn't too great if it can be avoided.

$user = test_input($_POST['user']);

The variable here is very ambiguous. I suggest calling something more appropriate such as $username or something that describes what the value is.

hash('sha512', $passtemp . $user);

This not as secure as it could be! First off, if your PHP version supports it, I suggest you use bcrypt instead of hash. It take scares of the salting for you, so that you don't use bad salts such as you have. The salt should typically be a long string composed of random characters. Not just a username.

if ((isset($_SESSION['session_id'])) and (checkSessionID($user)))

Why so many parentheses! It just makes things unclear!

$_SESSION['password'] = $password;

It's generally bad practice to store a password in any place but the database.

This condition returns true, again the else after this is not needed. Remove it to reduce nesting.

Overall, you're on your way, but there's a lot that could be improved.

• Your code is strictly procedural which is becoming less and less neccessary.

• Your code is reinventing something that doesn't need to be made again.

• Your code is hard to read and understand. Partly because of all the nesting you have (which is easy to avoid with an OO design). Here's an article about arrow code written by StackExchange's very own Jeff Atwood.

There're hundreds of frameworks and community run scripts that do the logging in/out for you. Don't think that using one of these is limiting you or making your coding skills worse.

If you insist on using this code, it will suffice, but it will be hard to maintain and expand...