8
\$\begingroup\$
<?php
namespace Users;

use mysqli;

class Users {
    protected $db;
    public function __construct() {
        $this->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) < 3 || strlen($username) > 15) {
            $errors[] = "The length of the username can only range from 3 to 15 characters.";
        }
        if(strlen($password) < 3 || strlen($password) > 15) {
            $errors[] = "The length of the password can only range from 3 to 15 characters.";
        }
        if(strlen($name) < 3 || 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', $username);
        $query->execute();
        $query->bind_result($username, $password, $rank, $name);
        $query->store_result();
        while($query->fetch()) {
            $this->username = $username;
            $this->password = $password;
            $this->rank     = $rank;
            $this->name     = $name;
        }
        $query->free_result();
        $query->close();
    }
    public function hash($string) {
        $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
        $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

        $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

        for($i=0; $i < 3; $i++) {
            $string = md5($string);
            $string = sha1($string);
            $string = bin2hex($string);
            $string = md5($string);
        }

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

$Users    = new Users();
$UserData = new UserData();
$UserUtil = new UserUtil();

?>

This is the beginning of my project, "Student Management System" and before I proceed I want this code to be rated, and how can I improve this code with better ways.

\$\endgroup\$
0

2 Answers 2

9
\$\begingroup\$
  • 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.

\$\endgroup\$
6
  • \$\begingroup\$ I understand about the fact that my hashing algorithm is not secure. I usually use PBKDF2 but this is not going to be a public project, but rather a private project as practise to learn new stuff. \$\endgroup\$ Jun 22, 2014 at 10:18
  • \$\begingroup\$ I've set a maximum length because, don't you think it will be a problem to the performance? (That was just a logic I applied there, and who will have a long password?) \$\endgroup\$ Jun 22, 2014 at 10:19
  • \$\begingroup\$ I don't understand about the static thing of the 'UserData Class'. Like C# do I have to make it static void? \$\endgroup\$ Jun 22, 2014 at 10:21
  • \$\begingroup\$ The method getAndSet does this: It gets the username and fetches all data of that user and sets all the variables with that data for easy access. \$\endgroup\$ Jun 22, 2014 at 10:22
  • \$\begingroup\$ The class UserUtil is for stuff like changePassword() etc. But I am still thinking of a way where I have to use only 1 function to update Password, rank etc. \$\endgroup\$ Jun 22, 2014 at 10:23
2
\$\begingroup\$
  1. Database connections all over the place.
  2. Make your code modular.
  3. Instead of using a lot of classes in one file, make one class per file, then include them where needed.
  4. Try to write code that conforms to loose-coupling.

The following code has not been tested but should give some insight into how to structure your code, more or less (just some structure to ponder about).

<?php

ini_set("display_error", "1");
error_reporting(E_ALL);

interface IConnectInfo {

    const HOST = "localhost";
    const UNAME = "root";
    const DBNAME = "sms";
    const PW = "php123";

    function testConnection();
}

class ConSQL implements IConnectInfo {

    // Passing values using scope resolution operator
    private $server = IConnectInfo::HOST;
    private $currentDB = IConnectInfo::DBNAME;
    private $user = IConnectInfo::UNAME;
    private $pass = IConnectInfo::PW;

    public function testConnection() {

        $hookup = new mysqli($this->server, $this->user, $this->pass, $this->currentDB);

        if (mysqli_connect_error()) {
            die("bad mojo");
        }

        echo "You're hooked up Ace! <br>". $hookup->host_info;

        $hookup->close();
    }
}

class Users extends ConSQL {

    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) < 3 || strlen($username) > 15) {
            $errors[] = "The length of the username can only range from 3 to 15 characters.";
        }
        if(strlen($password) < 3 || strlen($password) > 15) {
            $errors[] = "The length of the password can only range from 3 to 15 characters.";
        }
        if(strlen($name) < 3 || strlen($name) > 15) {
            $errors[] = "The length of the name can only range from 3 to 15 characters.";
        }

        $query->execute();
        $query->close();
    }
}

class UserData extends ConSQL {

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

    public function getAndSet($username) {
        $query = $this->db->prepare("SELECT username, password, rank, name FROM users WHERE username=?");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($username, $password, $rank, $name);
        $query->store_result();
        while($query->fetch()) {
            $this->username = $username;
            $this->password = $password;
            $this->rank     = $rank;
            $this->name     = $name;
        }
        $query->free_result();
        $query->close();
    }
    public function hash($string) {
        $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
        $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

        $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

        for($i=0; $i < 3; $i++) {
            $string = md5($string);
            $string = sha1($string);
            $string = bin2hex($string);
            $string = md5($string);
        }

        return $string;
    }
}

$Users    = new Users();
$Users->testConnection();
$Users->login(); 
$Users->register();

$UserData = new UserData();
$UserData->testConnection();

$UserUtil = new UserUtil();
$UserUtil->testConnection();


?>

Get this book, which gave me some valuable insights into thinking in an OOP way.

\$\endgroup\$
1
  • \$\begingroup\$ This code is outdated Ive updated the code. \$\endgroup\$ Jul 10, 2014 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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