28

In some of my code, I have a static factory similar to this:

public class SomeFactory {

    // Static class
    private SomeFactory() {...}

    public static Foo createFoo() {...}

    public static Foo createFooerFoo() {...}
}

During a code review, it was proposed that this should be a singleton and injected. So, it should look like this:

public class SomeFactory {

    public SomeFactory() {}

    public Foo createFoo() {...}

    public Foo createFooerFoo() {...}
}

A few things to highlight:

  • Both factories are stateless.
  • The only difference between the methods are their scopes (instance vs static). The implementations are the same.
  • Foo is a bean that does not have an interface.

The arguments I had for going static was:

  • The class is stateless, therefore doesn't need to be instantiated
  • It seems more natural to be able to call a static method than to have to instantiate a factory

The arguments for the factory as a singleton was:

  • Its good to inject everything
  • Despite the statelessness of the factory, testing is easier with injection (easy to mock)
  • It should be mocked when testing the consumer

I have some serious issues with the singleton approach since it seems to suggest that no methods should ever be static. It also seems to suggest that utilities such as StringUtils should be wrapped and injected, which seems silly. Lastly, it implies that I'll need to mock the factory at some point, which doesn't seem right. I can't think of when I'd need to mock the factory.

What does the community think? While I don't like the singleton approach, I don't seem to have a terribly strong argument against it.

bstempi
  • 457
  • 1
  • 4
  • 11
  • 2
    The factory is being used by _something_. To test that _thing_ in isolation you need to provide it a mock of your factory. If you don't mock your factory then a bug there can cause a failed unit test when it shouldn't. – Mike Nov 20 '13 at 19:25
  • So, are you advocating against static utilities in general, or just static factories? – bstempi Nov 20 '13 at 19:29
  • 1
    I'm advocating against them in general. In C# for example the `DateTime` and `File` classes are notoriously hard to test for exactly the same reasons. If you have a class for example that sets the `Created` date to `DateTime.Now` in the constructor how do you go about creating a unit test with two of these objects that were created 5 minutes apart? What about years apart? You really can't do so (without a lot of work). – Mike Nov 20 '13 at 19:42
  • 2
    Shouldn't your singleton factory have a `private` constructor and a `getInstance()` method? Sorry, incorrigible nit-picker! – TMN Nov 20 '13 at 19:47
  • @TMN Because this particular group of people leave it to the DI framework to handle instantiation, so they don't really do things like making constructors private. I do agree with you, though. – bstempi Nov 20 '13 at 19:54
  • @bstempi: Fair enough, I withdraw my objection (although I'll leave the comment to prevent confusion). – TMN Nov 20 '13 at 20:01
  • I think you've misunderstood that group's use of lowercase singleton (one single instance is created, not a Class that idiotically enforces that there can only be one instance). I find it literally impossible to believe that they'd be as insistent on proper dependency injection as you've described, yet ok with the anti-pattern of capital S Singleton. – Amy Blankenship Nov 21 '13 at 04:28
  • You want some strong arguments against singleton? There you go: http://jalf.dk/blog/2010/03/singletons-solving-problems-you-didnt-know-you-never-had-since-1995/ I'm curious what your reviewer thinks a singleton would bring to the table that you don't already have with a static object. More often than not, a singleton makes your design *worse*. – Julien Guertault Nov 21 '13 at 07:01
  • I forgot the SO discussion the link was from: http://stackoverflow.com/questions/4074154/when-should-the-singleton-pattern-not-be-used-besides-the-obvious – Julien Guertault Nov 21 '13 at 07:02
  • 1
    @Mike - Agreed - I've often used a DateTimeProvider class which trivially returns DateTime.Now. You can then swap it for a mock which returns a pre-determined time, in your tests. It is extra work passing it to all constructors - the biggest headache is when coworkers don't buy in to it 100% and slip explicit references in to DateTime.Now out of laziness... :) – Julia Hayward Nov 21 '13 at 10:25
  • @JulienGuertault The goal wasn't to find an argument against singletons in general, but rather to compare and contrast their use in this context. The article has some interesting points, but I'm not sure that they apply here. – bstempi Nov 21 '13 at 22:44
  • @bstempi: true, I'm a bit off topic here. The point was simply that if static are bad design (good point by Mike), singleton would only make it worse. That said, the simplicity of the chosen answer makes the argument moot. :) – Julien Guertault Nov 22 '13 at 08:37

