System_user Class - all in one class including login functions

Question

I wrote this class a few months ago and noticed from a few examples that it's better to break down these classes and separate them. I am not so sure what is the proper way to break if to parts.

It currently includes a creation of a System_user obj based on user id (fetching user data), login validation, logout, storing user data to session, and I think that's all.

This is my working code:

<?php
namespace MyApp\Models;
use \Exception;
use MyApp\Core\Database;
use MyApp\Core\Config;
use MyApp\Helpers\Session;
use MyApp\Helpers\Cookie;
use MyApp\Helpers\Token;
use MyApp\Helpers\General;
use MyApp\Helpers\Hash;



/**
 *
 *  System User Class
 *
 */
class System_user
{

/*=================================
=            Variables            =
=================================*/

    # @object database Database instance 
    private $db;

    # Users data
    private $data;

    # User user ID name
    public $user_id;

    # User first name
    public $first_name;

    # User last name
    public $last_name;

    # Username
    public $user_name;

    # User Email  
    public $email;

    # User Last logged in  
    public $last_login;

    # is user logged in
    public $isLoggedIn;

    # is user logged in
    public $login_timestamp;

    # is user IP
    private $user_ip;


/*===============================
=            Methods            =
================================*/

    /**
     *
     *  Construct
     *
     */
    public function __construct($system_user = NULL)
    {
        # Get database instance
        $this->db           = Database::getInstance();

        # If system_user isn't passed as a variable 
        if ( !$system_user ) {

            # ...so check if there is a session user id set 
            if (Session::exists(Config::$session_name)) {

                # Insert session data to system_user variable
                $system_user = Session::get(Config::$session_name);

                # Get user data
                $this->find($system_user);
            }

        } else {
            $this->find($system_user);
        }
    }


    /**
     *
     *  Find method: Find user by id or by username 
     *  @param      $user   String/Init     A username or user ID
     *
     */
    public function find($system_user = NULL)
    {
        if ($system_user) {

            // Enable search for a system_user by a string name or if numeric - so by id. 
            $field  = ( is_numeric($system_user) ) ? 'system_user_id' : 'uname';

            // Search for the system_user in the Database 'system_users' table. 
            $data   = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $system_user));

