What is a better 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 becoming a "God Class?"

I am unsure whether I should split this class up. The problem in doing so is that this class's methods are used by its domain to determine whether to show given UI elements on web pages, so there is not any behavior in the class.

I suppose I could put the permission-related methods in some Permission class, making those methods static (eg. ::userIsGroupAdministrator(...), ::userIsMemberOfGroup(...) ::userHasGroupPermission(...), ::userHasContentPermission(...))

Any suggestions on how this class could be better?

Answer 1

Yes, I think your User class is doing too much.

Split some of the group-related functions into a Group class, and the module-related functions into a Module class. Or maybe split permissions into its own set of classes.

Answer 2

Its hard to tell from method names alone. One heuristic I use to break a class up is to look at the state variables. Are there pieces of state that tend to be used together? Can they be broken out into a separate class?

Answer 3

I would consider creating a Permissions class and making that a member of the User class.

Answer 4

I would split the "user" aspect and "group" management aspect of the class.

Answer 5

I would at least remove the setCanLogIn($canLogIn) function. This should really be determined internally within the class based on if the user has supplied the correct credentials and has been authenticated.

Depending on how large this project is, or whether this is part of a framework will determine if more abstraction is needed.

Answer 6

You seem to need separate ACL and Role objects from the User object.
http://en.wikipedia.org/wiki/Role-based_access_control

You can also see how it's done in ZF.

For readability you might consider using __get() and __set() instead of:

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

Answer 7

It will help if you could describe how your User object behaves, who all it interacts with, what is it's environment etc.

I suppose I could put the permission-related methods in some Permission class [...]

Yes, you can take a look at policy based design.

I would also suggest that you take out the fixed-state of the User i.e. email/name etc and put it in a separate class.

Answer 8

It looks reasonably plausible. I don't know the application, of course. I think it might make sense to factor out a Permissions class.

Ideally, then you'd think about what it means to have a Permissions class -- what does it know, what are its actual actions? Also, since "permissions" is plural, you might wonder if you really want a collection of individual Permission objects, but if those are effectively accessors to a simple boolean that might be overkill.

Answer 9

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

But, for e.g. if canLogIn() method looks for some external service (repository, membership service etc.), there is a problem of simplicity and testability. If this is the case, you must have some service class and have those methods on it. Like

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

Answer 10

How difficult is it now to add new functionality or maintain existing functionality in your application? If you're not having problems in that respect, then you're probably OK keeping things as they are (at least for awhile).

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

The bottom line is: are you really gonna need it?

Answer 11

It's not too bad, but I think your permissions mechanics could use factoring. That's a case that clearly calls for aggregation, not inheritance, as well. If there's a chance of non-trivial contact information being associated with the user, you might want to factor out email address as well, maybe into an Identity that can have other contact info loaded on if needed.

In general, I could recommend thinking of your User as a central point for aggregating and composing (not inheriting) subunits of functionality -- an approach that's achieving buzzword status in game development under the rubric of 'entity system'.

Answer 12

Leave these... they are find IMO:

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

Suggest you replace these with Get/Set Name functions, which handle an instance of TUserName:

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

TUserName would have:

.FirstName
.LastName
.MiddleName
.UserName

Suggest you replace these with Get/Set Admin functions, which handle an instance of TUserGroup:

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

TUserGroup would have:

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

Suggest you replace these with Get/Set Permission functions, which handle an instance of TUserPermissions:

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 would have:

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