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

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开发者_JAVA技巧 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.

---

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)".

---

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.

---

I don't see this particular scenario being too smelly. It's a completely legitimate case to have a circular reference between the component and its dependency. You can make it 100% bulletproof by introducing a factory, it's up to you to judge if there is any benefit in doing so.

public class StreamingSubscriber
{
    private readonly ILogic _logic;

    public StreamingSubscriber(ILogicFactory logicFactory)
    {            
        _logic = logicFactory.Create(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
{   
    private readonly StreamingSubscriber _StreamingSubscriber;

    public ExchangeLogic (StreamingSubscriber subscriber){
       _StreamingSubscriber = streamingSubscriber;
       Subscribe();
    }

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

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

I do find the fact that your logic implementation wires up an event directly to its dependency's method more troubling than the whole circular reference issue. I would isolate that so that changes in StreamingConnection don't affect StreamingSubscriber, you can do so with a simple anonymous method like so (you could also remove sender from the signature if you want, half of the time I find I don't need it):

streamingConnection.OnNotificationEvent += (sender, args) => _StreamingSubscriber.OnNotificationEvent(sender, args);