Jump to content

Archived

This topic is now archived and is closed to further replies.

koen

which of these 2 is better design?

Recommended Posts

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?

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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?

Share this post


Link to post
Share on other sites

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 ;)

Share this post


Link to post
Share on other sites

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?

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites
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).

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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?

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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!

Share this post


Link to post
Share on other sites

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

 

Also, don't plug your site.

Share this post


Link to post
Share on other sites

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.