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.

• Mast: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . Feel free to post a follow-up question instead, although I'd recommend waiting at least a day before doing so. More answers might be incoming.
• John Conde: @Mast I was composing a meta question ask about this as, being a new member here, I wasn't sure what the proper protocol was. Thank you for clarifying.
• Mast: @JohnConde No problem. Feel free to ask if anything else is unclear, me and other regulars can usually be found in The 2nd Monitor.
• potatoguy: @Mast I didn't edit my original code (which is a working code) - I have just added an other code (the one i started to write, which I didn't test and don't know if it works). the questions still refers to the first code. the second code block is just for ref.
• Mast: I know, but that doesn't change anything. Don't add/change code after the first answer comes in, so all answerers see the same code. Should you have improved code, feel free to post a follow-up question linking back to this one. Make sure you tell a bit about what you changed and why.

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.

• potatoguy: Hello John Conde and thankyou so much for your attention. It's weird for me to pass the DB variable to every class which uses it, are you sure this the the best practice in these situations? I didn't really understand why it's not right from your explanation. + I added my current attempt to re-write this class following a different post from the one you have mentioned. I would be glad if I can get your opinion about it too. ~ still didn't finish the last class...
• John Conde: Yes, I am sure. It seems like a lot but it is very common. This promotes test-ability and re-usability. If you wanted to take it a step further, and make life easier for yourself, you can use a container like Pimple, create all of your shared objects once, and then pass that container to your other objects. That way you only have to write it once and if you add other shared objects to it any classes that need them will get them automatically since they already have that container. This is how enterprise applications are coded.
• Mast: I have rolled back both the question and answer to avoid a big mess being created. Should a follow-up question arise, feel free to move the comments you've made to the new question instead. They are still accessible in the edit history.