Field Injection - Is it a bad habit? [closed]

Question

My colleague and me have a dispute going on for some weeks now. And I wanted to hear the opinion of the community on this.

We use guice in our application. In the UI part of this application I prefer to instantiate all fields and components via DI and custom annotations (aka as place in for a factory) and wire them up in a buildUI method.

That looks as follows:

My Approach:

@Inject
@PreConfigured(caption="foo", width="100%")
Label field;

private void buildUI() {
   this.addComponent(field);
}

I wrote also some annotations for data binding and i18n.

My colleague does it this way:

His Approach:

private Label field;

private void buildUI() {
   field = new Label();
   field.setCaption("foo");
   field.setWidth("100%");
   this.addComponent(field);
}

The code here can be very long. Considering data binding and other topics, the second code can become even longer, whereas my approach is just another annotation.

My reasons are:

1. Central place to configure behaviour and appearance of all components
2. Code reuse
3. Performance - in a long running application the initialization code of components gets "hot" and the jit does more optimization on it.
4. More readable.

His reasons are:

1. Readability
2. A views code is not scattered all over the place.
3. The class works without DI
4. Flexibility of dependency injection is overrated, especially if concrete classes are injected.
5. The only valid way of using DI is constructor injection.

But I read also some articles from Fowler, where he states, that setter injection (substitute with field injection) is a bad design decision. But what about views, which don't have any reuse in other projects?

Is Field Injection a bad design? Or is the other way presented above more elegant?

Kind regards Christian

PS: I know the discussion started merely opinion based, but it is a long ongoing discussion (setter vs constructor injection), which is probably worth to let it go.

Answer 1

Disclaimer: since this is kind of a subjective question, I'll also make some subjective claims in this answer, based on personal preference. Here are my two cents:

Both approaches look bad to me.

Yours is bad because:

• You have all bindings in a module. Think how large can this module grow, specially if there are lots of screens. Think about the mantainability implications of this: Now a programmer wanting to change a single screen will have to

  1- Know the DI framework
  2- Find the module where the bindings for this screen are defined.
  3- Scroll down to find the binding.
  4- Make the change in the module, maybe also in the screen.
  5- Commit changes for the module class, maybe also for the screen class.
  

• Using DI means using reflection, which is going to be slower than not using it.

• Makes your code depend on a 3rd party code (the DI framework)
• Part of the layout is still hardcoded in the screen (what are the container views and what do they contain), and part is in another module. The single-responsibility principle is broken.
• Annotations are "dirty": Now the source files are written in two different languages. It is less readable and there can also be issues when using automatic code analysis tools.
• Hard to write unit tests (and btw the tests should use the DI framework to run, which is a bad thing, and slower specially if there are a lots of tests).

Your colleage's approach is of course flawed, since it contains hard dependencies. This might be acceptable for screens, though. I think DI is ok for big things (like injecting a service with a parser implementation, injecting controllers, daos, loggers), but I won't use it for views, and I'd rather stick to constructor injection. So (in Android) I'd just define the views in xml and code the view-controller part in the Activity class, which is the way you are supposed to do it.

Answer 2

I think that your question is not really suited for StackOverflow (check FAQ) but I can present you my ideas about it.

I support your idea in this problem because it seems that you are already using DI in this project so why would you not use the functionality it provides?

My remarks to your reasons:

1. I prefer using java code for all configuration if possible (because it can be easily refactored). If java code is too verbose I prefer annotations (this is your case) and I would do anything to avoid xml/yaml/any other markup language configuration.
2. You can actually reuse your annotation which simplifies a lot of boilerplate code into a succint and readable annotation. The hard-coded values are not reusable but reusability is not applicabe here anyway
3. I did not benchmark your code (since I don't have the codebase), but using annotations often involves reflection which is definitely slower than just calling setters. So I think that if this becomes a performance bottleneck you'll scratch your head what to do with the parts involving reflection
4. Definitely. I mean compare your example with his. No sane person will say that reading 4 lines of code full of syntax will be more readable than @PreConfigured(caption="foo", width="100%")

His points:

1. Explained it in 4.
2. What does he mean by "scattered all over the place"? An annotation is always on a field in your case but if you programmatically initialize it the code can be anywhere. That's scattered.
3. There is no point in not using DI if you already use Guice. Does he have a problem with DI?
4. This is like "name-calling". Makes no sense. Show him this essay written by Paul Graham.
5. Then think of an object which has like 20 fields. Of course having 20 fields is already a code smell but anything above 4-5 parameters is ugly. I personally prefer setter injection because it does not involve tampering private fields with reflection and it does not involve constructors with 8-10 parameters either. If you want immutable objects you can use the Builder pattern and if you want singletons you can rest assured that Spring's (I think Guice has the same concept) singleton scope is fine for you. And by the way this is common sense: if it were the only valid way there would be no other way, right?