Single Responsibility Principle Concerns (Am I thinking about refactoring properly)

Question

My current class PropertyManager looks like this:

public class PropertyManager : IDisposable
{
    private readonly IPropertyRepo _propertyRepo;
    private readonly IUserTypeRepo _userTypeRepo;

    public PropertyManager(IPropertyRepo propertyRepo, IUserTypeRepo userTypeRepo = null)
    {
        if (propertyRepo == null)
            throw new ArgumentNullException("propertyRepo");

        _propertyRepo = propertyRepo;

        if (userTypeRepo != null)
            _userTypeRepo = userTypeRepo;
    }
}

My Property Manager will use the _userTypeRepo in some method to accomplish some task. I think I want to implment a rule that says "Each Manager(Service,Factory,etc) should be responsible for its own repository."

The idea:

The PropertyManager, because it needs to do something with the UserTypeRepo, I should be using the UserManager for such activities.

As such, this means that I will not provide a repo when creating an instance of the UserManager (i.e., var usrMgr = new UserManager(); // no repo). Instead, the UserManager will use the default constructor which will create a new instance of the IUserTypeRepo and provide a new instance of a UserManager and then it can do its work.

I think this accomplishes some design principle such as Separation of Concerns and the Single Responsibility, but then I may be getting away from my Dependency Injection design pattern as the new Managers would now have multiple constructors and look like this:

public class PropertyManager : IDisposable
{
    private readonly IPropertyRepo _propertyRepo;

    public PropertyManager(){
        // use the default repo
        _propertyRepo = new PropertyRepo();
    }

    // Used from Controller or Unit Testing
    public PropertyManager(IPropertyRepo propertyRepo)
    {
        if (propertyRepo == null)
            throw new ArgumentNullException("propertyRepo");
    }
}

public class UserManager : IDisposable
{
    private readonly IUserRepo _userRepo;

    public UserManager(){
        // use the default repo
        _userRepo = new UserRepo();
    }

    // Used from Controller or Unit Testing
    public UserManager(IUserRepo userRepo)
    {
        if (userRepo == null)
            throw new ArgumentNullException("userRepo");
    }
}

Would this be frowned upon? Or am I on the right track? In either case, why and thanks?

Update. After reading Yawar's post I decided to update my post and I think I have a relevant concern.

Let's think of a real world example of the above. I have a PropertyManager in real life named "Robert" one of the jobs he performs each morning at work is to Open() the Property (i.e., he unlocks the Property he is the Manager of). I also have a UserManger who manages people who visit the Property and her name is "Sarah" she has a function that she does called EnterProperty() (which is what she does in the morning when she physically walks into the building).

Rule: UserManager has a dependency on PropertyManager when using the EnterProperty()

This looks like this according to all accepted standards:

Property Manager

class PropertyManager : IPropertyManager
{
    private readonly IPropertyRepo _propertyRepo;

    public PropertyManager(IPropertyRepo propertyRepo)
    {
        if (propertyRepo == null)
                throw new ArgumentNullException("propertyRepo");
            this._propertyRepo = propertyRepo;
    }

    // this is when Robert opens the property in the morning
    public void Open()
    {
        _propertyRepo.Open();
    }

    // this is when Robert closes the property in the evening
    public void Close()
    {
        _propertyRepo.Close();
    }

    // this answers the question
    public bool IsOpen()
    {
        return _propertyRepo.IsOpen();
    }
}

User Manager

class UserManager : IUserManager
{
    private readonly IPropertyRepo _propertyRepo;
    private readonly IUserRepo _userRepo;

    public UserManager(IUserRepo userRepo, IPropertyRepo propertyRepo = null)
    {
        if (userRepo == null)
                throw new ArgumentNullException("userRepo");
            this._userRepo = userRepo;

        if (propertyRepo != null)
            this._propertyRepo = propertyRepo;
    }


    // this allows Sarah to physically enter the building
    public void EnterProperty()
    {
        if(_propertyRepo.IsOpen())
        {
            Console.WriteLine("I'm in the building.");
        }else{
            _propertyRepo.Open(); // here is my issue (explain below)
            Console.WriteLine("Even though I had to execute the Open() operation, I'm in the building. Hmm...");
        }
    }
}

Web API Controller

{
    public void OpenForBusiness(){
        private const IPropertyRepo propertyRepo = new PropertyRepo();
        private IPropertyManager propertyManager = new PropertyManager(propertyRepo);
        private IUserManager userManager = new UserManager(new UserRepo(), propertyRepo);

        // Robert, the `PropertyManager`, opens the `Property` in the morning
        propertyManager.Open();

        // Sarah, the `UserManager`, goes into `Property` after it is opened
        userManager.EnterProperty();
    }
}

Now, everything is cool and I can walk away and I now have a Repository Pattern which use Dependency Injection which supports TDD and not tightly coupled classes among other benefits.

However, is the truly realistic? (explain why I ask in second)

I think a more real-world (realistic) approach is one that does:

Web API Controller

public void Method1()
{
    private IPropertyManager propMgr = new PropertyManager(new PropertyRepo());
    private IUserManager userMgr = new UserManager(new UserRepo()); // no dependencies on any repository but my own

    // 1. Robert, the `PropertyManager`, opens the `Property`
    propMgr.Open();

    // 2. Check to see if `Property` is open before entering
    // choice a. try to open the door of the `Property`
    // choice b. call or text Robert, the `PropertyManager`, and ask him if he opened the `Property` yet, so...
    if(propMgr.IsOpen()){
        // 3. Sarah, the `UserManager`, arrives at work and enters the `Property`
        userMgr.EnterProperty();
    }else{
        // sol, that sucks, I can't enter the `Property` until the authorized person - Robert - the `PropertyManager` opens it
        // right???
    }
}

the EnterProperty() method on the UserManager now looks like this: // this allows Sarah to physically enter the building public void EnterProperty() { Console.WriteLine("I'm in the building."); }

The promised explanation from above:

If we think in real-world terms we must agree that the later is preferred over the former. When thinking of a Repository lets say this is the definition of ones self (i.e., one's Person) (i.e., the UserRepo having all the data related to the User, is to the UserManager as the DNA, Heartbeat, Brain Wave Pattern, etc. is to a Human (the HumanRepo). As such, allowing the UserManager to know about the PropertyRepo and having access to its Open() method violates all Real-World security principles and Business Rules. In reality this says that through My Contructor() I can get an Interface Representation of a PropertyRepo that I can use any way I see fit. This is synonymous to the following logic of the HumanRepo:

I, Sarah - a UserManager - through a new instance of myself with the satisfaction of the PropertyRepo through my Constructor() create a Hologram Interface of Robert, the PropertyManager that I can use any way I see fit. Granted right now I only want to use the IsOpen() method of the PropertyRepo I actually use the Open() method to do it myself if Robert has not yet performed his duty. This is a security concern to me. In the real-world this says I don't have to wait for Robert to open the Property and use the Holocopy of him and implement his Open() method to get access.

That doesn't seem right.

I think with the last implementation I get SoC, SRP, DI, Repository Pattern, TDD, and Logical Security and as close to a real-world implementation as possible.

What do you all think?

Answer 1

I think this accomplishes some OOP goal such as Separating Concerns and the Single Responsibility Principle.

The result is opposite. Now, PropertyManager tightly couples to PropertyRepo; previously, they were loosely coupled.

First approach is better than the latter one. However, PropertyManager and UserManager should not create other objects on which they rely to do their work. The responsibility for creating and managing object should be offloaded to IoC container.

Interfaces describe what can be done, whereas classes describe how it is done. Only classes involve the implementation details—interfaces are completely unaware of how something is accomplished. Because only classes have constructors, it follows that constructors are an implementation detail. An interesting corollary to this is that, aside from a few exceptions, you can consider an appearance of the new keyword to be a code smell. - Gary McLean Hall

Answer for Updated Question:

In your updated question, you combine Service/Manager and somewhat Domain into a single class - PropertyManager, UserManager. It becomes personal preference.

I personally like to keep them separate. In addition, I like to use Role based and Claim based authorization. Let me use my GitHub sample project as a reference. Please feel free to clone it.

User Domain

User class is also used by Entity Framework Code First Fluent API.

public partial class User
{
   public int Id { get; set; }
   public string UserName { get; set; }
   public string FirstName { get; set; }
}

User Service

public class UserService : IUserService
{
   private readonly IRepository<User> _repository;

   public UserService(IRepository<User> repository)
   {
      _repository = repository;
   }

   public async Task<IPagedList<User>> GetUsersAsync(UserPagedDataRequest request)
   {
   ...
   }
}

Action Method

Notice that UI related Business Logic stays at UI layer.

public async Task<ActionResult> Login(LoginModel model, string returnUrl)
{
   if (ModelState.IsValid)
   {
      bool result = _activeDirectoryService.ValidateCredentials(
         model.Domain, model.UserName, model.Password);
      if (result)
      {
          ...
      }
   }
   ...
}

Answer 2

you can take quite a bit of a different approach.....( ignoring your repositories, but allowing for it to be injected )

In this system, the property is only readable, with an event system to handle the mutations, the event system also has rules system which controls what mutations are allowed. This means even if you have a property object you can't mutate it without going through its rules.

This code is more conceptual. The next logical step is to use a full actor model and something like (akka.net) and you may find your repository pattern just disappearing :)

public class Property
{
    public string Name { get; private set; }
    private IPropertyRules _rules;
    private List<User> _occupants = new List<User>();
    private IEventLog _eventLog;

    public Property(IPropertyRules rules, IEventLog eventLog)
    {
        _rules = rules;
        _eventLog = eventLog;
    }

    public ActionResult Do(IAction action, User user)
    {
        _eventLog.Add(action, user);
        if (_rules.UserAllowedTo(action, user, this))
        {
            switch (action)
            {
                case Open o:
                    Open();
                    return new ActionResult(true, $"{user} opened {Name}");
                case Enter e:
                    Enter(user);
                    return new ActionResult(true, $"{user} entered {Name}");
            }
            return new ActionResult(false, $"{Name} does not know how to {action} for {user}");
        }
        return new ActionResult(false, $"{user} is not allowed to {action} {Name}");
    }

    private void Enter(User user)
    {
        _occupants.Add(user);
    }

    private void Open()
    {
        IsOpen = true;
    }

    public bool IsOpen { get; set; }
}

public interface IEventLog
{
    void Add(IAction action, User user);
}

public class Enter : IAction
{
}

public interface IPropertyRules
{
    bool UserAllowedTo(IAction action, User user, Property property);
}

public class Open : IAction
{

}

public class ActionResult
{
    public ActionResult(bool successful, string why)
    {
        Successful = successful;
        WhatHappened = why;
    }

    public bool Successful { get; private set; }
    public string WhatHappened { get; private set; }
}

public interface IAction
{
}

public class User
{
}

Answer 3

I think I agree with your SoC and breaking the PropertyManager class into PropertyManager and UserManager classes. You are almost there.

I would just refactor as shown below:

public class PropertyManager : IDisposable, IPropertyManager
    {
        private readonly IPropertyRepo _propertyRepo;


        // Used from Controller or Unit Testing
        public PropertyManager(IPropertyRepo propertyRepo)
        {
            if (propertyRepo == null)
                throw new ArgumentNullException("propertyRepo");
            this._propertyRepo = propertyRepo;
        }
    }


public class UserManager : IDisposable, IUserManager
    {
        private readonly IUserRepo _userRepo;


        // Used from Controller or Unit Testing
        public UserManager(IUserRepo userRepo)
        {
            if (userRepo == null)
                throw new ArgumentNullException("userRepo");
            this._userRepo = userRepo;
        }
    }

Note: Just extract IPropertyManager & IUserManager so that the calling classes will depend upon the interfaces and provide the implementation.

Creating parameterless constructor is useless if you want to (you should) force the client to provide the concrete implementation of IPropertyRepo and IUserRepo interfaces.

public PropertyManager(){
        // use the default repo
        _propertyRepo = new PropertyRepo();
    }

I dont think you would need

if (propertyRepo == null)
    throw new ArgumentNullException("propertyRepo");

or

if (userRepo == null)
    throw new ArgumentNullException("userRepo");

as IPropertyRepo and IUserRepo will be resolved via a IoC at the startup of your application (say its MVC then before calling the controller IoC will resolve them) so no need to check for null. I have never checked the dependencies for null in my code.

From what you have posted here thats pretty much it.

Unit of Work pattern is used for repository layer not in the manager layer. I would delete that from the title.

Hope this helps!