Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model Design question #43

Open
solleer opened this issue Jan 1, 2017 · 22 comments
Open

Model Design question #43

solleer opened this issue Jan 1, 2017 · 22 comments
Labels

Comments

@solleer
Copy link
Collaborator

solleer commented Jan 1, 2017

@TomBZombie In a discussion at https://www.sitepoint.com/community/t/mvc-refactor-take-2/194004/8 you mention having a success method in the model because it allows the rest to be reused. In another question you said that FormModels should only be to connect the domain model with the GUI and should have no business logic. What is the point then of separating out the success method if the FormModel just connects two things and has no main logic to reuse.
I have an interface that I implement

interface Form {
    public function main($data);
    public function submit($data);
    public function success();
}

and I am starting to question the point of the success method if really all I do in submit is call a method on the model that the FormModel was given.

@TRPB
Copy link
Member

TRPB commented Jan 2, 2017

Essentially it gives more flexibility. For example, submit() might insert some data and success() might trigger an email response or do some kind of logging. By making this action part of the success() method there's no way in this scenario to use the data-insertion logic without also triggering the email send, which might be desirable behaviour.

Having said that, a better way to solve that might be:

interface Form {
    public function main($data);
    public function submit($data, $callback = null);
}

@solleer
Copy link
Collaborator Author

solleer commented Jan 2, 2017

One place I have followed this is at https://github.com/solleer/User-Module. I feel like my form models are doing to much and that I am getting to much of a god class with the User class.

@garrettw
Copy link

garrettw commented Jan 2, 2017

I am getting to much of a god class with the User class.

I would agree. One way to tell is to notice how many parameters your constructor takes. It's not always a sure way to tell, but it's usually a good hint.

I would think a single instance of User should represent a single user and nothing more. If that were the case, methods like getUsers() wouldn't seem to belong in that class.

@TRPB
Copy link
Member

TRPB commented Jan 6, 2017

I do wonder what the function 'validatePasswordConfirm` is doing in the class.

As Robert C. Martin says: A class should have a single reason to change. This class has multiple reasons. Another good test is Misko Hevery's line "Describe the class job without using the word AND". I am struggling to come up with a description of this class that encompasses all the methods and doesn't use the word AND.

There seem to be two different concerns here:

  1. Interacting with the ORM
  2. "Current user".

I'd separate these into two classes and I'm not sure why you need to use Respect\Validator just to check two strings are equal. Seems like an unnecessary dependency to me.

The line

$this->security->hashSecurityProperties($data);

Doesn't sit well with me. This function knows a lot about $data, Why isn't it just $this->security->hash($data->password) or similar?

@solleer
Copy link
Collaborator Author

solleer commented Jan 6, 2017

I have now updated it to remove the validatePasswordConfirm function. I also removed the create function. Both are really just implementation specific and have nothing to do with storing or getting the user so they are now just in the form models that used them.

About the current user, I have thought about extending the class also into a CurrentUser subclass and extending would be good since it could be used anyplace a User could be used and then the getCurrentUser and updateCurrentUser functions would just override the getUser and save functions since getCurrentUser and updateCurrentUser are really just wrappers for getUser and save. The main problem was that it seemed that everywhere that would use the regular User class would also need the CurrentUser class. I'm going to look it all over again and probably make the CurrentUser subclass.

@solleer
Copy link
Collaborator Author

solleer commented Jan 6, 2017

The places I would need both the User class and the CurrentUser subclass are the Credentials class and Authorize class. I think the reason in Credentials seems to be a break in SRP.

@solleer
Copy link
Collaborator Author

solleer commented Jan 6, 2017

The other issue with factoring out the current user stuff is that the save function uses it here

if ($this->getUser($data->username) !== false && $data->username !== $this->getCurrentUser()->username) return false;

@garrettw
Copy link

garrettw commented Jan 7, 2017

I don't understand why a CurrentUser class needs to exist at all. Couldn't you just have a property like $this->currentUser which is just an instance of the User class that represents the current user?

Or better yet, the property you store could even be $this->currentUsername, and then the method $this->getCurrentUser() could then just be shorthand for $this->getUser($this->currentUsername);

Of course, that doesn't address whether all those properties and methods actually belong in the same class..

@solleer
Copy link
Collaborator Author

solleer commented Jan 7, 2017

I do store the user_id and getCurrentUser is shorthand for what you said. The question is whether those current user methods belong in the user class since they have nothing to do with a regular user.

@solleer
Copy link
Collaborator Author