3 Answers3

21

Why would you separate your factory from the object-type it creates?

public class Foo {

    // Private constructor
    private Foo(...) {...}

    public static Foo of(...) { return new Foo(...); }
}

This is what Joshua Bloch describes as his Item 1 on page 5 of his book, "Effective Java." Java is verbose enough without adding extra classes and singletons and whatnot. There are some instances where a separate factory class makes sense, but they are few and far between.

To paraphrase Joshua Bloch's Item 1, unlike constructors, static factory methods can:

  • Have names that can describe specific kinds of object creation (Bloch uses probablePrime(int, int, Random) as an example)
  • Return an existing object (think: flyweight)
  • Return a sub-type of the original class or an interface it implements
  • Reduce verbosity (of specifying type parameters - see Bloch for example)

Disadvantages:

  • Classes without a public or protected constructor cannot be subclassed (but you can use protected constructors and/or Item 16: favor composition over inheritence)
  • Static factory methods do not stand out from other static methods (use a standard name like of() or valueOf())

Really, you should take Bloch's book to your code reviewer and see if the two of you can agree to Bloch's style.

GlenPeterson
  • 14,890
  • 6
  • 47
  • 75
  • I've thought about this, too, and I think I like it. I don't recall why I separated them in the first place. – bstempi Nov 20 '13 at 20:46
  • Generally speaking, we do agree on Bloch's style. Ultimately, we're going to move our factory methods into the class that they're instantiating. The only difference between our solution and yours is that the constructor will remain public so that people may still directly instantiate the class. Thanks for your answer! – bstempi Nov 20 '13 at 21:04
  • But...that's horrible. You most likely want a class that implements IFoo, and have it passed in. Using this pattern, that's no longer possible; you have to hardcode IFoo to mean Foo in order to get instances. If you have IFooFactory that contains a IFoo create(), the client code doesn't need to know the implementation detail of which concrete Foo was chosen as IFoo. – Wilbert Nov 21 '13 at 14:29
  • @Wilbert `public static IFoo of(...) { return new Foo(...); }` What's the problem? Bloch lists satisfying your concern as one of the advantages of this design (second bullet point above). I must not be understanding your comment. – GlenPeterson Nov 21 '13 at 15:03
  • In order to create an instance, this design expects: Foo.of(...). However, the existence of Foo should never be exposed, only IFoo should be known (as Foo is but _a_ concrete impl of IFoo). Having a static factory method on the concrete impl breaks this and leads to a coupling between the client code and the concrete implementation. – Wilbert Nov 21 '13 at 15:22
  • @Wilbert There is no `IFoo`. Foo is a bean that doesn't have an interface. – bstempi Nov 21 '13 at 22:16
  • The question was updated to reflect this. My mistake for not detailing `Foo` a bit more. – bstempi Nov 21 '13 at 22:18
  • @Wilbert To create an IFoo or Foo, you have to expose and clients have to couple to a factory or constructor of some kind. If you don't want to expose the class Foo outside your api, and Bar is a public API class, just provide `public IFoo Bar.constructFooishThing(...) { return Foo.of(...); }` Maybe if Bar is a builder for IFoo-ish stuff, then the factory belongs on Bar. It can happen. That's the exception rather than the rule for the kind of stuff I tend to work on. Keep it simple unless you need a specific complication. What am I missing? – GlenPeterson Nov 22 '13 at 01:16
  • How would you unittest classes using Foo, without depending on the implementation of Foo? If you inject a factory in such a class Baz, you could inject a mock for the factory that returns mock objects for Foo or IFoo. But what to do using this pattern? – Dr. Hans-Peter Störr Dec 02 '13 at 09:08
  • @GlenPeterson It hardcodes the use of that specific Foo implementation into all client code. That's a bad thing, because it forces tight coupling where there wouldn't be without this pattern. – Wilbert Dec 02 '13 at 09:41
5

Just off the top of my head, here are some of the problems with statics in Java:

  • static methods don't play by the OOP "rules". You can't force a class to implement specific static methods (as far as I know). So you can't have multiple classes implementing the same set of static methods with the same signatures. So you're stuck with one of whatever it is you're doing. You can't really subclass a static class, either. So no polymorphism, etc.

  • since methods aren't objects, static methods can't be passed around and they can't be used (without doing a ton of boilerplate coding), except by code that is hard-linked to them -- which makes the client code highly coupled. (To be fair, this isn't solely the fault of statics).

  • statics fields are pretty similar to globals


