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”?
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.
2
Two points:
- I prefer methods like these because they help break up method trains like:
myObject.getFoo().doBar().tooManyCalls()(i.e. Train Wrecks) - 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
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.
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.