A User class for visitors to register and log in to a site

I'm just looking for some feedback on my User class. The class is designed to handle users being able to register, login, and logout of the site. I'll be using it in conjunction with a forum I'm also working on.

I'm mostly curious how I've handled validation of input, MySQLi interactivity, and whether my code is redundant and horribly structured.

```
username = 'Guest';
$this->password = '';
$this->session = '';
$this->salt = '';

// Checks to see if a cookie is set. If so, examines contents of the
// cookie to see if the username and session ID match the set stored
// in the database. If a match is found, current Object is initilized
// for that user.
if(isset($_COOKIE['user'])) {
$user_details = explode("|", $_COOKIE['user'], 2);
$this->username = $user_details[0];
// First, check that the username is registered in the database. If
// the user exists, compare the session ID's and ensure a match. If
// one is found, set Object for that user, if not set the Object as a
// guest and delete the cookie.
if($this->user_exists()) {
$connection = user_connection();
$sql = "SELECT session FROM users WHERE username='$this->username'";
$result = $connection->query($sql);
$connection->close();
$row = $result->fetch_assoc();
if($row['session'] === $user_details[1]) {
$this->session = $row['session'];
$_SESSION['user'] = $this;
}
else {
$this->username = 'Guest';
setcookie('user', '', time()-1);
}
}
else {
$this->username = 'Guest';
echo "Cookie Login Failed: No such user.";
}
}
// If no cookie is set, check if a session exists. If so, set the current
// Object to that user's username and session ID.
else if(isset($_SESSION['user'])) {
$this->set_

Comments

There is such a thing as having too many comments. Most readers will read the first two or three, realize that they were not particularly useful to them, and skip the rest.

You should not document how the language works (eg // Class variables. or // Constructor).

Some of your comments also don't really add any information (eg Ensure the selected user is in the database. or // Include the MySQLi connection file containing functions to quickly setup a connection to the MySQL database.).

Generally, in-code comments should only explain why you do something, not what or how you do it. What you do should be expressed with good code, good function names, and possibly good PHPDoc function-level comments.

For function-level comments I would choose some organized comment style like PHPDoc:

/**
  * sets the given username if it only contains alpha-numerical characters.
  *
  * @param string $username username
  *
  * @return int 1 if the username was set, 0 if it was not set because it contains invalid characters
  */
  private function set_password($password) {
  }

Your function level comments could sometimes be more concrete. For example register: Adss the current User to the database. Always? No. Then when not? Why are two passwords passed? Does it hash the password? If so, how? Does it echo something to the user? When does it return what?

Naming

Your names do not always express what the function does:

• set_username: sets the username sometimes, but also checks the username.

• construct: construct this instance, but also performs database interactions, echoes, etc

Functions

Some of your functions do too much (see Naming). I would for example extract the filtering from the setting of the username.

Another example is your constructor, which is very long and nested. I would at least extract the database check to its own function, as well as the cookie check.

Reduce nesting

In addition to extracting code to functions, you can also return early to reduce nesting. For example:

if(!$this->set_username($username) || !$this->set_password($password) || !$this->user_exists()) {
    return 0;
}

Echoing

Your class should not echo, because it makes it very static and non-reusable. What if the calling class wants to handle it differently? It can't. Either handle these cases with a return, or throw an exception.

Class size

In my opinion, your class does too much (user model, handle sessions, register, delete, login, logout, etc). I would think about splitting some of this in different classes.

Security: Cookies

Important cookies should be httpOnly, so that they cannot be read out via JavaScript. This limits the damage an XSS attack can do.

Security: SQL injection

Filtering is not the solution to SQL injection. Right now, it actually works fine, but what if your website grows larger, you get more developers on board, and then a user requests that they can be named "Jack Smith" or "super_awesome" or "Jack O'Rily"? As your filtering is very restrictive, a developer might "improve" it, or just remove it completely. And then you are in trouble.

The way to prevent SQL injection is to use prepared statements always, everywhere, for all variable data.

Security: XSS

Same problem as above. Currently, you are save, but for how long? When echoing variable data, always protect against XSS.

Security: Hashing

sha512 is better than md5, but both are not ideal. If you still can migrate easily, use bcrypt.

Inconsistent return values

Sometimes, you use 0/1 as return values for success/failure, and sometimes TRUE/FALSE (sometimes these even mix in the same function). Try to be more consistent.

Misc

• if (cond) {return true; } else {return false; } can be written as return cond.

• your CSS should not be part of your HTML/PHP code, but stored inside an external CSS file.

• HTML doesn't belong in your User class (because its a model class, not a view).

• I don't like restricting a users password, and there doesn't seem to be a good reason to do it here. Why can't a user eg use ? or ß in their password?

• generate salt and generate session share a lot of code, which could be extracted to a function like generateString.

StackExchange Code Review Q#80726, answer score: 2