OOP/OOD advice

[Drongo_III]

Hello

 

I'm slowly getting to grips with OOP/OOD  but I would like some advice.

 

I try to follow the principles of single responsibility and high cohesion.  I'm struggling a bit to see how these principles work when you try to use composition.

 

For instance lets says I have an Authentication class handling login on a website.  On login the Authentication class needs to make a db call to find a given user and test against their stored password.  The user also needs to be stored in session when they successfully login so I also compose Authentication with a Session Manager class.

 

The question is am I violating SRP by composing my class with these things? I'm using an MVC so is it better practice to do more in my controller (i.e. run these things as individual actions) rather than composing and encapsulating these things into a single class?

 

I find the principles a bit confusing sometimes so a steer in the right direction would be appreciated.

 

Drongo

[requinix]

Composition is the solution to maintaining single responsibility but dealing with the unavoidable necessities of having a, well, functioning codebase: classes should be responsible for a specific set of actions/data/whatever, however they also need to interact with other actions/data/whatever, so they do that by (eg) composing themselves of the classes corresponding to those actions. It's still single responsibility because every class is only responsible for itself, and anything else it needs is handled by a different class.

 

Authentication should not talk to the database. Only database models should ever have to talk to the database (with occasional exceptions). Instead, Authentication should use a "User" model to grab the corresponding user and/or validate their credentials. Like

try {
	$user = \Models\User::getFromLogin($username, $password);
} catch (\Models\UserException\UserNotFound $e) {
	// no matching username/password
}

$user = \Models\User::get($username);
if ($user && $user->validateLogin($password)) {
	// good
} else {
	// bad
}

or one of the other million ways of doing that idea.

 

This way Authentication is responsible for performing the action of logging a user into the website, and the User class is responsible for user actions like finding them and checking credentials.

 

 

Side comment: Authentication is either a page controller or an entity encapsulating credentials for the website, right? It's not like some sort of utility class that has a single method to log a user in, right?

[Drongo_III]

 

 

Thank Requinix

 

That certainly helped to clear up some stuff and its clear I need to go read more about MVC and especially Models.

 

I realise your answer was simple and intended to illustrate a point but in general is it ok for models, like a User Model to have mixed responsibilities.  Like is it the responsibility of a user model to provision a login mechanism?  I can see how a user model may be a part of that but it feels like the responsibility of dealing with the process shouldn't belong to a user model.  Or should you have a specific LoginUserModel ?

 

Sorry for the extended question i just don't have a real world point of reference for what the model in an application should look like or how much you can put into it.

 

If it is considered ok for the user model to be packed with lots of stuff then is this approach correct?

//USER MODEL

class UserModel 
{
	
	private $userId;
	
	private $userName;
	
	private $password;
	
	/**
	* Some instance of a DB class
	*/
	private $db;
	
	/**
	* Some encryption class which can generate cipher text
	*/
	private $enc;
	
	public function __construct($databaseClass, $encryptionClass){
		
		//some database layer like a table gateway
		$this->db = $databaseClass;
		
		//some encryption class which can be used to test a password against a cipher
		$this->enc = $encryptionClass;
	}
	
	public function getFromLogin($username){
		
		// 1. Retrieve $_POST[''username], $_POST['Password']
		// 2. Set values to $this->username, $this->$password
		// 3. return boolean 
	}
	
	public function validateLogin($password){
		
		// 1. Turn pasword into cipher text
		// 2. Match against supplied password
		// 3. Return boolean
	}

}


//Authentication controller - receives request /AuthController/doLogin

class AuthController
{
	
	
	public function doLogin(){
		
		$db = new $databaseClass(); //e.g. a tablegateway
		$user = new UserModel($db);
		if($user->getFromLogin($_POST['username'])){
		
			$loginSuccess = $user->validateLogin();
			//do stuff 
		}
	}
	
}

Edited March 4, 2017 by Drongo_III

[requinix]

I realise your answer was simple and intended to illustrate a point but in general is it ok for models, like a User Model to have mixed responsibilities.  Like is it the responsibility of a user model to provision a login mechanism?  I can see how a user model may be a part of that but it feels like the responsibility of dealing with the process shouldn't belong to a user model.  Or should you have a specific LoginUserModel ?

Being a database model it should not perform the actual task of logging a user into the website - putting stuff into the session and whatnot - but validating credentials is an important job for it. After all, only the User class understands how its passwords work (supposedly) so it is the only one that can do it.

 

Therefore you use the User class to validate the login information provided, then use other code elsewhere to put stuff into the session and whatnot.

 

