Design Smell: Temporal Coupling by Mark Seemann

Great insight again in to core principles that often get over looked even by big names in software development.

Looking forward to the rest of the series.

Have you ever considered renaming your blog to programming 101? It's amazing how most my colleagues are still writing procedural code and violate each and every OOP principle. Fortunately there are blogs like yours that explain brilliantly and concisely these topics. Keep up the good job!

Although, I agree with the fact that temporal coupling must be avoided, when possible, I find the solution you propose quite overkill (as always with that dependency injection jazz, IMHO). With the initial design, at least, as a developer, the problem I had were simple to solve (ie: add the Initialize call). Now with DI, as a user of the Small API, I may have to deal with all sort of runtime problems, just because I didn't "read" the API, or the documentation properly :-) Plus, now I have 4 types instead of 1 loaded in my domain. A simple design may not be the most elegant, but it bears simple problems.

So, suppose I want my FragranceFactory to return a Fragrance that implements ISmell (as you have it above) instead of an IFragrance--perhaps because I can't change ISmell). I should just have FragranceFactory return initialized Fragrances wherein the Initialize method does nothing, right? This would accomplish all our goals, although ISmell would still be confusing to the consumer.

Simon, exactly the opposite is true. With the Fragrance API you wouldn't have to deal with any runtime problems, because as soon as you can compile, the design of the code makes it very likely that it's also going to work at run-time (except for null values, which you can't escape no matter what you do).

Getting feedback sooner is betting than getting feedback later. I much prefer getting a compiler error over a run-time exception.

The number of types is hardly a measure of the quality of the code, but if it is, the more the better. The Single Responsibility Principle favors many small classes over a few God Classes.

However, even if you don't agree, then comparing one type against four doesn't make sense. In the absence of interfaces there'd be one class in each alternative (Smell versus Fragrance). When you add interfaces into the mix, you have two types (Smell and ISmell) in the temporally coupled API against four types in the safe API.

Patrick, yes, that might be a possible variation, although perhaps a bit exotic... In which scenario would that be the case?

Not sure to follow you on the number of types. Initially, I had 1 Smell class, and in then end I have 2 classes and 2 interfaces. Or maybe I missed something.
In a real world project, multiply classes by 2, 3, 4 or more (lets add some WCF interfaces to this!) is indeed an issue, unless you generate some or all of it.
I'm not saying what you propose can't be done, or is inelegant. It is exactly the opposite, it's clever and smart, and is the style of code that can be taught in classes or blogged about, but the generalization of these principles can lead to bloated code and huge assemblies if applied massively.

About maintenability, I pretend your code is harder to extend. Initially, I had one class that I could easily extend and the JIT helps me a lot here. I can create a new Spread(arguments) methods easily without breaking any of my existing clients (and without changing the semantics coupling). It's less easy with interfaces, do I need one new ISmellxx interfaces for every new method? New types again? Classes are extensible, interfaces are carved in stone forever - in fact, most people *do* change their interfaces, but that's another story :-)

This blog post discusses two scenarios. The first scenario is that you have only the Smell class. In that scenario, you can replace the Smell class with the Fragrance class (minus the IFragrance interface). No new types are necessary, so you start out with one type and end up with one type.

The second scenarios is when you have a Smell class implementing an ISmell interface. In that case you'd need to go from two to four types.

When it comes to maintainability, I find that the SOLID principles are some of the best guidelines around. The Open/Closed Principle explicitly states that classes should be closed for modification, but open for extensibility. In other words, we extend code only by adding new classes.

Just to be sure:

The factory class is allowed to create concrete instances without using a DI container? And any dependencies that the concrete class needs will have to be passed to the factory class?

Use of a DI Container is IMO orthogonal to the issue.

Conceptually it's best to think about any such factories as concrete classes. If they need to pass dependencies to the objects they create, they can do so by requesting them by Constructor Injection. In other words you are correct that "any dependencies that the concrete class needs will have to be passed to the factory class" (excluding those that are passed as parameters to the Create method).

On a practical side, you can always consider implementing the factory as an infrastructure component based on a container. Essentially, the factory would be an Adapter over the container. However, if you do this, you must be sure to place the Adapter in the Composition Root of the application, as otherwise you risk that the container reference leaks into your application code. This means that the concrete class and the factory will end up being implemented in two different assemblies. If you use Castle Windsor, you can go all the way and not even implement the container at all because instead you can leverage the Typed Factory facility.

All of our code is full of temporal couplings. You can not prevent it because we solve our problems in doing first step1, followed by step2, and so on and so forth. So I don't think you can create code where there is no temporal coupling. What you can do on the other hand, and what I think this blog should have addressed, is to always create valid objects. The design of the class Smell makes it possible creating the object of type Smell without a name. Right after you issued the keyword new() your object is invalid. Any method you call after that will operate on an invalid object (Initialize(...) tries to remedy that). Your solution on the other hand is the right one.

