2

I have recently started working on a Java project that is already with a sizeable codebase developed by a team over 3 months. I noticed that at many places , some objects they are instantiating directly in the constructor of the client object , rather than using a dependency injection. I wanted to refactor the object construction into a factory and use some injection framework.

I have created a factory that essentially is a one liner for a doing new <type(some params here)>. There is nothing fancy here - no singleton , no static factory pattern. Just a newInstance() method that returns a new instance of the dependency.

To show something in code :

class A {   A() {  
       B bobj = new B();  // A and B are coupled directly
    } 
}

I want to refactor this to :

BFactory {
    newInstance() {  return new B(); // return B implementation  }
}

 class A {
   A(BFactory factory){  
     B bobj = factory.newInstance(); // A does not know about B impl
  }
}

My argument is that objects should not be created anywhere in the code except in a Factory meant for that purpose. This promotes loose coupling , otherwise you are coupling the two types tightly. One senior member ( the author of the code I am trying to refactor ) feels that the one liner factory is a over-complicating design.

Are there authoritative advices/references on patterns governing this problem ? Something that can be used to decide which approach is better and why exactly ?

Improve this question
5
  • 2
    My argument would be "If it ain't broke don't fix it." The lose coupling gained by using factory methods isn't worth the time or risk (bugs) that it would require to touch every part of your code base. Just roll with it for now. Commented Jun 18, 2015 at 16:10
  • Can your post a short example of what you are talking about to make it clear? Your description could be interpreted several different ways. Commented Jun 18, 2015 at 16:12
  • 2
    I would slightly change @markspace's comment to "Wait until it is broke to fix it.". If and when you need to make a change that would be significantly simpler and cleaner with a Factory approach, refactor construction for that class. If something is working and does not need changing, leave it alone. Commented Jun 18, 2015 at 16:23
  • I don't believe this question deserves to be closed as primarily opinion based. The OP asks whether his way of doing something is the right way. I don't the answer is depends on your situation. Commented Jun 18, 2015 at 16:57
  • Use loose coupling only when you need it, if a factory can create possibly multiple implementations of that interface/class then you should use a factory to create them, if it's an object then don't make life complicated by adding a factory to it. Commented Jun 18, 2015 at 16:58

2 Answers 2

Reset to default
1

One senior member ( the author of the code I am trying to refactor ) feels that the one liner factory is a over-complicating design.

This looks like the crux of your question and not whether you should be refactoring the code or not so let us answer it rather than deviating from the actual question. If we consider the examples that you present in your code, I agree with your colleague. You shouldn't be creating a factory class for each dependency you want to inject. There is nothing wrong with what you are trying to achieve but the way you try to achieve it is an overkill.

You either depend upon a hierarchy of Factory classes that know how to create each and every dependency or you depend on the actual class itself and have a Container that can wire the objects together for you.

Option 1 : Depend on a common Factory

class A {
   B bobj;
   C cobj;
   A(Factory factory){  
     bobj = factory.createB(); 
     cobj = factory.createC();
   }
}

Option 2 : Depend on the dependency directly

class A {
   B bobj;
   C cobj;
   A(A a,B b) {  
      this.bobj = b;
      this.cobj = c
   }
}

You can then create a Container class that knows how to wire objects together :

class Container {
    public static B createB() {
        return new BImpl();
    }

    public static C createC() {
         return new CImpl();
    }

    public static A createA() {
        return newAImpl(createB(),createC());
    }
}

The examples presented above are way too basic. In the real world, you will mostly have a more complex graph of dependencies. That's where DI frameworks come in handy instead of reinventing the wheel. If your ultimate goal is to start using a DI framework, you could go with option 2 since DI frameworks achieve inversion of control by supplying dependencies to their clients rather than the client code asking for them.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

Your underlying point is perfectly valid, it's usually not a good idea to instantiate objects directly in the constructor (they may be valid exceptions from that rule).

If you do this:

class Car {

   private Engine engine;

   public Car() {
       engine = new Engine();
   }

}

You will have a hard time testing the Car without the engine. You'll have to use reflection in order to exchange the instance by a mock.

If you do the following instead

class Car {

   private Engine engine;

   @Inject
   public Car(Engine engine) {
       this.engine = engine;
   }

}

it is very easy to test the Car with a fake engine, replace the implementation of engine or change the way the engine should be constructed (e.g. add more parameters to the constructor).

But you should definitely use an established dependency injection framework instead of writing your own factories. If you pass in a "common factory" as Chetan suggested you end up hiding your dependencies.

Good resources for more motivation using dependency injection can be found here: https://github.com/google/guice/wiki/Motivation or https://youtu.be/acjvKJiOvXw (very good talk, should be worth your time).

Improve this answer

4 Comments

Why not have both types of constructors from both classes, one is default that provides an engine and then other allows you to inject an engine.
@AlexC And what happens when your DefaultEngine for one of the clients is no longer the DefaultEngine it used to be? The Factory should ideally be injecting an engine that should act as the default engine if the client does not know what engine they want. For one client, Engine1 could be the default engine and for the other, it would be Engine2. Should Car really care? The answer is no because defy the purpose of inversion of control.
A lot of what-ifs. You made up a use case and now you are making up constraints and answering your own questions. What if IoC config that contains the default is no longer what it used to be? Code can go stale and so can config files. Read my answer again, you CAN have both default behavior and IoC, what exactly is your point?
@AlexC I don't think you got my point at all. Let me rephrase. How is a default engine different from any other Engine. Why should a Car know what it's default engine is? A car should not care what engine it is given as long as it is given an engine. Different clients may want a different engine to be treated as the default engine? Do you create a new Car class for each client with a default engine? No. You create one Car class and let the clients decide what they want the default engine to be. If a car starts deciding what it's default engine is, it's breaking a lot of rules.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.