Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm trying to improve my OOP code and I think my User class is becoming way too fat.

In my program a user has rights over "lists". Read, Write, Update, Delete. So I made a User class

class User
{
protected $_id;
protected $_email;
protected $_username;
protected $_hashedPassword;

//...Various setters/getters

public function canRead(List $list){
    //Database query verifies if user has READ rights
}

public function canUpdate(List $list){
    //Database query verifies if user has UPDATE rights
}
//etc...
}
  • Should canRead, canUpdate, canWrite, canDelete methods be moved to another class (UserAccessCheck or something...)?
  • If not, should the actual SQL be moved into the List object (listCanBeReadByUser()) ?
share|improve this question

6 Answers 6

up vote 5 down vote accepted

What you are doing is putting together a mechanism for access control (lists) or ACL. In general, as Ritesh says, it's a bad idea to do that all in one class.

This answer on SO has a nicer, OO way of dealing with access control.

Rather than extending your User class, you should wrap your User class inside a SecureContainer. See the linked-to answer for details, but then the overall call would be:

// assuming that you have two objects already: $currentUser and $controller
$acl = new AccessControlList( $currentUser );

$controller = new SecureContainer( $controller, $acl );
// you can execute all the methods you had in previous controller 
// only now they will be checked against ACL
$controller->actionIndex();

This allows the User class to focus on what it needs to do, and allows the AccessControlList class to focus on what it needs to do.

share|improve this answer
    
Nice, I didn't know about ACL ! –  Ping Apr 10 '13 at 12:45
1  
+1. No need for inheritance when composition is sufficient. –  Brian Apr 10 '13 at 13:09

A class should have only one purpose to exist.

Should canRead, canUpdate, canWrite, canDelete methods be moved to another class (UserAccessCheck or something...)?

Yes, these methods should be put in separate classes.

share|improve this answer
    
Not necessarily. A class should have only one reason to change. You can have these methods in a domain object and delegate the logic to other classes, injected into the domain object. –  pdr Apr 10 '13 at 13:05
  • if the permission is in some sense a property of the user, that's the natural place for it to store the data
  • if the permission is a property of the list, that's the natural place to store it
  • if the test for permission is naturally an action on the user (ie, that syntax fits best with your usage pattern), that's the natural place to put the method
  • if the list shouldn't know about the user, and the user shouldn't know about the list, but some separate ACL should know about both and their relationships, maybe that is where the data and/or method should go
  • etc. etc.

In general, there seem to be three separate (or separable) concerns: describing a User, storing a List, and managing the Access of one to the other.

If you choose to combine two of those concerns into the same class (and if so, which), it should either be because this arrangement emerges naturally from your model (... is a property of ...), or because it's more convenient or efficient. You haven't told us enough to make that choice for you.

share|improve this answer

I like the idea of putting business logic in model classes like User. In fact, I have done exactly that before with privilege methods like canDoBlah().

It makes for easy-to-read code when one uses the object in the UI layer:

public void processList(User user, List list) {
    if (user.canReadList(list)) {
        // do some work
    }
}

I suppose you could put the logic in the List class like you suggested with listCanBeReadByUser(), which for all intents and purposes is the same, but I think it's easier to read on the User object.

The only suggestion that I would make is to avoid putting SQL in the model class. That belongs in the persistence layer, perhaps in a DAO.

share|improve this answer

Abstraction is the idea.

Map all actions you want to achieve in the user class

Any action not relevant to that object should be removed.

share|improve this answer
    
"Abstraction is the idea" - what does this mean? –  gnat Apr 10 '13 at 13:43

Abstraction is one of the main principles of OOP

proper mapping of physical objects to the abstract ones that are programmed is key.

It helps to determine how you want to represent this objects.

share|improve this answer
    
Isn't this answer essentially the same as the one you posted yesterday? –  zenzelezz Apr 12 '13 at 10:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.