Classes for user registration and authentication

```
db = new mysqli('localhost', 'root', 'php123', 'sms');
}
public function login($username, $password) {
$query = $this->db->prepare("SELECT username, password FROM users WHERE username=? AND password=? LIMIT 1");
$password = UserData::hash($password);
$query->bind_param('ss', $username, $password);
$query->execute();
$query->bind_result($username, $password);
$query->store_result();
if($query->num_rows ==1) {
while($query->fetch()) {
$_SESSION['loggedin'] = 1;

UserData::getAndSet($username);

header("Location: dashboard.php");
}
} else {
return false;
}
$query->free_result();
$query->close();
}
public function register($username, $password, $rank, $name) {
$errors = array();
$query = $this->db->prepare("INSERT INTO users VALUES('', $username, $password, $rank, $name);");

$password = UserData::hash($password);

$query->bind_param('ssss', $username, $password, $rank, $name);

if(strlen($username) 15) {
$errors[] = "The length of the username can only range from 3 to 15 characters.";
}
if(strlen($password) 15) {
$errors[] = "The length of the password can only range from 3 to 15 characters.";
}
if(strlen($name) 15) {
$errors[] = "The length of the name can only range from 3 to 15 characters.";
}

$query->execute();
$query->close();
}
}
class UserData {
protected $db;

public $username;
public $password;
public $rank;
public $name;

public function __construct() {
$this->db = new mysqli('localhost', 'root', 'php123', 'sms');
}

public function getAndSet($username) {
$query = $this->db->prepare("SELECT username, password, rank, name FROM users WHERE username=?");
$query->bind_param('s', $us

-
Why are you starting a new database connection for each class? Pass in a database connection object to the constructor:

public function __construct(mysqli $dbConnection) {

This allows your class to work without worrying about actually doing the connection. The class's job is to handle users, not handle database connections!

-
You not only said LIMIT 1 in your query, but you also check that $query->num_rows is 1, and then you even loop over the results (even though there's only one). This sort of triple check is redundant and unreadable. You know there's either 1 or 0 results because you LIMITed the query!

-
Your hashing algorithm is not secure. Use PHP 5.5's password_hash(), or if you don't use 5.5 (And you should!!!) use Password Compat, a library written by the same guy who wrote the password_hash() function for PHP's core.

-
Do not limit your users' password length - Setting a minimum length is good. Setting a maximum length is a kitten killing crime. Don't.

-
Your login() method returns false on error, but nothing on success. Either return true on success, or simply throw an exception on errors.

-
Your UserData class is not a class, it's a namespace. You only use it for static functions (even though you don't declare them as static in your class!).

-
What does your getAndSet method does? It's not clear. Use a proper name or add documentation.

-
Your UserUtil class does absolutely nothing.

Generally, I'd say you can improve the structure of your code by having the following 3 classes:

-
UserService - is responsible for registering, authenticating and verifying users using the following two objects. Basically higher level management.

-
User - Represents a single user in the system. Has absolutely no database interaction whatsoever, and has no idea where it came from. It accepts parameters through the constructor and getters/setters:

class User {
     public function __construct($id, $username, $hashedPassword, $name) {

-
UserMapper - Which is responsible for mapping and/or creating user objects to/from the database. For example

class UserMapper {
     public function fetch(User $user) {
         //$user object will already have the ID filled in. Fetch according to that and fill the same $user object.
     }
 }

Example of execution:

class UserService {

    private $mapper;
    
    public function __construct(UserMapper $mapper) { ... }

    public function register($username, $password, $name) {
        $user = new User($username, $password, $name);
        $user->validateRegistrationDetails(); //Throws ValidationException on validation error.
        $user->generateHash();
        $this->mapper->save($user); //Save user to database;
    }
}

Note that in the above, the User is unaware of the database, which means it's perfectly reusable across applications with different storage systems.

StackExchange Code Review Q#54920, answer score: 9