Mark, thanks for this and the other articles in this and other series.
I have one question however:
FragranceFactory.Create checks name for null. Why?
I see the following problems with it:
1) Code duplication:
As Fragrance can be instantiated without the use of the factory, the null check can't be removed from the constructor of Fragrance.
2) Knowledge of implementation details of Fragrance:
The FragranceFactory knows that Fragrance can't work with a name that is null. This is not implicit, this is explicit knowledge. Another implementation of IFragrance could simply assign a default value if null is passed. As FragranceFactory is coupled tightly to Fragrancy, that is not such a big problem. Still I think it is not correct
3) No benefit whatsoever:
Adding the null check in the Create method brings no benefit at all. On the contrary: If we were to change the constructor of Fragrance to allow null and to assign a default value, but would forget to change FragranceFactory.Create accordingly, the semantics of the object creation would differ, depending on how the object is created.

Can you please explain what I have missed and why you decided to put the null check into the Create method?

Daniel, thank you for your insightful comment - you are completely correct.

I just added the Guard Clause out of habit, but your arguments are valid and I will in the future be more careful with my Guard Clauses. It only makes sense that one should only have a Guard Clause if one uses the argument in question. That's not the case when it's just being passed on, so it should not be there.

Thanks for pointing that out.

Mark, thanks for your answer. I hope I didn't sound like I was nitpicking. I simply was a bit puzzled about it.

Not at all - I'm happy that you pointed out something I hadn't properly considered. I learned something from it :)

Hey Mark,
What about fluent interfaces? They completely rely on temporal coupling by design:

var query = new QueryBuilder().WithField("dressing").NotNull().Containing("bacon");

Do you consider this an edge case, or do you actually dislike fluent interfaces in general?

Not at all (to both statements). A Fluent Interface doesn't rely on Temporal Coupling at all - I frequently implement Fluent APIs with immutable types.

See this blog post for a very simple example. For a very complex example, see AutoFixture's Ploeh.AutoFixture.Dsl namespace, which is kicked off by the fixture.Build<T>() method.

In any case, Temporal Coupling doesn't disallow mutability - it just states that if you have mutating methods, the order in which they are invoked (if at all) mustn't matter.

Offtopic question: striving for immutability on the domain model is a good practice (or only value objects should be immutable)?

The most common approach is certainly to model Entities as mutable objects while Value Objects are kept immutable.

When you use 'classic' CRUD-style Repositories or Unit of Work, that may also be the best match to that style of architecture. However, my more recent experience with CQRS and Event Sourcing seems to indicate to me that an immutable domain model begins to make a lot more sense. However, I've still to gain more experience with this before I can say anything with confidence.

It has been my general experience over the last couple of years that the more immutable objects I can define, the easier the code is to work with and reason about.

Quick noob question, why the IFragranceFactory interface? Your code factory.Create("name") which i imagine would be preceded by "var factory = new FragranceFactory()" does not use the interface, does it?

An IFragranceFactory instance can be injected into any client that needs it, freeing it from having to create the tightly coupled instance itself.

The IFragranceFactory instance can be created in the Composition Root, potentially far removed from any clients. At this point it could be created with the 'new' keyword, or by a DI Container.

i came to this blog post with the specific intention of reading about temporal coupling, but after reading your reply to my message and feeling utterly confused i decided to go and read your book, maybe after reading it i'll actually understand what you're saying, and gather some knowledge about DI in the process, obviously this DI is a thing

Sorry, I didn't mean to confuse you...

Hi Mark, this is an old post now but it's still very relevant for me. What about the case when initialization is a time consuming process?

For example, at my company we work with a lot of devices and we often end up writing initialization methods for some Type, let's call it 'FooDevice,' that provides functionality for that kind of device.

Let's say our FooDevice implements an interface very similar to ISmell:

IFooDevice
{
    Task Initialize(string comPort);
    int DoSomething();
}

This particular initialization method accepts as input parameter the COM port on which the device is connected to and through which we send serial commands to control the device.

