My answer for – What is wrong with this code #02

A lot of things can be found in the snippet at “What is wrong with this code #02″. Here it is again:

[sourcecode language=”java”]
public MyObject {

private String myField;

… (imagine multiple fields) …

public GenericObject toGenericObject() {
GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();
GenericObject genericObject = genericObjectFactory.create("myObject");
genericObject.setField("myField", myField);
// imagine multiple lines using genericObject.setField("field", field)
return genericObject;
}

}
[/sourcecode]

Already many things are pointed out in the comments of the initial post.

One thing that immediately struck me is this line:
[sourcecode language=”java”]
GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();
[/sourcecode]

Don’t do this.

GenericObjectFactory implies with the getInstance to be a Singleton. Even though it is a singleton, never retrieve and instance inside your method like that.

Even though there “should be only one instance” of the GenericObjectFactory, it still is a dependency for serializing MyObject. Never hide your dependencies. It makes your code hide its intent, it’s hard to decouple, test, refactor, in short hard to maintain. The code should clearly communicate its intent and usage(s).

In my opinion, there are three (well four sort-off) options to deal with this:

1. Deliver GenericObjectFactory as a parameter (invasive in method signature, but automated refactoring tools (i.e. IntelliJ) handle this perfectly):
[sourcecode language=”java”]
public GenericObject toGenericObject(GenericObjectFactory genericObjectFactory ) {
GenericObject genericObject = genericObjectFactory.create("myObject");
[/sourcecode]

2. Inject GenericObjectFactory as a property of MyObject (invasive in constructor)
[sourcecode language=”java”]
public MyObject {

private final GenericObjectFactory genericObjectFactory;

public MyObject(final GenericObjectFactory genericObjectFactory) {
this.genericObjectFactory = genericObjectFactory;
}

private String myField;

… (imagine multiple fields) …

public GenericObject toGenericObject() {
GenericObject genericObject = genericObjectFactory.create("myObject");

[/sourcecode]

2B. If you find option two too invasive, add the default constructor again but let it call the new constructor (makes point 2 less invasive, remove this default constructor after period of time).
[sourcecode language=”java”]
public MyObject {

private final GenericObjectFactory genericObjectFactory;

public MyObject(final GenericObjectFactory genericObjectFactory) {
this.genericObjectFactory = genericObjectFactory;
}

// add this default constructor
public MyObject() {
this(GenericObjectFactory.getInstance());
}

private String myField;

… (imagine multiple fields) …

public GenericObject toGenericObject() {
GenericObject genericObject = genericObjectFactory.create("myObject");

[/sourcecode]

3. Introduce protected getInstance method in MyObject (least invasive, increases testability)
[sourcecode language=”java”]
public MyObject {

… snip …

protected GenericObjectFactory getInstance() {
return GenericObjectFactory.getInstance();
}

public GenericObject toGenericObject() {
GenericObjectFactory genericObjectFactory = getInstance();
GenericObject genericObject = genericObjectFactory.create("myObject");

}

[/sourcecode]
This is sorted by my personal preference. Although option one and option two trade places from time to time. The third option is less favorable, as you still do not expose the need for dependencies, but it does make it more testable. It is also the least invasive way to refactor, which is sometimes beneficial in legacy projects with lots of untested code.

To conclude, never hide your dependencies, if you ever do see some class using getInstance, please, refactor it. It will make your, and your fellow-programmers, life a lot easier.

Thank you!

16 thoughts on “My answer for – What is wrong with this code #02

  1. I could agree more! Our team used to do dependency injection with Spring-annotations on private fields. This made testing unnecessarily hard, up to the point that Spring was used to construct the test instances!

    Injecting dependencies either by constructor or passing them as an argument is always preferable.

    1. Do you use Mockito in your project?
      In that case testing classes that uses Spring annotations on private fields for dependency injection isn’t that hard at all and you don’t have to bootstrap Spring as well. If you want I can post a simple example.

      I do agree that constructor injection is preferable over setter injection at least if the dependencies are required.

      1. I’m interested in that example, though I am a bit reluctant about trying it. The mean reason is that you’re basically work around a design problem.

        Also, dependencies that are not ‘required’ for the class to function, should probably be passed to functions instead. And if these functions then become loosely coupled from the class, they are perhaps at the wrong class? 🙂

  2. You can see it also in another perspective. You benefit the strengths of the framework, in this case Spring, and you don’t have to write boilerplate code (setters).

    I totally agree that dependencies that are not required should be passed to functions.

    1. The problem with this is though, your framework != design. Just because you can set fields on private methods using a framework (which uses reflection for setting them anyway), does not mean there is something wrong in your design.

      Adding setters would probably expose more of your design flaw, as you said, if dependencies are required, they perhaps should be put in the constructor instead (and should be final).

      Nowadays I barely write setters, most dependencies go by constructor now 🙂

      1. I do think that the framework that you use will force you in a particular design though. Your design looks totally different when you make the same application in Spring or in Play.

        1. Some frameworks will force you yes, but I believe Spring does not force you to not have final fields and default constructors. I believe the frameworks should be help me write the code I want, rather than the other way around 🙂

          You can perfectly @Autowire classes with required fields. Design wise preferable, and Spring can do it, so thats nice.

          Are you going to blog about Play! some day btw? About the usage of the framework? Rather than the default ‘crud like’ examples?

      2. Spring does not prevent you making the dependencies final. Play however is a complete different story though. I will write a blog in the near future about that kind of stuff in Play, but if you stick to your arguments you will not use Play 😉

        Since dependency injection and the use of the annotations is a final JSR spec we have decided to write less boilerplate code (constructor or setter injection) and use the annotations on the fields. 😉

        1. Its funny how annotations somehow make it ‘ok’ to inject required dependencies later. I think there is a serious design flaw that cannot be solved by annotations.

          I look forward to you blog about Play, I’m interested in how things are working there and how my image of good design would fit in there.. I might learn a thing or two 🙂

      3. You can, but Play doesn’t have dependency support by default. You should use Guice or Spring if you want to support DI. In Scala you can take a look at the Cake Pattern.

        1. The only ‘drawback’ is that you probably need to get beans/instances from the context (ie Spring’s ApplicationContext) and go from there. It also makes it look more like the code example in this blog post. Not sure if it is really something I would be happy about. I’ll take a look at this Cake Pattern.

  3. @Controller
    public class SimpleController {

    @Resource
    private MyDependency myDependency;

    @RequestMapping(value = “/”, method = RequestMethod.GET)
    public String index() {
    // Delegate stuff to dependencies
    return “view-name”;
    }
    }

    @Service
    public class MyDependency {

    public void execute() {
    // Do some magic
    }
    }

    @RunWith(MockitoJUnitRunner.class)
    public class SimpleControllerTest {

    @Mock
    private MyDependency myDependency;

    @InjectMocks
    private SimpleController simpleController = new SimpleController();

    @Test
    public void shouldTestSomething() {
    doNothing().when(myDependency).execute();
    // Rest of the test
    }
    }

    1. Ah yes, the @InjectMocks annotation. Cool that it works with Spring annotations!

      Though even better would be to instantiate the Controller with the dependencies given through the constructor. You’re still hiding your required dependencies this way.

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.