What is the best design?

Question

I have the following class:

class User { public function setName($value) { ... } public function setEmailAddress($value) { ... } public function setUsername($value) { ... } public function getName() { ... } public function getEmailAddress() { ... } public function getUsername() { ... } public function isGroupAdministrator($groupId) { ... } public function isMemberOfGroup($groupId) { ... } public function isSiteAdministrator() { ... } public function isRoot() { ... } public function hasModulePermission($moduleId, $recordId, $permissionCode) { ... } public function hasGroupPermission($groupId, $permissionCode) { ... } public function hasContentPermission($recordId, $permissionCode) { ... } public function hasModulePermission($moduleId, $recordId, $permissionCode) { ... } public function canLogIn() { ... } public function isLoggedIn() { ... } public function setCanLogIn($canLogIn) { ... } } 

Is this a "class of God?"

I am not sure if I should break this class. The problem is that this class method is used by its domain to determine whether to display interface elements on web pages, so there is no behavior in the class.

I believe that I could use permission-related methods in some Permission class, making these methods static (e.g. :: userIsGroupAdministrator (...), :: userIsMemberOfGroup (...) :: userHasGroupPermission (...), :: userHasContentPermission (...))

Any suggestions on how this class could be better?

Answer 1

If you already have a startup code, then refactoring is done in small parts. If not, I would look at the following:

 class User { String username String name String emailAddress Boolean active Integer permission # A bitmask of the statics in the Permission class } class Group { String name } class UserGroupMapping { User user Group group Boolean administrator } class Permission { static IS_ROOT = 1 static IS_SITE_ADMIN = 2 } class Session { User user Boolean logged_in } 

The rest of this really belongs to the service class:

 class SecurityService { static public function hasModulePermission($user, $module, $record, $permission) { ... } static public function hasGroupPermission($user, $group, $permission) { ... } static public function hasContentPermission($user, $record, $permission) { ... } static public function hasModulePermission($user, $module, $record, $permission) { ... } } 

Answer 2

Yes, I think your custom class does too much.

Separate some group-related functions in the Group class and module-related functions into a module class. Or perhaps split permissions into your own set of classes.

Answer 3

It is hard to say just the names of the methods. One heuristic that I use to break up a class is to look at state variables. Are there parts of the state that are usually used together? Can they be broken down into a separate class?

Answer 4

This will help if you can describe how your User object behaves, with whom it interacts, what is its environment, etc.

I suggest that I could put permission-related methods in some Permission class [...]

Yes, you can take a look at political design.

I would also suggest pulling out the fixed state of User ie email / name, etc. and put it in a separate class.

Answer 5

How difficult is it now to add new features or maintain existing features in your application? If you have no problems in this regard, then you are probably good at keeping things as they are (at least for a while).

Yes, your User class has some overlapping responsibilities that could potentially be reorganized into a role-based access control system .

The bottom line is: will you really need it ?

Answer 6

I would like to create the Permissions class and make it a member of the User class.

Answer 7

I would separate the “user” aspect and the “group” approach to managing the class.

Answer 8

I would at least remove the setCanLogIn($canLogIn) function. This should really be determined inside the class, depending on whether the user provided the correct credentials and whether the authentication passed.

Depending on how large this project is or whether it will be part of the structure, it will be determined whether more abstraction is required.

Answer 9

It seems you need separate ACL and Role objects from the User object.
http://en.wikipedia.org/wiki/Role-based_access_control

You can also see how this is done in ZF .

For readability, you can use __get() and __set() instead:

  public function setName($value) { ... } public function setEmailAddress($value) { ... } public function setUsername($value) { ... } public function getName() { ... } public function getEmailAddress() { ... } public function getUsername() { ... } 

Answer 10

It looks reasonably believable. Of course, I do not know the application. I think it makes sense to split the Permissions class.

Ideally, then you will think about what it means to have a class of permissions - that he knows what his actual actions are? In addition, since “permissions” are multiple, you may wonder if you really want to create a collection of individual Permission objects, but if they really are accessories to a simple boolean that can overflow.

Answer 11

As long as the methods in the instance return instance data or only change the internal state, there is nothing wrong with the class.

But, for example, if the canLogIn () method is looking for some external service (repository, member service, etc.), the problem of simplicity and testing capabilities arises. If so, you should have some class of service and have these methods on it. how

 new SomeFactory().GetUserService().canLogIn(user); 

Answer 12

This is not so bad, but I think your permission mechanics can use factoring. This is a case that clearly requires aggregation, not inheritance. If it is likely that non-trivial contact information is associated with the user, you may also want to include an email address, possibly in Identity, which may contain other contact information, if necessary.

In general, I could recommend thinking of your user as a central point for the aggregation and compilation of (non-inheriting) units of functionality - an approach that ensures the achievement of the status of a buzzword in game development under the heading "entity system".

Answer 13

Leave it ... they will find IMO:

 public function isLoggedIn() { ... } public function getEmailAddress() { ... } public function setEmailAddress() { ... } 

Suggest replacing them with Get / Set Name functions that handle the TUserName instance:

 public function getUsername() { ... } public function setUsername($value) { ... } public function getName() { ... } public function setName($value) { ... } 

TUserName will have:

 .FirstName .LastName .MiddleName .UserName 

Suggest replacing them with Get / Set Admin functions that handle the TUserGroup instance:

 public function isGroupAdministrator($groupId) { ... } public function isMemberOfGroup($groupId) { ... } public function isSiteAdministrator() { ... } public function isRoot() { ... } 

TUserGroup will have:

 .MemberOfGroup(GroupID) .AdminSite .Root .AdminGroup(GroupID) 

Suggest replacing them with Get / Set Permission functions that handle the TUserPermissions instance:

 public function hasModulePermission($moduleId, $recordId, $permissionCode) public function hasGroupPermission($groupId, $permissionCode) public function hasContentPermission($recordId, $permissionCode) public function hasModulePermission($moduleId, $recordId, $permissionCode) public function canLogIn() public function setCanLogIn($canLogIn) 

TUserPermissions:

 .ModulePermitted(ModuleID,RecordID,PermCode) .GroupPermitted(GroupID,PermCode) .ContentPermitted(RecordID,PermCode) .ModulePermitted(ModuleID,RecordID,PermCode) .CanLogIn