An Annotation Nightmare

20 Dec 2013 
@XmlElementWrapper(name="orders")
@XmlJavaTypeAdapter(OrderJaxbAdapter.class)
@XmlElements({
   @XmlElement(name="order_2",type=Order2.class),
   @XmlElement(name="old_order",type=OldOrder.class)
})
@JsonIgnore
@JsonProperty
@NotNull
@ManyToMany
@Fetch(FetchMode.SUBSELECT)
@JoinTable(
    name = "customer_order",
    joinColumns = {
        @JoinColumn(name = "customer_id", referencedColumnName = "id")
    },
    inverseJoinColumns = {
        @JoinColumn(name = "order_id", referencedColumnName = "id")
    }
)
private List orders;

Wait. What? Is this really what we have come to? I can't even see the damn property under this bloat. How did this happen? Yeah ok - we had to get rid of the old xml configuration horror somehow. But this? This is even WORSE. This class is supposed to be a goddamn pojo with a bunch of properties. Short and concise, easy to read. I, as a reader of this cass, am not interested at all how the database table is joining customers to orders. I'm neither interested how its being serialized. This is just implementation details. Reading this class, i am living in an object world and i want to know what data and behaviour the object has. Not more, not less. I don't care about column names, fetchtypes or json serialization for the moment. And i don't want to read, change or recompile this class for the sake of a tablename change. I don't want to add another annotation for storing this entity in a mongoDB neither. The entity should not have responsibility for these details. We are not only violating the Single Responsibility Principle here, we are doing a f****** responsibility party.

Ok ok, enough of the rage. How do we deal with this issue? Some duplicate the entity for various layers with different annotation purposes. They map the entity onto the next layers related entity using an automated mapper like Dozer. Some even write that mapping themselves. But this is by no means a solution. It is just replacing one code smell with another one: Duplication.

So please, focus on frameworks that don't force you to clutter your code. jOOQ is a nice solution to map database records to entities without annotations. Also, hibernate allows you to define your mappings in XML.

Private Field Injection


@Inject
private MyService myService

This is used quite often, while it shouldn't even be possible. The myService field is private, thus it's inaccessible from outside the class. Nevertheless it's possible and people do it. In reality it is a hack. The DI-framework sets the field using reflections doing setAccessible(true). You don't want hacks in your code, do you? Lets have a look at the alternatives:

Setter Injection
Well, at least its better than the private field injection, since its using a public method instead of hacking a private field. But still, ask yourself this: 'Is this class even supposed to live without the injected value?' Because if it's not, there is no reason what so ever for the class to get constructed without an instance of MyService. You want to implement this constraint on the class level and inside the constructor, not on the framework level.

Constructor Injection
This is usually the way to go. It allows you to
  • make the field immutable (there is usually no need to change it).
  • implement the constraint, that the class is not instantiatable without a given MyService in the right place.
Of course it means that you cannot inject by annotation. But why would you want to? The class doesn't need to know, if it gets injections by a DI-Container or a Factory Class. It should not know anything of this. No @Autowired, no @Qualifier. All it needs to know is its own behaviour. Everything else should be handled outside of the class.
One could use a configuration class or file for the actual injection.

A DI-Container is a usefull tool that helps you wire your classes together. Use it for this purpose, but don't let it dictate your code. Uncle Bob wrote a great post where he explained how to use DI-Frameworks without having them dictate your code.

@RunWith(SpringJUnit4ClassRunner.class) in UnitTests

Why would you need this in unittests? Because it is automatically generated by your IDE/app template? No! You want to test the behaviour of a class, living in isolation in unittests. Not if the DI-Conainer is injecting a fields accordingly. Just inject yourself in a setup method. No DI-Container needed. By the way, all this testrunner does is these 3 lines of code.

private TestContextManager testContextManager;
//..
this.testContextManager = new TestContextManager(getClass());
this.testContextManager.prepareTestInstance(this);

They are not worth blocking your only TestRunner slot. You want to keep it free for something like parameterized @RunWith(JUnitParamsRunner.class) or concurrency @RunWith(ConcurrentJunitRunner.class) tests.

tl;dr
Annotations have become more harmful than helpful these days. We should get back to pojos and focus on keeping our code as clutterless and framework-agnostic as possible to make it more readable and reuseable. Don't let frameworks dictate your codebases, since they should be exchangable tools. Beware of what a class should know, and what not. Some annotations are useful, most aren't.

@DevNull({
 @SuppressWarnings
 @Autowired, 
 @Inject, 
 @Override,  
 @XmlElementWrapper,
 @XmlJavaTypeAdapter,
 @XmlElement,
 @JsonIgnore,
 @JsonProperty,
 @ManyToMany,
 @Fetch,
 @JoinTable
})
 Java  Hibernate  Clean Code  Spring

Comments

Related Posts