            // If there is a result
            if ( $data ) {
                // Set data 
                $this->setUserData($data);

                return $this;
            } else {
                return false;
            }
        }
        else{
            return false;
        }
    }


    /**
     *
     *  Check if user exist in 'system_users' table
     *  @param      $username       String              Get a username user input
     *  @param      $password       String              Get a password user input
     *  @throws                     Array/Boolian       Is this a signed System user?
     *
     */
    private function system_user_login_validation($username, $password)
    {
        $user_data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));

        if ($user_data)
            return $user_data; 
        else
            return false; 
    }


    /**
     *
     *  Login method
     *  @param      $customer_name  String      Get a customer_name user input
     *  @param      $username       String      Get a username user input
     *  @param      $password       String      Get a password user input
     *  @throws                     Boolian     Is this a signed System user?
     *
     */
    public function login($customer_name, $username, $password)
    {

        # Create a Customer Obj
        $customer = new \MyApp\Models\Customer($customer_name);

        try {
            # Check if the result is an array
            # OR there is no row result: 
            if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
                throw new \MyApp\Core\Exception\Handler\LoginException("Bad company name: {$customer_name}");

           # Change localhost string to 127.0.0.1 (prevent dns lookup)
           $customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;

            # Connect to new database
            $new_connection = $this->db->customer_connect($customer->host, $customer->dbName);

            # If status is connected 
            if ($new_connection) {

                # Check for user credentials data 
                $user_data = $this->system_user_login_validation($username, $password); 

                # If the result isn't a valid array - EXEPTION  
                if ( (!is_array($user_data)) || (empty($user_data)) )
                    throw new \MyApp\Core\Exception\Handler\LoginException("Customer: '{$customer_name}' - Invalid username ({$username}) or password ({$password})");

                # Store Customer in the sesison
                Session::put(Config::$customer, serialize($customer));

                # Update host and db for the db object
                # $this->db->update_host_and_db($customer->host, $customer->dbName);

                # Set data for this System_user object
                $this->setUserData($user_data);

                # Set a login session for the user id: 
                Session::put(Config::$session_name, $this->user_id);

                # Set logged in user sessions
                $this->set_loggedin_user_sessions();

                return $this;

            } else {
                # Connect back to backoffice (current db set)
                $this->db->connect_to_current_set_db();
                throw new \MyApp\Core\Exception\Handler\LoginException('User does not exist');
                return false;
            }

        } catch (\MyApp\Core\Exception\Handler\LoginException $e) {
            $e->log($e);
            return false;
            // die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
        }
    }


    /**
     *
     *  Set sessions for the logged in user. 
     *  Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
     *  
     */
    public function set_loggedin_user_sessions()
    {
        # Generate security sessions
        $this->generate_security_sessions();

        # Set login timestamp 
        Session::put(Config::$login_timestamp, $this->login_timestamp);

        # Set login flag to true
        Session::put(Config::$is_logged_in, true);

        # Set login IP 
        Session::put(Config::$login_user_ip, $this->user_ip);
    }


    /**
     *
     *  Generate system user security sessions
     *  @param      $new_session        Boolean         (optinal) Dedices if to delete the cookie session id [default is set to true]
     *
     */
    public function generate_security_sessions($new_session = true)
    {
        if ($new_session)
            # Generate a new session ID
            session_regenerate_id(true);

        # Fetch cookie session ID 
        $session_id = session_id();
        # Set the session id to the session
        Session::put(Config::$session_id, $session_id);

        # Create a secret token 
        # Set it in session (does them both)
        $secret     = Token::generate_login_token();

        # Combine secret and session_id and create a hash
        $combined   = Hash::make_from_array(array($secret, $session_id, $this->user_ip));
        # Add combined to session
        Session::put(Config::$combined, $combined);
    }


    /**
     *
     *  Check if there is a logged in user
     *
     */
    public function check_logged_in()
    {
        if ( Session::exists(Config::$secret)       &&  # Secret session exists
             Session::exists(Config::$session_id)   &&  # Session_id session exists
             Session::exists(Config::$session_name) &&  # User session exists 
             Session::exists(Config::$is_logged_in) &&  # Check if 'logged in' session exists
             Session::exists(Config::$session_name)     # Check if sys_user id is set in session
            )
        {
            # Get users ip
            $ip = $this->get_system_user_ip();

            # if the saved bombined session 
            if ( 
                 (Session::get(Config::$combined) === Hash::make_from_array(array(Session::get(Config::$secret), session_id()), $ip)) && 
                 (Session::get(Config::$is_logged_in) === true ) 
                )
            {
                # Set ip to system user object
                $this->user_ip = $ip;

                return true;

            } else {
                return false;
            }
        }
        else {
            return false; 
        }
    }


    /**
     *
     *  Check if loggin session is timeout
     *
     */
    public function check_timeout()
    {
        if (Session::exists(Config::$login_timestamp)){

            # Calculate time 
            $session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ; 

            if ($session_lifetime_seconds > Config::MAX_TIME){
                $this->logout();
                return true;
            } else {
                return false;
            }

        } else {
            $this->logout();
            return false;
        }
    }


    /**
     *
     *  Get user IP 
     *
     */
    private function get_system_user_ip()
    {
        if (!empty($_SERVER['HTTP_CLIENT_IP']))
            $ip = $_SERVER['HTTP_CLIENT_IP'];
        elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) 
            $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
        else
            $ip = $_SERVER['REMOTE_ADDR'];

        return $ip;
    }


    /**
     *
     *  Set User data to (this) System_user object
     *  @param      $user_data      Array   User data fetched from the db (usually by the find method)
     *
     */
    private function setUserData($user_data) 
    {
        // Set data for this user object
        $this->user_id              = $user_data['system_user_id'];
        $this->first_name           = $user_data['fname'];
        $this->last_name            = $user_data['lname'];
        $this->user_name            = $user_data['uname'];
        $this->email                = $user_data['email'];
        $this->last_login           = $user_data['last_login'];

        $this->isLoggedIn           = true;
        $this->user_ip              = $this->get_system_user_ip();
        $this->login_timestamp      = time();
    }


    /**
     *
     *  Logout: Now guess what this method does.. 
     *
     */
    public function logout()
    {
        $this->isLoggedIn   = false;
        Cookie::eat_cookies();
        Session::kill_session();
        session_destroy();
        session_write_close();
    }

}