You mentioned:

I have some serious issues with the singleton approach since it seems to suggest that no methods should ever be static. It also seems to suggest that utilities such as StringUtils should be wrapped and injected, which seems silly. Lastly, it implies that I'll need to mock the factory at some point, which doesn't seem right. I can't think of when I'd need to mock the factory.

Which makes me curious to ask you:

  • what, in your opinion, are the advantages of static methods?
  • do you believe that StringUtils is correctly designed and implemented?
  • why do you think you'll never need to mock the factory?
  • Hey, thanks for the answer! In the order that you asked: (1) The advantages to static methods are syntactic sugar and the lack of an unnecessary instance. It's nice to read code that has a static import and to say something like `Foo f = createFoo();` rather than to have a named instance. The instance itself provides no utility. (2) I think so. It was designed to overcome the fact that many of those methods don't exist within the `String` class. Replacing something as fundamental as `String` isn't an option, so utils are the way to go. (cont'd) – bstempi Nov 20 '13 at 19:45
  • They're easy to use and don't require you to cary around any instances. They feel natural. (3) Because my factory is very similar to the factory methods within `Integer`. They're very simple, have very little logic, and are there only to provide convenience (eg, there's no other OO reason to have them). The main part of my argument is that they rely on no state -- there's no reason to isolate them during tests. People don't mock `StringUtils` when testing -- why would they need to mock this simple convenience factory? – bstempi Nov 20 '13 at 19:53
  • 3
    They don't mock `StringUtils` because there is a reasonable assurance that the methods there don't have any side-effects. – Robert Harvey Nov 20 '13 at 19:54
  • @RobertHarvey I suppose I'm making the same claim about my factory -- it doesn't modify anything. Just like `StringUtils` returns some result without altering the inputs, my factory also modifies nothing. – bstempi Nov 20 '13 at 19:56
  • @bstempi I was going for a bit of Socratic method there. I guess I suck at it. –  Nov 20 '13 at 20:03
  • @bstempi but it seems you've missed my points about coupling and passing methods around. –  Nov 20 '13 at 20:05
  • @MattFenwick I didn't miss those points -- they simply didn't play a part in my answers, so there was no need to address them. I agree that using a static class is less modular than something that is injectable. For this, I don't think it matters. `SomeFactory` is packaged along with `Foo`, so I can be assured that it isn't going anywhere, and therefore I'm ok with coupling with it. I do the same thing all the time with the `Collections` class -- it's coupled, but I think it's ok. – bstempi Nov 20 '13 at 20:19
  • @bstempi why do you think they didn't play a part in your answers? Looks to me like they did. Why do you think they would say something like "it's good to inject everything"? –  Nov 20 '13 at 20:28
  • @MattFenwick Most of the discussion revolved around testing. While coupling did come up, we also use `StringUtils` et al unwrapped, so the point was largely dropped. Testing was definitely the focus of injecting. – bstempi Nov 20 '13 at 20:38
  • @bstempi I'm sorry, but I can't help feeling that you're moving the goalposts (again, I apologize if you're not). If those extra details are important to the question, I would recommend adding them to the OP. –  Nov 20 '13 at 20:46
  • @bstempi it seems we simply don't agree on the points I've made -- I guess I just place more importance on coupling and higher-order code than do you. Which is not to say in any way that you're wrong. Just that we disagree. –  Nov 20 '13 at 20:48
  • The question wasn't, "Which is more testable?" The question was, "Which is more favorable?" If you wish to use testability as part of your answer, be my guest. Also, I did mention mocking as an advantage of the singleton, so the only extra bit my last comment gave you was how we came to that point in our discussion. – bstempi Nov 20 '13 at 20:49
  • @bstempi I have to apologize, but I no longer think I get what you're asking -- and I haven't mentioned testability a single time. ... ??? Given the paragraph I quoted, I thought your question was something along the lines of what are some of the issues involved with using static methods in a factory; I have tried to answer that. Again, sorry if that's not what you're looking for. –  Nov 20 '13 at 21:08
  • let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/11609/discussion-between-bstempi-and-matt-fenwick) – bstempi Nov 20 '13 at 21:17