solleer commented Jan 9, 2017

One problem is that is CurrentUser extends User, then if on a separate project I want to extend User to change something for that project then I would have to redefine a CurrentUser object. So I think that composing the User object would be best for the CurrentUser to do.

@TRPB
Copy link
Member

TRPB commented Jan 10, 2017

Generally speaking extends is evil. Don't use it, use composition instead.

@solleer
Copy link
Collaborator Author

solleer commented Jan 10, 2017

I have now factored the Current user out in solleer/Basic-User-Module@f3b62a8. I'm thinking about making an interface for User or something like that.

@TRPB
Copy link
Member

TRPB commented Jan 10, 2017

I'd refactor authorize.php. see http://misko.hevery.com/code-reviewers-guide/flaw-digging-into-collaborators/

You'd be better off just passing the user instance into the class:

<?php
namespace User\Model;
class Authorize {
    private $model;
    private $currentUser;
    private $functions = [];
    private $defaultFunctions = [
        "user" => "\\User\\Model\\Authorize\\User"
    ];
    private $id = false;
    public function __construct(User $model, $currentUser) {
        $this->model = $model;
        $this->currentUser = $currentUser;
        foreach ($this->defaultFunctions as $key => $function) $this->addFunction($key, new $function);
    }
    public function __call($name, $args) {
		if (isset($this->functions[$name])) {
            if ($this->id) $user = $this->model->getUser($this->id);
            else $user = $this->currentUser;
			$result = $this->functions[$name]->authorize($user, $args);
            $this->id = false;
            return $result;
		}
		throw new \Exception("Method \\User\\Model\\Authorize::" . $name . " does not exist");
	}
    public function id($id) {
        $this->id = $id;
        return $this;
    }
    public function addFunction($name, Authorizable $function) {
		$this->functions[$name] = $function;
	}
}

@solleer
Copy link
Collaborator Author

solleer commented Jan 11, 2017

Thanks, I made the change and updated the DI config. I'm trying to come up with a good way to make this system easily portable and customizable to any needs. I am working on an extension of this Module at https://github.com/solleer/Office365-User-Module in order to see how easily extensible the set up it to any needs. The problem I see is that I can't find a good way to do it without extending the User object. I will upload some code to the extension module and comment here when I do.

@solleer
Copy link
Collaborator Author

solleer commented Jan 11, 2017

I have added a bit of code to https://github.com/solleer/Office365-User-Module. I can't figure out how to make this easily extendable.

@solleer
Copy link
Collaborator Author

solleer commented Jan 12, 2017

@TomBZombie I am also questioning the need of the User\Model\Signin class. Should the logic in their just be put into the form models or should it stay in a separate class. I also implemented the model in the Office365 extension and it feels like I have too many things happening.

@solleer
Copy link
Collaborator Author

solleer commented Jan 12, 2017

Also, should this function in Credentials move to the CurrentUser class

public function checkCurrentUserSecurity($username, $password) {
    $id = $this->validateUserCredential($username, $password);

    if ($id === false || $id !== $status->getSigninVar()) return false;
    return true;
}

This function just feels like it doesn't truly belong in any of the current classes.

@solleer
Copy link
Collaborator Author

solleer commented Jan 23, 2017

The above function seems to best fit where it is or in the CurrentUser class but it doesn't seem to fit in either of those either.

@solleer
Copy link
Collaborator Author

solleer commented Jan 27, 2017

@TomBZombie Where do you think the checkCurrentUserSecurity should go. I think I've decided that it definitely will not go in current user because that would make CurrentUser too implementation specific.

@solleer
Copy link
Collaborator Author

solleer commented Feb 14, 2017

@TomBZombie Also I have an issue with https://github.com/solleer/Events-Module in RepeatingEvents and Events. This code is repeated in both

usort($events, function ($event1, $event2) {
      $date1 = new \DateTime($event1->start_date);
      $date2 = new \DateTime($event2->start_date);
      if ($date1 === $date2) return 0;
      else return ($date1 < $date2) ? -1 : 1;
});

I thought about factoring it out but that would require a class to be passed to both classes that only is needed for the getUpcomingEvents method.

@solleer
Copy link
Collaborator Author

solleer commented Mar 21, 2017

Also should the class CurrentUser implement User or stay the way it is.
They are located at https://github.com/solleer/User

@solleer
Copy link
Collaborator Author

solleer commented Apr 2, 2017

@TomBZombie Should the class CurrentUser implement User or stay the way it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants