1

I have 2 classes that have dependencies.

public class AuditManager
{
      //set of methods
      public static AuditManager Singleton= new AuditManager();
      public  int AuditEvent(int x){
          Event event=new Event(x);
          event.SaveToDB();
       }
} 

Also I have a class:

public  class Event 
{
    int x;

    //set of methods

    public Event (int x)
    {
       this.x=x;
    }

    public User getUserOfEvent(int eventId){
        User usr = Respository.Get(eventId);
        Audit.Singleton.AuditEvent(usr.x); //circular dependency between Audit and event
        return usr;
    }

    public int SaveToDB()
   {
      new EventDal(){id=x}.Save(); //EventDal is used a lot in more Entity-type classes
   }

}

As you see, Audit uses classes like Event (responsibles of representing the model) And also , some actions in the model should be audited.

How is the best way to remove these circular dependencies?

  • 7
    DO. NOT. USE. SINGLETONS! – Euphoric Sep 22 '17 at 6:17
  • 10
    @Euphoric: usage of singletons is as bad as writing sentences in capital letters ;-) – Doc Brown Sep 22 '17 at 6:19
  • Badulake, can you explain what you mean by the term "event" here? Do you use it in a purely technical sense, as in "event driven system", or in a domain sense, like an occasion or a meeting? (I am trying to understand what the responsibility of the Event class might or should be). – Doc Brown Sep 22 '17 at 6:24
  • 2
    You're confusing Event/Audit/User database entities with other roles that the classes have. Make each class do exactly the CRUD operations for the database and nothing else to preserve single-responsibility status. If you want, those classes can also be responsible for associating its own instance with a child table (one-to-many relationship). Anything beyond this must be put in a separate class. – Neil Sep 22 '17 at 6:44
  • 2
    This one? stackoverflow.com/a/35134687/3723423 – Christophe Sep 22 '17 at 6:49
9

The general problem here is the Event class having too many responsibilities. It should be a dumb information container, maybe with some persistence methods, but not more. Keeping any domain logic out of the class, the problem will vanish.

For example, this method:

    User getUserOfEvent(int eventId){
        User usr = Respository.Get(eventId);
        Audit.Singleton.AuditEvent(usr.x); 
        return usr;
    }

should not be part of the Event class, because it contains domain logic. In this example, it does not even use any member of the Event class, so there is no clear reason why it needs to be there. One can place it somewhere else, for example in a separate controller or helper class, which resolves the cyclic dependency.

Even if the method would use members of an Event, one can always pass the event object as a parameter to the method, which makes it possible to refactor the domain logic into another class. Maybe some class EventManager or EventController, or another class with better name, depending on the context. One just have to make sure the Event does not depend on the EventManager, only vice versa.

Here is an older answer of mine for a similar problem, showing three ways of using the SRP to remove a cyclic dependency in a similar, but more complex situatiion.

  • I edited the code. It is not static sorry – Badulake Sep 22 '17 at 8:40
  • @Badulake: sorry, I don't see why you cannot make it static. – Doc Brown Sep 22 '17 at 8:49
  • I can not make it static. Event is an example of class, and it has a much more properties than x ,Also I have dozens of classes similar to Event that represents other items like (Users, GroupsOfUsers, Items, etc) so the problem is more general that my example of code reflects – Badulake Sep 22 '17 at 9:18
  • 2
    @Badulake: in your example, getUserOfEvent does not access the x in Event, it accesses the x in User. So either give a better example, or read my answer again and tell us precisely why applying the SRP does not work for your case. – Doc Brown Sep 22 '17 at 10:53
0
public interface IAuditManager {

  public int CreateEvent(int x);

}

public class MyAuditManager : IAuditManager
{
  //...your implementation of IAuditManager, doesn't matter if it's 
  //singleton
}

public class Event  //...or whatever class from your project
{
    public IAuditManager AuditManager { get; set; }
    int x;

    public Event(int x, IAuditManager auditManager) 
    {
        AuditManager = auditManager;
        this.x = x;
    }

    //...whatever you need to do here...
    //no circular dependencies anymore, because you're using
    //dependency injection here
}

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.