2

Consider the following simplified example:

abstract public class Controller {

    protected final boolean isUserAdmin() {
        return getServiceContainer().getUserService().isUserAdmin();
    }

    /* ... other methods ... */ 

}

public class UserController extends Controller {

    public Page showUserPage() {
        /* ... */
        if (isUserAdmin()) { /* Instead of getServiceContainer().getUserService().isUserAdmin() */
            /* ... */
        }
    }

}

Is creating methods such as isUserAdmin() bad practice? More specifically, does the Controller class violate the Single Responsibility Principle when it has such "shortcut methods"?

  • 4
    This is actually the recommended approach. Check the Law of Demeter. – Andy Jul 7 '16 at 11:12
  • 1
    But allowing shortcut methods will eventually lead to a giant Controller class with a huge amount of unrelated methods, will it not? – Pete Jul 7 '16 at 11:20
  • 1
    It might. If it does, that's when you should ask yourself: Isn't the class doing too much? But the problem will not suddenly appear because you started hiding the recursive calls, it has been there all along, just looked differently. – Andy Jul 7 '16 at 11:25
  • 4
    Normally you'd inject the user-service instead of having the consumer go through the IoC container. (Your code is a mild form of the service locator anti-pattern) – CodesInChaos Jul 7 '16 at 11:34
  • "Is creating methods such as isUserAdmin() bad practice?" Yes, and it's equally bad practice for UserService::isUserAdmin() as it is for Controller::isUserAdmin(). Use a capabilities system instead, don't hardcode the privileges of each role all over the code. This code should look more like var user = getServiceContainer().getUserService();... if (user.hasCapability(UserManagement.ENUMERATE_USERS)) ... if (user.hasCapability(UserManagement.CREATE_USER)) ... if (user.hasCapability(UserManagement.RESET_PASSWORD_LOCKOUT)) – Ben Voigt Jul 21 '16 at 20:08
5

Is creating methods such as isUserAdmin() bad practice?

It is, though it isn't so much that this method is bad practice, but rather using a parent class as a dumping ground for methods you want to share with the child objects but that don't have any particular relationship to each other in terms of behaviour is a bad idea.

If the method was a core functionality of all controllers then putting it into the parent class is a good idea. But if it is just a utility method that some controllers might use, then the parent class is not the place for it.

You don't want your parent filling up with methods that the children might or might not need depending on what they do. One set of child classes might use this isUserAdmin method but many might not.

Keep the controller class small and containing specific methods that all controllers will use. If you have behaviour that some controllers might use then for this use composition rather than inheritance. Inheritance is only for behaviour that all children share.

If you have some controllers that need to look up if the user is an admin, and you don't want to re-implement this code in each object (which you are correct to not want to do), create an object that contains this behaviour that you can give to the controllers when they are created.

|improve this answer|||||
  • Dependency injection seems like a good idea. – Pete Jul 8 '16 at 8:01
  • It is. And also as a few others have mentioned, as an aside to main point about parent classes, you also don't want a controller class responsible for determining if the user is admin. The controller should create a small handful of objects and then hand over control to them. So you might create a User object, injecting into it the object used to query if it is admin (which in turn might have say an object representing the current DB connection), and then the controller just hands over control. Small light controllers that do nothing other than create other objects is way to go. – Cormac Mulhall Jul 8 '16 at 8:47
1

Two points:

  1. I prefer methods like these because they help break up method trains like: myObject.getFoo().doBar().tooManyCalls() (i.e. Train Wrecks)
  2. However, If you find yourself writing several of these "shortcut" methods you may very well have a bigger/better refactoring that you can take advantage of.

See Bob Martin's book "Clean Code" and code-smell G36

|improve this answer|||||
1

You use common sense.

I might care a lot about whether the user is an administrator and add a method for that. I might care a lot about the user service object of the controller and use it a lot, for all the purposes that a user service object is designed to be used for. In that case I'd add a method for the user service object.

I suspect that I don't care much about the service container object and that it is just an implementation detail. If that is the case, then having to call getServiceContainer().getUserService() instead of getUserService would be very annoying. Especially if the designer of the class didn't really want this to be more than an implementation detail, and having the to access getServiceContainer() everywhere in my code makes it very hard to change.

|improve this answer|||||
0

Apparently you don't want to fix this methods chain when isUserAdmin() changes its location. So, with respect to the Law of Demeter, you moved it to the base class, which sounds reasonable. So what you have now is code reuse with inheritance, which is, yes, a bad practice. So it throws you back to where you started.

As a solution I see injecting a user itself in the controller. Though it could be awkward, since more probably than not you're using some framework with DI-container. This is where a pure DI is stepping out.

|improve this answer|||||

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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