Are factories that only return one type of object bad?

Question

Someone in a Stackoverflow post (I didn't bookmark the question unfortunately) commented, that Factories that only return one type of object are a code smell.

I find myself writing these kinds of factories pretty often. Sometimes because i don't want to bloat my code with constructor overloads and sometimes because i just want to give a name to an object with specific attributes. A factory would then look like this:

class CarFactory {
    public static Car createDefault() { return new Car(0, 0, 0, 0, 0, Color.BLACK) }
    public static Car createFancy() { return new Car(1, 1, 1, 1, 1, Color.RAINBOW) }
    ...
}

This is just a general example, but i think the calls look much better in the code than the constructor calls with a load of ominous values.

However these kinds of factories usually don't do runtime decision making (like factory methods often do). And i probably won't create interfaces either (Like the Abstract Factory Pattern uses). It's just a class that provides static creation methods, so that my code doesn't get cluttered and i have only one method to change if i think, that a fancy car should be red with stripes.

Are these kinds of Factory Classes necessarily bad?

Answer 1

The purpose of the factory pattern is to provide an abstraction between object creation and code that needs to create objects. There are many reasons why this may be desirable:

• Objects might be able to be pooled, and a factory can manage the pool. This is what Java's Integer.valueOf(int) does.

• Objects might have tricky constructors to use. This is often a sign of an object doing too much, but sometimes that is just how an object needs to be. A factory can provide a single location to invoke the complex constructor.

• Objects might have state that requires both a constructor call and post-construction initialization. While a bad thing, you might be backed up against a wall and forced to do that because of language or framework constraints. A factory allows you to ensure all creation logic goes through one chokepoint that can be tested and reused, ensuring all objects are created correctly.

None of this answers your question directly, however.

YAGNI is a really good mantra, but sometimes you know you will need it, and it is less effort to get it right the first time than to do extensive refactoring later. If I sketch out a design where I know there will be complex construction logic, I may go ahead and create a factory first so I do not need to perform tedious refactoring later. Even with an IDE, that can still require a lot of manual changes.

Factories that return a single object, inflexible factories, are testable. If you know you always get the right type of car from that factory, code that uses cars is easier to test as well. Maybe I have code that crashes cars together like a child playing with Hot Wheels. If there is a defect in my crash logic, is it because of the crash logic itself? Maybe it is constructing cars incorrectly? A factory like the one you mention might make it easier to identify defective code. If the car test is correct but the crash test fails, that tells me something different than both tests failing.

In the end this is a bit of a subjective question, but there are objective reasons for picking one way or the other (factory or direct instantiation). No, this is not necessarily a sign of bad code. Look at the design as a whole, and weigh the cost and benefit of both options in the context of your project.

Answer 2

I don't think what you are showing in the sample is bad, but there are some alternatives.

You can create a type that represents the car kind, for example, minimally

enum CarKind { Default, Fancy };

Then have a simple constructor in Car that take the CarKind, so clients do

new Car ( CarKind.Default )

instead of CarFactory.createDefault ().

The advantage of this is that you can treat the kind of car to be created as a value that can be stored or passed around, whereas with the CarFactory method, you might find clients building their own mechanism for that (such as using a boolean to decide whether to call CarFactory. createDefault or createFancy.

This reaches more in the direction of the builder pattern than the factory pattern. The idea of the builder pattern is that you have an object that represents what you want to build (e.g. the Car), and it can be manipulated (passed around, modified), and eventually invoked to generate the Car of choice.

I think this is more what you are after than the factory pattern, since as you say, you're not returning different subtypes. The purpose of the builder pattern is specifically to address large numbers of parameters or options in constructors, which you are hinting at in your code example.

(Still, you can combine factory and builder too, so, for example, a builder object could construct and return different subtypes as needed.)

Answer 3

Are they a smell? No, they're not. They make our code better by separating the responsibilities of creating an object and of using it.

I don't know if your language has the concept of "using" a resource, then automatically disposing of a resource, but it can hamper testability.

Consider the following code from a service implementation.

public DomainObject Get(int id)
{
     using (var dbContext = new DbContext())
     {
         return dbContext.Get(id).ToDomainObj();
     }
}

We want to keep the scope of DbContext small, but now we can't easily test the service or swap out the DbContext without modifying each and every using statement in the service. We have two options.

1. Inject the DbContext, make its lifespan bigger, and push the responsibility of releasing it up a layer.
2. Create a simple factory like your sample, inject it, and successfully decouple our persistence from the service by decoupling the creation of these objects.

---

public DomainObject Get(int id)
{
     using (var dbContext = this.factory.Create())
     {
         return dbContext.Get(id).ToDomainObj();
     }
}

Now, this can be cleaned up a bit because Funcs and Delegates can be used as simple factories. Instead of having a class (or bunch of classes) with a single method, you can just pass a delegate into the service's constructor.

 public Service(Func<DbContext> factory)
 {
      this.CreateDbContext = factory;
 }

public DomainObject Get(int id)
{
     using (var dbContext = this.CreateDbContext())
     {
         return dbContext.Get(id).ToDomainObj();
     }
}

Where you create the service like so.

var service = new Service(()=> new DbContext());

---

I apologize for all the C# examples. I don't know how well this applies to your particular language, but I hope it goes to show that the kind of factories you're speaking of aren't really a smell. They actually make our code better and more flexible.

Answer 4

I think you alluded to the answer of when it's bad:

these kinds of factories usually don't do runtime decision making

In my opinion, factories should generally only be used when you don't know at compile time what you need to be creating.

While the factory example you gave might actually serve a useful purpose in whatever context its being used for, I personally would give it some other name so that I avoid mistaking it with an actual factory with an actually useful purpose.