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?

X.Otano
  • 612
  • 2
  • 7
  • 17
  • 7
    DO. NOT. USE. SINGLETONS! – Euphoric Sep 22 '17 at 06:17
  • 10
    @Euphoric: usage of singletons is as bad as writing sentences in capital letters ;-) – Doc Brown Sep 22 '17 at 06: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 06:24
  • @DocBrown No. They are so bad that extra emphasis is needed. And I believe `Event` here is domain concept. – Euphoric Sep 22 '17 at 06:27
  • I think this programming question belongs to StackOverflow. By the way there are already plenty of answers there for similar issues – Christophe Sep 22 '17 at 06:41
  • Event is an action that ocurrs and should be persisted to database – X.Otano Sep 22 '17 at 06:43
  • @Christophe I surfed stackoverflow but I have not found the answer – X.Otano Sep 22 '17 at 06:44
  • 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 06:44
  • Audit has similar functionality as a Logger, it should manage some actions, and persist somewhere( in this case helped by event class in the database) The problem is that Event class (and many others) have direct dependency with Audit – X.Otano Sep 22 '17 at 06:45
  • @Neil how can I refactor the classes so, Audit don't depend on Db classes (Entity) ? Audit needs to sotre them in Database in the AuditEvent method – X.Otano Sep 22 '17 at 06:47
  • 2
    This one? https://stackoverflow.com/a/35134687/3723423 – Christophe Sep 22 '17 at 06:49
  • @Badulake If Audit is not a database class, that's fine. Audit can depend on them, because Audit's role is to persist when an event occurs and so Audit can call on your DAO to persist items. The database classes persist because they are asked to do so and don't need to depend on other classes to do it for them. You'll need to rewrite them entirely with this mindset. Ideally your DAO classes don't require any non-DAO classes to work. – Neil Sep 22 '17 at 06:53
  • @Neil so your proposal is to remove the Audit calls from DAO classes, so it will respect Single Responsability Principle? My problem then, will be, how to Audit in the way I want, because it depends on the succes/failure of the INSERTS/UPDATES....How can I solve it? (delegates?) – X.Otano Sep 22 '17 at 06:57
  • Your DAO classes can indicate success/failure of the inserts/updates without knowing *how* the errors are handled. You would traditionally handle it through exceptions, but you could also return an object that describes the result of the call as well. – Neil Sep 22 '17 at 07:11
  • @Christophe: the better approach is often not to use interfaces, but to refactor the code in a way it follows the SRP. See this [older SE post for a bigger example](https://softwareengineering.stackexchange.com/questions/306483/how-to-solve-circular-dependency/306489#306489). – Doc Brown Sep 22 '17 at 09:02
  • @DocBrown is a problem if a do a Singleton with injected dependencies? (i pass them in the constructor of the singleton – X.Otano Sep 27 '17 at 08:38
  • @Badulake: not sure what you mean by this comment exactly, but why don't you just follow the recommendation of my answer below? – Doc Brown Sep 27 '17 at 16:50
  • @DocBrown see this new post https://softwareengineering.stackexchange.com/q/358140/283925 – X.Otano Sep 27 '17 at 16:52
  • @DocBrown here all is done – X.Otano Sep 27 '17 at 16:52

2 Answers2

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.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • I edited the code. It is not static sorry – X.Otano Sep 22 '17 at 08:40
  • @Badulake: sorry, I don't see why you cannot make it static. – Doc Brown Sep 22 '17 at 08: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 – X.Otano Sep 22 '17 at 09: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
}
Emerson Cardoso
  • 2,050
  • 7
  • 14