2

I am developing a project integrated with Dependency Injection (just for reference, I'm using Unity).

The problem is that I have some Manager classes with several methods and in many cases I have dependencies used only in one method. 

public class UserReportManager : IUserManager
{
    private UserRepository  UserRepository  {get;set;}
    private TeamRepository   TeamRepository   {get;set;}
    private CacheRepository   CacheRepository   {get;set;}
    private WorkgroupRepository   WorkgroupRepository   {get;set;}

    public UserManager(UserRepository userRepository,
                       TeamRepository teamRepository,
                       CacheRepository cacheRepository ,
                       WorkgroupRepository workgroupRepository,
                       ... // Many other dependencies 
                      )
    {
        UserRepository  = userRepository;
        TeamRepository = teamRepository;
        CacheRepository = cacheRepository ;
        WorkgroupRepository = workgroupRepository;  
        ... // Setting the remaining dependencies
    }

    public void CreateReportAboutMostActivesUsers(){
        // Uses only UserRepository  
    }

    public void CreateReportAboutUserInsideTeams(int teamID){
        // Uses UserRepository and TeamRepository 
    }

    public void CreateReportAboutUserHistory(){
        // Uses UserRepository and CacheRepository 
    }

    public void CreateReportAboutUsersInsideWorkgroup(int workgroupID){
        // Uses UserRepository and WorkgroupRepository 
    }
}

The UserManager is instantiated in this way:

DependencyFactory.Resolve<IUserManager>();

Note: DependencyFactory is just a wrapper to simplify the access to the UnityContainer

Is it OK to have classes like that in the example? Is there a better way to implement avoiding to instantiate all the unnecessary dependencies?

Improve this question
4
  • 4
    Your Manager class is breaking Single Responsibility Principle. Separate each method into it's own class and you should be fine. Commented Aug 22, 2014 at 10:17
  • All these methods are manages data related to the User entity, but they also need to interacto with the repositories of different entities. I have edited the example methods, maybe now is more understandable their purpose. Should I still create a single class for each method? Commented Aug 22, 2014 at 10:23
  • 4
    Yes. Just because methods operate on same entity doesn't make them cohesive. Their responsibilities are completely different. Just having some class named "Manager" is code smell. Commented Aug 22, 2014 at 10:26
  • On the first glance I would agree to Euphoric, but I am not totally convinced. If the user manager class would be an abstraction about where the user data actually comes from, where the user of that class finally does not need to know any more which repository is involved for a specfic operation, then it will make sense to keep those methods in one class. Commented Aug 22, 2014 at 10:35

2 Answers 2

Reset to default
2

Why is the purpose of this UserManager class?, you can use your repos directly when you need them. The name "Manager" its normally and advise of a class with a lot of or unclear responsibilities, who knows what a "manager" does?.

Other thing looks a little strange in your design its that looks like if you are creating one repository per table or storage (this cache repo) more than one repository per entity/aggregate like the original proposal in Domain Driven Design.

For example, you have a teamRepository instead of having a method findByTeam in the user repository. I expect a teamRepository return me "teams" not users. Think in a repository as an abstraction to access the user entity whatever this users really are. Inside this repo implementation you can off course access different tables or storage systems to get those users.

About the original question, in general if you have N dependencies in a class and each dependency is only used in one method you have a cohesion problem in your class, this is a bad symptom, is an "alarm" to revise your design.

Improve this answer
2
  • The previous example in the question was wrong. With the names used it was easy to think those methods just collected data. I updated the example, so maybe it's easier to understand the purpose of the manager class Commented Aug 22, 2014 at 12:14
  • With this new names the lack of cohesion problem its more evident. You can have one different class for each type of report you need. Think in how your system evolves, a new report with your design need a change in a existing class instead of adding a new report class. Always prefer extending a system adding new classes over modifying existing ones. Commented Aug 22, 2014 at 12:22
-1

You don't need a user manager, what you need is to create an interface for each repository.

So you will have IUserRepository, ITeamRepository and go on.

After that you have to separate the logic from the factory using two different projects, I use to call the logic project DAL and the factory Resolver. You have to do this because if for example you are actually using Sql Server to access your repositories and for some reason you will decide to use EF you have to rewrite the Factory code that it is wrong. Factory must be generic so if you will decide to use EF you have just to create a new DAL, using the same interfaces and just use EF logic, after that you will reference the new DAL instead of the old one in the Factory project and everything will work again.

So create a DAL project, create a class for each repository inheriting from the correct interface (interfaces must be in a third project that I use to call dependencies and must be referenced from both DAL and Resolver) add to the interface the methods sign for the methods that you need like:

public interface IUserRepository
{
    void GetLatestUser(); // get with void makes no sense
}

than in your factory all that you need is just to register the interface with the implementation:

container.RegisterType<IUserRepository, UserRepository>();

PS: you don't need this:

public class DependencyFactory<T> //but just

public static class DependencyFactory

And at the end to obtain your repository you just need to add this function to your factory:

public static T Resolve<T>()
{
    var result = default(T);

    if (Container.IsRegistered(typeof(T)))
    {
        result = Container.Resolve<T>();
    }

    return result;
}
Improve this answer
1
  • Sorry, but I don't agree with the idea to move the methods in the repositories. The repositories should have only the purpose to collect data from a source (e.g. database) not to manage the information in order to prepare it for the User Interface. Commented Aug 22, 2014 at 12:00

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.