Making my User model class code more modular, and correct

Question

This is my first User model I created.

And I wanted to know if there are any good ways I can improve my code? It works with sessions, database and cookies.

I feel like some parts are somewhat written in a weird way, like in my constructor.

I would like to have my code as modular as possible, and maybe if there is anything to improve on when it comes to readability. It's understandable to me, because I wrote it, but it gets slightly complex.

class User
{
 private $db;
 private $req;
 private $data;
 public function __construct(Database $db, RequestHandler $req, $user = null)
 {
 $this->db = $db;
 $this->req = $req;
 $sid = $req->session('uid');
 $cHash = $req->cookie('shash');
 if ($sid && !$user && !$cHash && !$this->data = $this->search($sid)) {
 # if only session user id is set AND user cannot be found
 unset($_SESSION['uid']);
 } elseif (!$sid && $user && !$this->data = $this->search($user)) {
 throw new Exception('User, ' . $user . ', does not exist.');
 } elseif (!$sid && !$user && $cHash) {
 # if only shash (remember) cookie is present
 $sql = 'SELECT * FROM sessions WHERE hash = ?';
 $data = $this->db->query($sql, $cHash);
 if ($data->rowCount() > 0) {
 $id = $_SESSION['uid'] = $data->fetch()->user_id;
 $this->data = $this->search($id);
 }
 }
 }
 public function create($fname, $lname, $email, $pw)
 {
 $sql = 'INSERT INTO users (firstname, lastname, email, password) VALUES(?, ?, ?, ?)';
 $this->db->query($sql, [$fname, $lname, $email, $pw]);
 }
 public function search($user)
 {
 $col = is_numeric($user) ? 'id' : 'email';
 $sql = 'SELECT * FROM users WHERE ' . $col . ' = ?';
 $data = $this->db->query($sql, $user);
 if ($data->rowCount() > 0) {
 return $data->fetch();
 }
 return false;
 }
 public function login($email, $pw, $remember = false)
 {
 if ($data = $this->search($email)) {
 $pwHasher = new PasswordHash(8, false);
 $pw = $pwHasher->CheckPassword($pw, $data->password);
 if ($pw) {
 # user succussfully authenticated
 $this->data = $data; //load data in entire class
 $_SESSION['uid'] = $data->id;
 if ($remember == true) {
 $sql = 'SELECT hash FROM sessions WHERE user_id = ?';
 $data = $this->db->query($sql, $data->id);
 if ($data->rowCount() === 0) {
 $hash = CryptoCharGen::alnum(60);
 $sql = 'INSERT INTO sessions (user_id, hash) VALUES(?, ?)';
 $this->db->query($sql, [$this->data->id, $hash]);
 setcookie('shash', $hash, time() + 60 * 60 * 24 * 7);
 }
 }
 return true;
 }
 }
 return false;
 }
 public function logout()
 {
 $sql = 'DELETE FROM sessions WHERE user_id = ?';
 $this->db->query($sql, $this->data->id);
 unset($_SESSION['uid']);
 setcookie('shash', '', time() - 1);
 session_regenerate_id(true);
 }
 public function getData()
 {
 return $this->data;
 }
 public function isLoggedIn()
 {
 return isset($this->data);
 }
}

Answer 1

There are a couple of things that I would personally change about the code, at the moment the model is doing far too much. It's pulling in cookies, request parameters and accessing the database. That's not the responsibility of the model, any domain logic should be separated from the models themselves.

First I would recommend moving the session and cookie related code somewhere more suitable, like a Filter, which can be ran before an action in a Controller is executed (if we're using the MVC pattern here).

Let's call our Filter class UserAuthFilter, before each request is routed to the relevant controller this class will intercept the request and check if the user is currently signed in.

class UserAuthFilter extends Filter {
 public function beforeAction(RequestHandler $request) {
 $uid = $request->session('uid');
 $sessionHash = $request->cookie('shash');
 //check if these variables are connected to a valid login
 if(/*we are logged in*/) {
 $user = ...;
 $request->set('user', $user);
 }
 return true; // continue with execution
 }
}

Second, I would recommend having your User model be nothing more than a representation of data. So no domain logic in the User class, just data. Anything database related we will move into a new class, let's call it UserRepository.

public class UserRepository {
 private $db; 
 public function __construct(Database $db) {
 $this->db = $db;
 }
 public function create($fname, $lname, $email, $password) {
 // create user
 }
 public function search($searchTerm) {
 // search for $searchTerm
 }
 public function getUserByCredentials($username, $password) {
 // find a user with the gieven username and password or return null/false 
 }
}

Now, the last thing left to refactor is the authentication side of things. To do this we'll create another new class, UserAuthService, which will handle authentication and session management.

public class UserAuthService {
 private $userRepo;
 public function __construct(UserRepository $userRepo) {
 $this->userRepo = $userRepo;
 }
 public function authenticate($username, $password) {
 $user = $this->userRepo->getUserByCredentials($username, $password);
 return $user === false ? false : $user;
 }
 public function logout(User $user) {
 // delete from sessions table where uid = $user->id
 }
}

Then instead of inserting into the session table from the UserAuthService on a successful login, from within your controller you check the credentials with a call to $userAuthService->authenticate(...) and if you get a valid result back, then you insert into the session table.

That's about all I can think of besides a couple of small improvements:

• Give your classes meaningful names. RequestHandler doesn't seem like a suitable name given the calls you're making to it. Perhaps just Request would be better.
• Try to keep your constructors as simple as possible, do you really need to be making calls to the database when a User object is created?

Hope this helps.