Resolving circular dependencies in c++

I stumbled several times already upon situations where I created a circular dependency between two classes. Probably like every c++ developer. A circular dependency is bad design in general, it increases coupling between classes and thus makes changes more difficult (Circular dependency - Wikipedia). But in some languages (for example Java) it's possible to create classes which depend on each other without compiler errors. So very often circular dependencies are created and used without noticing. Differently for C++. The strict processing order of C++ compilers will spill out a bunch of errors when you try to make two classes depend on each other.
But you surely had a reason for this attempt. So how you achieve your aim whilst avoiding a circular dependency? See this example of a thread safe, usage based, load balanced object pool implementation:

// File: ObjectPool.h

#pragma once

#include <list>
#include <stdlib.h>
using namespace std;

// provides basic locking capabilities
#include "Lock.h"

// object type supplied by the pool
#include "Element.h"

class ObjectPool {
private:
 list<Element*> elements_;
 Lock lock_;

public:
 ObjectPool(size_t size) {
  for (size_t i = 0; i < size; i++) {
   elements_.push_back(new Element());
  }
 }

 Element *take() {
  // acquires the pool lock. AutoLockAndRelease will release the lock when scope is left
  AutoLockAndRelease autoLock(lock_);

  Element *chosen = getLeastUsedElement();
  chosen->incUsageCount();
  return chosen;
 }

 void release(Element *element) {
  AutoLockAndRelease autoLock(lock_);

  assertThisIsOurElement(element);
  element->decUsageCount();
 }

private:
 // checks if this element belongs to this pool
 void assertThisIsOurElement(Element *element) {
  // implementation details omitted
 }

 Element *getLeastUsedElement() {
  // implementation details omitted
 }
};




// File: Element.h

#pragma once

class Element {
private:
 int usageCount_;

public:
 Element():usageCount_(0) {
 }

 void incUsageCount() {
  usageCount_++;
 }

 void decUsageCount() {
  usageCount_++;
 }

 int getUsageCount() {
  return usageCount_;
 }
};

This is how it'd be used:

ObjectPool pool(5);
Element *element = pool.take();
// use element
pool.release(element);

I spare you the implementation details of Lock.h. Just assume it contains a lock implementation capable of synchronizing threads.

I'd like to modify this object pool and add the capability to the element to release itself. Id' like to use the pool like this:

ObjectPool pool(5);
Element *element = pool.take();
// use element
element.release();

Here are the code changes implementing this feature:

// File: ObjectPool.h

 ...
public:
 ObjectPool(size_t size) {
  for (size_t i = 0; i < size; i++) {
   elements_.push_back(new Element(*this));
  }
 }

 ...




// File: Element.h
...
class Element {
private:
 int usageCount_;
 ObjectPool &parent_;

public:
 Element(ObjectPool &parent):usageCount_(0), parent_(parent) {
 }

 ...

 void release() {
  parent_.release(this);
 }
};

This example suffers from two problems:

1. A a very subtle problem: The reference to the ObjectPool instance is made public to the Entry instance before the constructor of the ObjectPool class finished initializing the instance. This is known as early reference leak and should be avoided (Java theory and practice: Safe construction techniques, Should you use the this pointer in the constructor?).
2. The obvious one: The example doesn't compile. My compiler issues the error "'ObjectPool' does not name a type". This arises from the attempt to create a circular dependency: ObjectPool depends on Element, and Element depends on ObjectPool.

Let's first have a look at what we actually are trying to achieve here. The ObjectPool class manages a specified amount of entries. For each entry a usage count is maintained. The user can call take() to receive the entry with the lowest usage count. The ObjectPool class will use a lock to synchronize take() and release() invocations so it can be used safely in a threaded context. Once the user is done using a entry he invokes release() on the entry which then invokes release() on its parent which does a synchronized release. So how do we resolve the circular dependency while preserving the logic?

The solution is to decouple the ObjectPool and the Element class. I'll show here how to decouple both using a interface. The idea is, the Element class doesn't has to know the whole implementation of the ObjectPool class. It only needs to know the release() method. So we create a interface containing the release method and let ObjectPool implement the interface. Then we tell Entry only about the interface but not the ObjectPool class.

Here is the interface:

// File: Releaser.h

#pragma once

template<typename T>
class Releaser {
public:
 virtual ~Releaser() {};
 virtual void release(T *element) = 0;
};

It defines a single release method with a template type. The template type is essential here. Without the use of a template the interface would need to include Element type to use it as parameter type. But this would again create a circular dependency: ObjectPool -> Releaser -> Element -> Releaser. Using the template trick we break the cycle. Here are the modified implementations of ObjectPool and Element.

// File: ObjectPool.h

#pragma once

#include 
#include 
using namespace std;

#include "Lock.h"
#include "Element.h"
#include "Releaser.h"

class ObjectPool: public Releaser {
private:
 list elements_;
 Lock lock_;

public:
 ObjectPool(size_t size) {
  for (size_t i = 0; i < size; i++) {
   elements_.push_back(new Element(*this));
  }
 }

 Element *take() {
  AutoLockAndRelease autoLock(lock_);

  Element *chosen = getLeastUsedElement();
  chosen->incUsageCount();
  return chosen;
 }

 void release(Element *element) {
  AutoLockAndRelease autoLock(lock_);

  assertThisIsOurElement(element);
  element->decUsageCount();
 }

private:
 void assertThisIsOurElement(Element *element) {
  // implementation details omitted
 }

 Element *getLeastUsedElement() {
  // implementation details omitted
 }
};



// File: Element.h

#pragma once

#include "Releaser.h"

class Element {
private:
 int usageCount_;
 Releaser &releaser_;

public:
 Element(Releaser &releaser):usageCount_(0), releaser_(releaser) {
 }

 void incUsageCount() {
  usageCount_++;
 }

 void decUsageCount() {
  usageCount_++;
 }

 int getUsageCount() {
  return usageCount_;
 }

 void release() {
  releaser_.release(this);
 }
};

Now Element only references Releaser and the dependency cycle is broken.

It’s not my code! I googled it.

Actually this blog post is not about Google. And it is even not about copying code. But it is indeed about code which I did not write. Since a couple days we use Sonar to check our code for coding style or conventions violations. Well this is not that new. We used to use Checkstyle, PMD, Findbugs already for a couple of years. But the switch to Sonar brought the heap of violations in our code up my mind again. Sonar says we have around 3000-5000 violations in our projects. Probably the most of them are eligible. But some of them are not:


Here you see some of the violations found in the Configuration class in the equals() method. Nearly each line has a violation. The problem is: equals() is a automatically generated method. Coding conventions violations in generated code are just useless. Generated code doesn’t has to be maintainable. It doesn’t has to be readable.

I thought about how to tell Sonar to ignore this code. One could use the //NOSONAR comment to make Sonar ignore lines. But you’d have to place it on every line. Or you could use

@SuppressWarnings("all")

but this would suppress all warnings, not only Sonar violations (reference).

Then I stumbled upon @Generated annotation which is part of Java since 1.6. Using this annotation, code generators could automatically mark generated code, making life easier for code analyzers and developers. So in a perfect world my Eclipse code generator would generate this method:

@Override
@Generated("Eclipse source generator")
public boolean equals(final Object obj) {
 if (this == obj) return true;
 if (obj == null) return false;
 if (getClass() != obj.getClass()) return false;
 final ConfigurationBase other = (ConfigurationBase) obj;
 if (autoRefreshDatabaseEnabled != other.autoRefreshDatabaseEnabled) return false;
 ....
}

and all code analyzers would magically ignore this method.

This idea was already picked up by the sonar team but sadly not yet implemented.