Testable code: Attach event handler in constructor

Question

as for my understanding, part of writing (unit-)testable code, a constructor should not do real work in constructor and only assigning fields. This worked pretty well so far. But I came across with a problem and I'm not sure what is the best way to solve it. See the code sample below.

class SomeClass
{
    private IClassWithEvent classWithEvent;

    public SomeClass(IClassWithEvent classWithEvent)
    {
        this.classWithEvent = classWithEvent;

        // (1) attach event handler in ctor.
        this.classWithEvent.Event += OnEvent;
    }

    public void ActivateEventHandling()
    {
        // (2) attach event handler in method
        this.classWithEvent.Event += OnEvent;
    }

    private void OnEvent(object sender, EventArgs args)
    {
    }
}

For me option (1) sounds fine, but it the constructor should only assign fields. Option (2) feels a bit "too much".

Any help is appreciated.

Answer 1

A unit test would test SomeClass at most. Therefore you would typically mock classWithEvent. Using some kind of injection for classWithEvent in ctor is fine. Just as Thomas Weller said wiring is field assignment.

Option 2 is actually bad IMHO. As if you omit a call to ActivateEventHandling you end up with a improperly initialized class and need to transport knowledge of the requirement to call ActivateEventHandling in comments or somehow else, which make the class harder to use and probably results in a class-usage that was not even tested by you, as you have called ActivateEventHandling and tested it but an uninformed user omitting the activation didn't, and you have certainly not tested your class when ActivateEventHandling was not called, right? :)

Edit: There may be alternative approaches here which are worth mentioning it

Depending on the paradigm it may be wise to avoid wiring events in the class at all. I need to relativize my comment on Stephen Byrne's answer.

Wiring can be regarded as context knowledge. The single responsibility principle says a class should do only one task. Furthermore a class can be used more versatile if it does not have a dependency to something else. A very loosely coupled system would provide many classes witch have events and handlers and do not know other classes.

The environment is then responsible for wiring all the classes together to connect events properly with handlers. The environment would create the context in which the classes interact with each-other in a meaningful way. A class in this case does therefore not know to whom it will be bound and it actually does not care. If it requires a value, it asks for it, whom it asks should be unknown to it. In that case there wouldn't even be an interface injected into the ctor to avoid a dependency. This concept is similar to neurons in a brain as they also emit messages to the environment and expect answer not knowing neighbouring neurons.

However I regard a dependency to an interface, if it is injected by some means of a dependency injection container just another paradigm and not less wrong.

The non trivial task of the environment to wire up all classes on start may lead to runtime errors (which are mitigated by a very good test coverage of functional and integration tests, which may be a hard task for large projects) and it gets very annoying if you need to wire dozens of classes and probably hundreds of events on startup manually. While I agree that wiring in an environment and not in the class itself can be nice, it is not practical for large scale code.

Ralf Westphal (one of the founders of the clean code developer initiative (sorry german only)) has written a software that performs the wiring automatically in a concept called "event based components" (not necessarily coined by himself). It uses naming conventions and signature matching with reflection to bind events and handlers together.

Answer 2

Wiring events is field assignment (because delegates are nothing but simple reference variables that point to methods).

So option(1) is fine.