0

I think the Application you're building must be fairly uncomplicated, or else you're relatively early in its lifecycle. The only other scenario I can think of where you wouldn't have already run into problems with your statics is if you've managed to limit the other parts of your code that know about your Factory to one or two places.

Imagine that your Application evolves and now in some instances you need to produce a FooBar (subclass of Foo). Now you need to go through all places in your code that know about your SomeFactory and write some kind of logic that looks at someCondition and calls SomeFactory or SomeOtherFactory. Actually, looking at your example code, it's worse than that. You plan on just adding method after method for creating different Foos and all your client code has to check a condition before figuring out which Foo to make. Which means all clients need to have an intimate knowledge of how your Factory works and they all need to change whenever a new Foo is needed (or removed!).

Now imagine if you just created an IFooFactory that makes an IFoo. Now, producing a different subclass or even a completely different implementation is child's play--you just feed in the right IFooFactory higher up the chain and then when the client gets it, it calls gimmeAFoo() and will get the right foo because it got the right Factory.

You might be wondering how this plays out in production code. To give you one example from the codebase I'm working on that's starting to level out after just over a year of cranking out projects, I have a bunch of Factories that are used to create Question data objects (they're something like Presentation Models). I have a QuestionFactoryMap that's basically a hash for looking up which question factory to use when. So to create an Application that asks different questions, I put different Factories into the map.

Questions can be presented in either Instructions or Exercises (which both implement IContentBlock), so each InstructionsFactory or ExerciseFactory will get a copy of the QuestionFactoryMap. Then, the various Instructions and Exercise factories are handed to the ContentBlockBuilder which again maintains a hash of which to use when. And then when the XML is loaded in, the ContentBlockBuilder calls up the Exercise or Instructions Factory out of the hash and asks for it to createContent(), and then the Factory delegates the building of the questions to whatever it pulls out of its internal QuestionFactoryMap based on what is in each XML node vs. the hash. Unfortunately, that thing is a BaseQuestion instead of an IQuestion, but that just goes to show that even when you're being careful about design you can make mistakes that can haunt you down the line.

Your gut is telling you that these statics will hurt you, and there's certainly plenty out there in the literature to support your gut. You will never lose by having Dependency Injection that you wind up only using for your Unit Tests (not that I believe that if you build good code that you won't find yourself using it more than that), but statics can completely destroy your ability to quickly and easily modify your code. So why even go there?

Amy Blankenship
  • 805
  • 5
  • 7
  • Think for a moment of the factory methods in the `Integer` class -- simple things like converting from a String. That's what `Foo` does. My `Foo` is not meant to be extended (my fault for not stating this), so I don't have your polymorphic issues. I understand the value of having polymorphic factories and have used them in the past...I just don't think they're appropriate here. Could you imagine if you have to instantiate a factory to do the simple operations on `Integer` that are currently baked-in via static factories? – bstempi Nov 21 '13 at 05:23
  • Also, you assumptions about the application are pretty off. The application is fairly involved. This `Foo`, however, is much simpler than many of the classes. It's just a simple POJO. – bstempi Nov 21 '13 at 05:25
  • If Foo is meant to be extended, then that right there is a reason for the Factory to be an instance--you provide the correct instance of the factory to give you the correct Subclass rather than calling `if (this) then {makeThisFoo()} else {makeThatFoo()}` all through your code. One thing that I've noticed about people with chronically bad architecture is that they're like people in chronic pain. They don't actually know what it feels like not to be in pain, so the pain they're in doesn't seem all that bad. – Amy Blankenship Nov 21 '13 at 12:08
  • Also, you might want to check out [this link](http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/) about the problems with static methods (even the Math ones!) – Amy Blankenship Nov 21 '13 at 12:15
  • I've updated the question. You and one other person mentioned extending `Foo`. Rightfully so -- I never declared it final. `Foo` is a bean that's not meant to be extended. – bstempi Nov 21 '13 at 22:20
  • I agree with the article, but only partially. If the method is static, contains no state, and does not modify it's arguments, then there's nothing to mock. It's argument are it's injections. I don't see a testability issue so long as those conditions are true. – bstempi Nov 21 '13 at 22:26