A LoginUserModel is breaking the limits of what classes mean. A class basically represents an entity. A thing, like a user. It should not represent an action, like the action of logging a user in. (In the real world it's not quite that definitive but it is the rule of thumb you should go for.)

 

So that does leave a question of where to put the login code: it can't go in the User class because that's beyond the scope of the model, and it can't go into its own dedicated class because logging in is an action and not an entity.

 

The answer depends on your application. Most modern sites use MVC, and one answer is intrinsic to the design pattern itself: use a controller. The "M"odel is the User class and it interacts with the database, the "V"iew is the login page itself that the user works with, while the "C"ontroller is the one that actually does things. As a controller - an entity that represents the concept of a login page, if you will - it performs actions based on the page. One action is to present the login page. Another is to process the login information and react accordingly (ie, log in with good credentials or error with bad ones). So the code to log a user in goes in the login controller.

 

Another answer is to use a business model...

 

Sorry for the extended question i just don't have a real world point of reference for what the model in an application should look like or how much you can put into it.

There are different types of models but they come in basically two flavors:

 

1. Database model. This object probably mirrors the backing database table, with the table's columns as properties or accessor methods. It does things like load from the database, save a record back to it, and perform basic searches. There's generally not too much logic in there.

 

2. Business model. This object tries to bridge the gap between what the database model offers and what the application actually needs. For instance, a plain User database model will be mostly about the data itself (eg, find it in the database, change it, save it back) but an application probably needs to do more (eg, log the user in, send a password reset email).

 

3. A hybrid of the two, which IMO is bad design. Having two classes is only a little more code but you get a clean separation between the representation (database model) and the behavior and usage (business model) and that's more than worth the tradeoff.

 

It is at this point that I realize I made some assumptions that may not be true...

 

If it is considered ok for the user model to be packed with lots of stuff then is this approach correct?

I've been assuming we were talking about treating the User as a database model, I'm not sure that's what you were thinking.

 

So, that code you posted is odd. Incomplete and inconsistent. I'm going to make more [assumptions].

 

Your UserModel looks like... well, it's partly a factory and partly a database model. I've seen that done before and I don't care for it. I'll get back to that in a minute.

- It has a constructor that uses DI for the database and encryption stuff. That's fine. [Except that it is using encryption for passwords and not hashing, which is not fine.]

- It has a getFromLogin method (fetch from the database using the unique username) and validateLogin method (check the provided password against the actual password). Those are fine too.

- getFromLogin has comments talking about getting stuff from $_POST. That's not fine. [but the way the rest of the code is written, let alone there being a $username parameter, suggests the comments are simply wrong.]

 

AuthController looks like a typical MVC controller. The login action

- Constructs the UserModel factory with the given database (but no encryption)

- Grabs data from $_POST and gives it to the model/factory to find a user

- Uses the model to test [the POSTed password] and performs the actual login if successful

 

Back to the factory aspect of UserModel. You're doing two things with an instance of this class: using it to locate user data, and using to represent user data. I've already written enough in this post so I won't get on my soapbox and preach about stuff like statefulness here (unless someone wants to hear it?) so the short version is

 

Don't do that. Either you use a distinct factory class and distinct user class, or (more conveniently) you use static methods for the factory aspect and keep the instance dedicated to being a user model. In other words,

 

// separate factory
class UserModelFactory {

	public function __construct($databaseClass, $encryptionClass) {
		$this->db = $databaseClass;
		$this->enc = $encryptionClass;
	}

	public function getFromLogin($username) {
		/* grab data from database */
		/* if found { */
			return new UserModel($data, $this->db, $this->enc);
		/* } else { */
			return null; // or whatever
		/* } */
	}

}

class UserModel {

	public function __construct($data, $databaseClass, $encryptionClass) {
		$this->data = $data;
		$this->db = $databaseClass;
		$this->enc = $encryptionClass;
	}

	public function validateLogin($password) {
		/* validate... */
	}

}

$factory = new UserModelFactory($database, $encryption);
$user = $factory->getFromLogin($_POST["username"]);
if ($user && $user->validateLogin($_POST["password"])) {
	// ...
} else {
	// ...
}

// static factory
class UserModel {

	public function __construct($data, $databaseClass, $encryptionClass) {
		$this->data = $data;
		$this->db = $databaseClass;
		$this->enc = $encryptionClass;
	}

	public static function getFromLogin($username, $databaseClass, $encryptionClass) {
		/* grab data from database */
		/* if found { */
			return new self($data, $databaseClass, $encryptionClass);
		/* } else { */
			return null; // or whatever
		/* } */
	}

	public function validateLogin($password) {
		/* validate... */
	}

}

$user = UserModel::getFromLogin($_POST["username"], $database, $encryption);
if ($user && $user->validateLogin($_POST["password"])) {
	// ...
} else {
	// ...
}

They're very similar, which is why the factory class is probably overkill for you so I'd say go with static methods.

 

Note that's a separate issue from having database and business models vs. having a hybrid.

 

Not sure if I answered all the questions, or whether I'm creating more.

 

Oh, and you may be starting to feel like the way you're doing DI (by passing around the database and encryption stuff) isn't working very well. I would agree with that.

[Drongo_III]

Hi Requinix

 

I really appreciate your detailed response to this.  It has helped hugely to realise the distinction between a Business Model and a Database model.

 

I've had a shot at a reworked example (not tested just typed atm) and I'd welcome your thoughts.  It's pretty much your example but I've removed the auth dependency from the UserModel and changed up the factory a bit.

 

There is a table gateway acting as the User Database Model (i.e. just transacting data to and from the database for users) and the User Model now concentrates just on delegating to the table gateway and performing tasks on its own entity.

 

I would really welcome your feedback as this is helping me learn...hopefully how to do it the right way.

 

Here is the example:

class UserModel
{

private $userTableGateway;

private $name;

private $username;

private $password;

private $lastLogin;


public function __construct($userTableGateway){

$this->userTableGateway;
}

/*
* Getters/Setters
*/


/*
* Everything below probably represents 'actions'
*/
public function validateLogin($hashedPassword){
if($hashedPassword == $this->password){
return true;
}
return false;
}

/*
* Should these methods be outside the class?
*/

public function updateUser($data){
//loop data into current object
//Delegate to $this->userTableGateway
}

public function deleteUser($id){
//Delegate to $this->userTableGateway
//return boolean on success/failure
}

}



class UserModelFactory
{

public function __construct(){

/*
* $tableGateway encapsulating all the DB access for user table
*/
$this->tableGateway = new userTableGateway();
}

/**
* Required when you want to find a user and get back a populated UserModel
*/
public function getUserModelAsDBUser($userName){

//get data from db
$dbUserData = $this->tableGateway->getUserData($userName);

if($dbUserData) {
//create instance and pass it data
$userModel = $this->createUserModel($dbUserData);

return $userModel;
}
return null;

}



/**
* Required for creating an instance when you want to add a new user
* and pass optional data to it to be populated into the UserModel
*/
public function createUserModel(array $data){

$userModel = new UserModel($this->tableGateway);

if( !empty($data) ){
foreach($data as $k=>$v){
//Call $userModel setters to map raw data to object
//Could be some sort of mapper class instead of foreach
}
}
return $userModel;
}
}




/*
* Controller Code - i.e. Auth Controller
*/

$userModelFactory = new UserModelFactory();
$userModel = $userModelFactory->getUserModelAsDBUser($_POST['username']);

//some class that runs the same hash algorithm as for stored users
$hashedUserPassword = new passwordHasher($_POST['password']);

if($userModel && $userModel->validateLogin($hashedUserPassword) ){

$userModel->setLastLogin(time());
$userModel->updateUser(); //i.e. refresh the user's db record
}

And you said above "Oh, and you may be starting to feel like the way you're doing DI (by passing around the database and encryption stuff) isn't working very well. I would agree with that.".  What would be a better way?  

[requinix]

There's a fair bit of boilerplate code in your factory - particularly in createUserModel. You'll find yourself repeating that in all the factories.

Consider any of

a) Subclassing the factory, so the parent class handles the general work and the child classes provide the specialized logic for the particular models

b) Doing something similiar with the model; a parent class would handle the constructor and setting properties while the children implement any other logic they need

c) Using traits to at least move the logic into a centralized place, if you don't want a class hierarchy

 

For DI, you've already changed it up a bit so what I said is less applicable now.

Having to pass around various classes and such in constructors is annoying, right? It provides a sort of "perfect" dependency injection because you can customize absolutely everything, but at the cost of increasing complexity and of being a nuisance to the developer. You can take a step back from that and use a sort of (gasp!) registry instead; as a demonstration, I use something like

$this->tableGateway = Dependencies::get(userTableGateway::class);

final class Dependencies {

	private static $classes = [];

	private function __construct() { }

	public static function get($class) {
		if (isset(self::$classes[$class])) {
			$class = self::$classes[$class];
		}
		return new $class();
	}

	public static function register($dependency, $class) {
		self::$classes[$dependency] = $class;
	}

}

But that's one of many ways, and they all have advantages and disadvantages. Use whatever works for you.

[Drongo_III]

Thanks for all your help Requinix!