Is it a code smell to inject a dependency and set one of its members to `this`?

Question

Is it a code smell to inject a dependency and set one of its properties to your current instance? I set my code in this manner so I could completely isolate the service implementation. I have a series of test which all pass (including setting the StreamingSubscriber instance in the logic class).

For example

public class StreamingSubscriber
{
    private readonly ILogic _logic;

    public StreamingSubscriber(ILogic logic)
    {            
        _logic = logic;

        // Not sure I like this...
        _logic.StreamingSubscriber = this;
    }

    public void OnNotificationEvent(object sender, NotificationEventArgs args)
    {
        // Do something with _logic
        var email = _logic.FetchEmail(args);
        // consume the email (omitted for brevity)
    }
}

public class ExchangeLogic : ILogic
{   
    public StreamingSubscriber StreamingSubscriber { get; set; }

    public void Subscribe()
    {
        // Here is where I use StreamingSubscriber
        streamingConnection.OnNotificationEvent += StreamingSubscriber.OnNotificationEvent;
    }

    public IEmail FetchEmail(NotificationEventArgs notificationEventArgs)
    {
        // Fetch email from Exchange
    }
}

If this is a code smell how do you go about to fix it?

Edit

The reason I chose this implementation is because I wanted to be able to test that when streamingConnection from ExchangeLogic was called that it would consumer the email. The current design, while not perfect, allow me to write tests similar such as this.

    [Test]
    public void FiringOnNotificationEvent_WillConsumeEmail()
    {
        // Arrange
        var subscriber = new StreamingSubscriber(ConsumerMock.Object, ExchangeLogicMock.Object);

        // Act
        subscriber.OnNotificationEvent(It.IsAny<object>(), It.IsAny<NotificationEventArgs>());

        // Assert
        ConsumerMock.Verify(x => x.Consume(It.IsAny<IEmail>()), Times.Once());
    }

Now, this is obviously not achievable without doing full blown integration tests If I told my ExchangeLogic to consume the email.

Answer 1

It doesn't strike me as a code smell per se, no.

However, having this work via a setter creates a situation where you could have a timing problem--what if someone calls subscribe and the StreamingSubscriber has not been set yet? Now you have to write code to guard against that. I would avoid using the setter and rearrange it so you would call "_logic.Subscribe(this)".

Answer 2

Yes, this is bad; you are creating a circular dependency.

Generally, not using constructor injection can be considered a code smell, in part because it is impossible for a dependency injection container to satisfy a circular dependency graph when constructors are the only injection points. In this way, constructor injection prevents you from creating situations like this.

Here you are using property injection to make a circular dependency possible, but the prescribed fix for such a code smell is to instead redesign your system to avoid the need for a circular dependency.

The book Dependency Injection in .NET discusses this in Chapter 6: DI refactorings, section 6.3: resolving cyclic dependencies.