views:

1335

answers:

5

Say I have a class called PermissionManager which should only exist once for my system and basically fulfills the function of managing various permissions for various actions in my application. Now I have some class in my application which needs to be able to check a certain permission in one of its methods. This class's constructor is currently public, i.e. used by API users.

Until a couple of weeks ago, I would have simply had my class call the following pseudo-code somewhere:

     PermissionManager.getInstance().isReadPermissionEnabled(this)

But since I have noticed everyone here hating singletons + this kind of coupling, I was wondering what the better solution would be, since the arguments I have read against singletons seem to make sense (not testable, high coupling, etc.).

So should I actually require API users to pass in a PermissionManager instance in the constructor of the class? Even though I only want a single PermissionManager instance to exist for my application?

Or am I going about this all wrong and should have a non-public constructor and a factory somewhere which passes in the instance of PermissionManager for me?


Additional info Note that when I say "Dependency Injection", I'm talking about the DI Pattern...I am not using any DI framework like Guice or Spring. (...yet)

A: 

The singleton pattern is not bad by itself, what makes it ugly is the way it's commonly used, as being the requirement of only wanting a single instance of a certain class, which I think it's a big mistake.

In this case I'd make PermissionManager a static class unless for any reason you need it to be an instanciable type.

Trap
+2  A: 

If you are using a dependency-injection framework, then the common way to handle this is to either pass in a PermissionsManager object in the constructor or to have a property of type PermissionsManager that the framework sets for you.

If this is not feasible, then having users get an instance of this class via factory is a good choice. In this case, the factory passes the PermissionManager in to the constructor when it creates the class. In your application start-up, you would create the single PermissionManager first, then create your factory, passing in the PermissionManager.

You are correct that it is normally unwieldy for the clients of a class to know where to find the correct PermissionManager instance and pass it in (or even to care about the fact that your class uses a PermissionManager).

One compromise solution I've seen is to give your class a property of type PermissionManager. If the property has been set (say, in a unit test), you use that instance, otherwise you use the singleton. Something like:

PermissionManager mManager = null;
public PermissionManager Permissions
{
  if (mManager == null)
  {
    return mManager;
  }
  return PermissionManager.getInstance();
}

Of course, strictly speaking, your PermissionManager should implement some kind of IPermissionManager interface, and that's what your other class should reference so a dummy implementation can be substituted more easily during testing.

Eddie Deyo
The point about injecting via an interface is an important one. The ability to mock the PermissionManager object in tests is what makes DI a powerful approach for more testable code.
Mike Furtak
+1  A: 

You can indeed start by injecting the PermissionManager. This will make your class more testable.

If this causes problems for the users of that class you can have them use a factory method or an abstract factory. Or you can add a parameterless constructor that for them to call that injects the PermissionManager while your tests use another constructor that you can use to mock the PermissionManager.

Decoupling your classes more makes your classes more flexible but it can also make them harder to use. It depends on the situation what you'll need. If you only have one PermissionManager and have no problem testing the classes that use it then there's no reason to use DI. If you want people to be able to add their own PermissionManager implementation then DI is the way to go.

Mendelt
+1  A: 

If you are subscribing to the dependency injection way of doing things, whatever classes need your PermissionManager should have it injected as an object instance. The mechanism that controls its instantiation (to enforce the singleton nature) works at a higher level. If you use a dependency injection framework like Guice, it can do the enforcement work. If you are doing your object wiring by hand, dependency injection favors grouping code that does instantiation (new operator work) away from your business logic.

Either way, though, the classic "capital-S" Singleton is generally seen as an anti-pattern in the context of dependency injection.

These posts have been insightful for me in the past:

Mike Furtak
A: 

So should I actually require API users to pass in a PermissionManager instance in the constructor of the class? Even though I only want a single PermissionManager instance to exist for my application?

Yes, this is all you need to do. Whether a dependency is a singleton / per request / per thread or a factory method is the responsibility of your container and configuration. In the .net world we would ideally have the dependency on an IPermissionsManager interface to further reduce coupling, I assume this is best practice in Java too.