Are circular class dependencies bad from a coding style point of view?

Question

Are circular class dependencies bad from a coding style point of view?

Example:

In a database application we have two classes, one encapsulating information about a single database (DBInfo) and one class which can create a database connection. (ConnFactory)

DBInfo has a getConnection method which uses ConnFactoryto create a connection. But ConnFactory itself needs a DBInfo object to do so.

Like this: (Any coding styles disregarded for the sake of readability)

class DBInfo {
    String name;
    String connectionUrl;

    Connection getConnection() {
        return ConnFactory.getConnection(this);
    } 
}


class ConnFactory {
    Connection getConnection(DBInfo toWhat) {
        return new Connection(toWhat.connectionUrl);
    }
}

My co-workers argue that this is bad practice and it would be better if there were only one direction of dependencies and no circular ones like here.

Is this bad practice, an anti-pattern or a code smell? Are there any drawbacks?

Answer 1

In general, I would call circular dependencies a Code Smell. Note that the term 'Code Smell' mainly indicates that 'here is a piece of code that requires special attention, and is likely to benefit from redesign.'

In most cases I would strongly consider a design where a circular dependency is not necessary, but in rare cases it may be okay.

In your example, the ConnFactory seems redundant, but that may be because your example has been trimmed down. It seems to me, however, that the Connection creation logic would be better if it was moved to the DBInfo class. When you already have a class that contains data about a database, it seems only natural to make it responsible for creating a connection to that database.

Answer 2

Yes, generally speaking circular dependencies are bad, though not always evil. Problems with circular dependencies include tight coupling, mutually dependent modules and generally domino effect, when changes in one module propagate to other modules.

That said, your code is violating Single Responsibility Principle in that DBInfo not only stores information about the database, but is also responsible for obtaining Connection objects. Remove that particular piece of functionality to a separate class and everything will be just fine.