6

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?

4
  • 3
    Declaring something a code smell without a justification is meaningless. Jun 22, 2016 at 22:15
  • I was confused aswell. It was one of these vague oneliners that get a lot of upvotes without answering for real. Jun 22, 2016 at 22:21
  • It would be difficult to evaluate such a statement without a justification. We don't know what he was thinking. Jun 22, 2016 at 22:24
  • I will try to find the post in question. Jun 22, 2016 at 22:26

4 Answers 4

10

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.

3
  • I actually never thought about factories making testing more complex, nice insight. Since i started reading up on software design i feel like the best answer always is not to choose the best solution, but the least poor :) Jun 22, 2016 at 23:17
  • Or at least the most simple solution that is consistent with your software requirements. Jun 22, 2016 at 23:23
  • 1
    @LucaFülbier I wasn't making an argument about the complexity of testing with factories, my point is they can help make errors easier to find by making it more clear when testing object creation and tests that need those objects.
    – user22815
    Jun 23, 2016 at 13:29
5

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

2
  • Your enum approach pretty much is the factory method pattern, right? I actually like that one - I would make it a method instead of a constructor call tho. The only thing that bugs me is that i have to clutter my class with all of that switch/case and constructor code. Jun 22, 2016 at 23:24
  • (1) It is a factory of sorts, but it has something that the factory pattern doesn't have which is that is reifies some notion of the "what to create" as its own entity, more like the builder pattern (which reifies the intent to create as well as merely what to create). (2) That is sort of the nature of the builder mechanism, as it is effectively doing translation between class hierarchies (we can use a class hierarchy within the the builder itself to remove that, but then you might need a builder or factory for the builder, so that could be worth it or just plain too much, depending).
    – Erik Eidt
    Jun 23, 2016 at 0:28
2

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.

3
  • 1
    I am not programming in C#, but since this was a general question it was a nice read. Interesting idea to use delegates as a factory stand-in. I probably wouldn't do that in Java, but that's mostly because it feels like it belongs in a more functional environment (i like going that route in Python, makes short scripts even shorter). Jun 22, 2016 at 23:55
  • I'm glad you found value in it @LucaFülbier. I was hesitant to answer because I don't know anything about Java's delegates or lambdas.
    – RubberDuck
    Jun 22, 2016 at 23:58
  • I think Java has no in-built delegation mechanism and only supports lambdas since Java 1.8. So i wouldn't worry too much about it. Jun 23, 2016 at 12:33
1

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.

1
  • Any spontanuous idea for a name? The Builder pattern comes to mind (see Erik's answer), but that's a bit more special than a static creation class. Jun 22, 2016 at 23:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.