Is it a code smell to inject a dependency and set one of its members to `this`?
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);
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.