So the initialization method may then create a serial port object for that port, configure the device in various ways (such as disabling physical buttons, since we're now controlling it programatically) and so forth.

The initialization may be a time consuming process that can potentially take in excess of a few seconds or longer.

Following your advice, we should apply a refactoring here. The constructor for FooDevice should accept the COM port instead. Though that seems to imply that initialization should be done in the constructor:

class FooDevice : IFooDevice
{
    public FooDevice(string comPort)
    {
        // Initialize(comPort);
    }
    
    private void Initialize(string comPort) { /* ... */ }
}

Yet that becomes problematic, because now we've lost the ability to return a Task. But of course if we have our IFooDeviceFactory, it can provide the Task:

IFooDeviceFactory
{
    Task<IFooDevice> Create(string comPort);
}

Now, if I understand your example correctly, then the whole purpose of providing an Abstract Factory is to satisify the requirement that the client be in control of when the object is created. Or else the client could just directly accept an IFragrance and no factory would be needed. Another client could feasibily skip the factory and invoke the Fragrance constructor directly.

But if we allow the same behavior with my above example, we're back in the same awkward situation with the constructor.

What can we do then? Should we make the constructor private, and instead provide a static factory method like the following?

class FooDevice : IFooDevice
{
    private FooDevice() { }

    public static Task<FooDevice> Create(string comPort)
    {
        var device = new FooDevice();
        await device.Initialize(string comPort);

        return device;
    }

    private Task Initialize(string comPort) { /* ... */ }
}

James, thank you for writing. Based on what you describe, I find the last option, the one with a static factory method, preferable.

You may want to consider calling it something like Connect instead of Create, but then again, perhaps this isn't much of an improvement. The most important information is in the return type. Since the method returns a task, it's a strong signal that this could take some time.

What about closing the connection? Is that a concern? If so, you may want to make the object implement IDisposable. These days, various code analysers will tell you to dispose of such an object once you're done with it.

You write:

"the whole purpose of providing an Abstract Factory is to satisify the requirement that the client be in control of when the object is created"

Not quite. The main purpose of such a design is encapsulation, guaranteeing that an object can never be in an invalid state.

Thanks for the reponse Mark. I too would consider calling the static factory method Connect but I stuck with Create for the sake of consistency with the example.

I'm not sure I follow your comment about encapsulation. With your example, FragranceFactory merely delegates its work to the Fragrance constructor--the Fragrance class already achieves encapsulation by requiring name as a constructor argument.

If you had instead introduced a SmellFactory that behaved like the following, then I would agree it encapsulates the required initialization protocol:

			class SmellFactory : ISmellFactory
			{
				ISmell Create(string name)
				{
					var smell = new Smell();
					smell.Initialize(name);
					return smell;
				}
			}
			

So the only usage I can see for keeping a factory around after improving the design with IFragrance is to allow the client to lazily create an instance of Fragrance without tightly coupling the client via the constructor. Otherwise, an already valid instance of IFragrance could be passed to the client.

Indeed, you preface the introduction of the Abstract Factory improvement with, "In many cases the injected value (name) is unknown until run-time..."

As for disposal, yes, we almost always need to implement IDisposable, since the supplier's managed API for the device usually implements IDisposable as well.

However, wouldn't using an Abstract Factory like this be undesirable? IDevice would need to implement IDisposable so that the client could invoke Dispose on the device instance created by the factory. But as you and Steven point out in your book on dependency injection, an abstraction that implements IDisposable is a leaky abstraction. Or would that not apply in this case?

James, thank you for writing. There are two separate concerns here:

• Guaranteeing that an object is in a valid state (encapsulation)
• Enabling polymorphism

What valid state means is specific to an implementation; i.e. a concrete class. Thus, if FooDevice can only be initialised like shown above, that's an implementation detail.

The other concern is polymorphism. Your original definition of the IFooDevice interface had two methods: Initialize and DoSomething. In order to retain both of these capabilities, you're going to need to put both of these functions somewhere. In order to address the sequential coupling implied by that version of the interface, you'll have to distribute those methods over two different polymoprhic types:

IFooDeviceFactory
{
    Task<IFooDevice> Create(string comPort);
}

IFooDevice
{
    int DoSomething();
}

This prevents programmer error. With the original design, a client developer could call DoSomething without first calling Initialize. With the new design, this is no longer possible. You can't call DoSomething unless you have an IFooDevice instance, and you can't get an IFooDevice instance unless you call the Create method.

Is the factory required? Not necessarily. That depends on where the responsibility to create the object lies. If client code is the only code in possession of the comPort value, then you'll need the polymorphic factory in order to intialise a polymorphic object. On the other hand, if the comPort value is available to the application infrastructure (e.g. via a configuration value or environment variable) then you may not the the abstract factory. In that case, any client code that needs an IFooDevice object can receive it via the constructor, and the application's Composition Root can create the object using the concrete Create method.

As to the observation about interfaces inheriting from IDisposable, I still consider that a leaky abstraction. There's usually a way to avoid it, but it tends to depend on the details of the application architecture. Thus, it's difficult to give general advice on that topic without knowing more about the actual circumstances.