which of these 2 is better design?

[koen]

Let's say I have a class that handles a login. Now let's look at two possible implementations of this login in a program.

 

Implementation 1:

 

class control {
    function doActions() {
            $login = new login;
            if ($login) { [...] } else { [...] }
    }
}
class login {
    function __construct() {
            $cred = $this -> getCredentials();
            return $checkCredentials($cred['name'], $cred['pass']);
    }
    function getCredentials() { [return array with name/pass] }
    function checkCredentials($name, $pass) { [return true/false] }
}

 

implementation 2:

 

class control {
    function doActions() {
            $input = registry::getInstance() -> input;
            $login = new login();
            $login_result = $login -> checkCredentials($input['name'], $input['pass'])
    }
}
class login {
    function checkCredentials($user, $pass) { [return true/false] }
}

 

Which of these 2 is better design and why?

[websiterepairguys]

Really to subjective to answer.

 

Better design?  I'd say its less about design and more about utility.  Go with what works.  You're not painting a Picasso here

 

Mark

[koen]

I've read a principle that says 'separate the code that uses an object from the code that creates an object'. I'm not sure why however.

[Evan69]

probably the first one, but its not really gonna make a difference, the class is too small for you to worry about design.

[koen]

The class and the program are a lot bigger but the smaller the example on a forum the more people will be able to read through a post I think. As long as the concept is clear that's ok.

 

Can you elaborate why you would prefer the first above the second?

[448191]

The examples are a bit vague, but I'd also say nr1, mostly because it doesn't directly depend on the registry.

 

I'd personally use something like the following (heavily stripped):

 

class User extends DomainObject {

   const LOGGED_IN = 1;

   public function login($pass){
      if($this->hashPass($pass) == $this->passHash){
        $this->status = self::LOGGED_IN;  
        return true;
      }
            else {
            return false;
      }
   }
}

 

In the abstract class DomainObject you'd have two generic methods: one for restoring a DomainObject's state from the DB, one to do the opposite.

 

Simplified client code:

 

$dataMapper = new UserMapper;
$user = User::factory($_POST['uid'], $dataMapper); //Invokes UserMapper::getDTO($uid), and creates User with the persistent data
if($user->login($_POST['pwd'])){
   //doSomething
}
$dataMapper->save($user);//UserMapper::save() invokes DomainObject::getDTO(), and stores the new state to the db

 

Having a class 'Control' makes little sense. What exactly does it control? Using DomainObjects such as User is a lot more desciptive, and provides higher cohesion

[koen]

Example 2 would have to get the input somewhere so that would also be from the registry. 'Control' was chosen because it should act like a controller, eg getting the input and deciding what course of actions must be taken next.

 

Sidenote: I see you use class constants to set the status. I've seen the same principle used in eg the zend framework: class constants to set some value. What is the reason you do it this way and not directly set status = 1? Maybe because if there were more statuses to set and the '1' would change for some kind of reason, there would only be one place you'd need to change it?

[448191]

Example 2 would have to get the input somewhere so that would also be from the registry.

 

True. Note that I said 'directly'. It's a good idea to centralize dependancies on the Registry to the highest degree still practical. That would probably mean in some key places, like the main accesspoint for each of the three MVC triad members. More than three classes that access the Registry could quickly become a maintenance nightmare. Also, whenever you fetch something from a Registry, validate that you get what you expect. Example:

 

$input = Registry::get('input');
FrameworkAssert::isInstanceOf(
     $input, 'ControllerInput', new SomeException('Registry key "input" must contain instance of "ControllerInput"')
);

 

'Control' was chosen because it should act like a controller, eg getting the input and deciding what course of actions must be taken next.

 

Right, but it doesn't say what it controls. A FrontController/UserController/LoginCommand combo would look something like this:

 

class FrontController {

public static function run()
{
	$token = RequestResolver::tokenFactory();
	$controllerClass = $token->getControllerClass();
	$requestClass = $token->getRequestClass();
	$operation = $token->getMethodName();
	$controller = new $controllerClass(new $requestClass);
	$contoller->$operation();
	$controller->checkout();
}
}
class UserController {

private $request;
private $user;
private $mapper;

public function __construct(Request $request){
	$this->request = $request;
	$this->mapper = new UserMapper;
	$this->user = User::factory($request->getPost('uid'), $this->mapper);
}
public function doLogin() {
	$login = new LoginCommand($this->user);
	if($login->execute($this->request->getPost('psw'), true)){
		$this->view->forward('userindex');
	}
}
public function checkout(){
	$this->mapper->save($this->user);
}
}
class LoginCommand {

private $user;

public function __construct(User $user){
	$this->user = $user;
}
public function execute($psw){
	return $this->user->login($psw);
}
}

 

In this basic scenario abstracting commands into seperate objects may seem pointless, but it's an extra level of abstraction that will prove useful when a single request initiates several commands on different business objects. It allows a controller to execute several commands, thus reducing repeating logic.

 

Sidenote: I see you use class constants to set the status. I've seen the same principle used in eg the zend framework: class constants to set some value. What is the reason you do it this way and not directly set status = 1? Maybe because if there were more statuses to set and the '1' would change for some kind of reason, there would only be one place you'd need to change it?

 

Why do you use regular constants? To avoid dependencies on the value, instead of the meaning of the value. Class constants are just constants, but provide 'grouping': better semantics and avoidance of name clashes.

[koen]

More than three classes that access the Registry could quickly become a maintenance nightmare.

 

Would that classify as an example of striving for strong cohesion? Getting the right value from the registry can be interpreted as a responsibility. So it would make sense to have a seperate class responsible for getting the right values out of the registry (like you said, make sure the object gets what it asks for).

[448191]

I wouldn't go as far as make a class reponsible just for validating the return value of a call on the Registry. If validation was one's only concern, one could move type cheking into the Registry package, and still have calls on the Registry scattered throughout the app.

 

But, limiting the number of classes that access the Registry is indeed a way to reduce coupling and increase cohesion. The call on the Rgiestry itself is the responsibility you should worry yourself about. Every class that makes calls on the Registry is dependent on it's interface.

[koen]

I wouldn't go as far as make a class reponsible just for validating the return value of a call on the Registry. If validation was one's only concern, one could move type cheking into the Registry package, and still have calls on the Registry scattered throughout the app.

 

I guess that's the way I would prefer to implement it. Eg in the registry class making the $input private and creating a method for setting and getting the input values. I don't think scattered calls on the registry are bad smell in this case. There would be scattered calls to whatever class/method is used to get the input data.

 

But, limiting the number of classes that access the Registry is indeed a way to reduce coupling and increase cohesion. The call on the Rgiestry itself is the responsibility you should worry yourself about. Every class that makes calls on the Registry is dependent on it's interface.

 

So, if the calls to the registry became more complex, it would be better to have something in between these callers and the registry, is that what you are trying to say?

[448191]

I guess that's the way I would prefer to implement it. Eg in the registry class making the $input private and creating a method for setting and getting the input values. I don't think scattered calls on the registry are bad smell in this case. There would be scattered calls to whatever class/method is used to get the input data.

 

I do. If you make a large amount of objects dependent on the Registry, you're basically limiting it's capability to change. If the Registry's interface or behaviour changes, it's clients may break or not behave like you originally intended them to. In other words, the clients are coupled to the Registry. To reduce this effect, access the Registry only in key places.

[rosemaster]

I think the best way is making a simple but powerful code. It's not necessary to implement a complex code for a login system. That is a subjective question. Try to find the best option, but I think both codes are good!

[448191]

I'm pretty sure he wants do a little more than create a login system.

 

Also, don't plug your site.