I would like to get suggestions about my current code, and if possible, about structuring it differently with more than one class. (class SystemUser, class systemUserLogin, class systemUserAuthenticator, ect')

ps: In general, the webapp by default logs in to a general database. when a user inserts his company_name, username and password, I check if the company name actually exist, if if does, I disconnect from the general db and connect to the customers database and validate his username & password.

Answer 1

Interestingly enough we just had another question where there was a large user class doing a lot. It was correctly pointed out that is not a good thing as it violates the Single Responsibility Principle. To summarize it, a class should have one and only one responsibility. If your user class is handling the user properties, login, and other actions it is doing too much.

You should familiarize yourself with Dependency Injection. In your constructor you instantiate a database class and then use it to get your database abstraction object. Now you cannot unit test this class because you cannot mock that object. (You can still do an integration test, though). "Dependency injection allows a client to remove all knowledge of a concrete implementation that it needs to use. This helps isolate the client from the impact of design changes and defects. It promotes reusability, testability and maintainability". (source) In other words, your user class has a dependency on the Database class and is at risk if backwards incompatible changes are made to it.

A high level explanation of what you would want to do here to improve this is:

1. Create an interface that your database implements. This will enforce that any database objects in your code will adhere to the same contract (assuming they all implemnt this interface).
2. Instantiate the database object in the client code (the code that calls the user class).
3. Pass it as a parameter to your constructor and then assign it to your User::db property. Make sure you type hint that parameter using the name of the interface you created in step 1 so if a different database object is created and used it will have to adhere to the same contract or else your code will blow up (in testing before it ever goes live).

Here's some simple code to get you started:

The Database Interface

This is just a stub. You will need to complete it.

interface iDatabase
{
    public function row($sql);
    public function customer_connect($host, $dbName);
}

Implement the interface

class Database implements iDatabase

Make your database object a parameter of your contstructor

// Put optional parameters after required parameters
public function __construct(iDatabase $db, $system_user = NULL)

Instantiate your class passing the database object as a parameter

$db = Database::getInstance();
$this->user = new User($db);

You would follow the same example above for any other logic that you pull out of your user class and into its own object. Now your User class does only one thing and does it well and it testable.

Some little stuff

Put a line between your namespace and use statements

PSR-2 coding standards say there should be a line between the namespace declaration and your use statements.

namespace MyApp\Models;

use \Exception;

Class names should be camel case

The PSR-1 standards say class names should be camel case and should not use underscores:

class SystemUser

The PHP community prefers // to # for comments

Although # is a valid syntax for a one line comment in PHP, it is common practice to use //. This came out as a result of the PEAR coding standards.

No need to point out your class' "variables"

Besides the fact that they aren't technically variables but "class members", convention says they go at the top of the class so it is already clear what they are. No need to add unnecessary comments pointing out the obvious. Save your comments for anything that might be ambiguous or needs explanation because it isn't clear from reading the code.

Don't mix coding styles

Your class properties you use both underscores and camel case in your names. Use one or the other but not both.