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.

12
  • 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, 2013 at 19:25
  • So, are you advocating against static utilities in general, or just static factories?
    – bstempi
    Nov 20, 2013 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, 2013 at 19:42
  • 2
    Shouldn't your singleton factory have a private constructor and a getInstance() method? Sorry, incorrigible nit-picker!
    – TMN
    Nov 20, 2013 at 19:47
  • 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... :) Nov 21, 2013 at 10:25

3 Answers 3

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.

10
  • 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, 2013 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, 2013 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, 2013 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. Nov 21, 2013 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, 2013 at 15:22
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?
14
  • 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, 2013 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, 2013 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. Nov 20, 2013 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, 2013 at 19:56
  • @bstempi I was going for a bit of Socratic method there. I guess I suck at it.
    – user39685
    Nov 20, 2013 at 20:03
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?

6
  • 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, 2013 at 5: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, 2013 at 5: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. Nov 21, 2013 at 12:08
  • Also, you might want to check out this link about the problems with static methods (even the Math ones!) Nov 21, 2013 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, 2013 at 22:20

Your Answer

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

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