2

I had a discussion with a colleague and we don't seem to be able to agree. Can someone help me understand his arguments?

Initial situation:

We have an adapter in our application which queries other internal services and constructs out of those request an object for our service. Currently every time we want to use this adapter we have to pass the adapters to communicate with those services into the constructor. Creating and using our adapter looks something like this (we use a builder):

UserAdapter UserAdapter = UserAdapter
    .builder()
    .amisAdapter(
        (AMISAdapter) AdapterFactory.getInstance(
            ServiceConfiguration.AMIS.code()
        ).getAdapter(AdapterType.AM)
    ).build()
;

LocationAdapter locationAdapter = LocationAdapter
    .builder()
    .bdisAdapter(
        (BDISAdapter) AdapterFactory.getInstance(
            ServiceConfiguration.BDIS.code()
        ).getAdapter(AdapterType.BD)
    ).businessPartnerAdapter(businessPartnerAdapter)
    .build()
;


//Create our custom adapter and inject the adapter dependencies
OurCustomAdapter ourCustomAdapter = OurCustomAdapter
    .builder()
    .lnisAdapter(
        (LNISAdapter) AdapterFactory.getInstance(
            ServiceConfiguration.LNIS.code()
        ).getAdapter(AdapterType.LN)
    ).UserAdapter(UserAdapter)
    .locationAdapter(locationAdapter)
    .build()
;


//Query services with given adapters and construct our custom domain entity
ourCustomAdapter.getCustomDomainObject();

This exact same code 1:1 copied appears at the moment a total of 4 times. And this new service is in its early development; in the end we will use this adapter probably at dozens of places. Every time we want to use this adapter we currently have to copy this exact code. 12 lines of identical code every time we want to create the adapter...

My solution

I thought this cant be good and created a "factoryMethod" or however you want to call it. Basically there is a static method (createNew()) on OurCustomAdapter which will create a new instance of our adapter. My idea was to prevent all this duplicate code. Instead of all this code above creating and using our adapter would look like this:

//Query services with given adapters and construct our custom domain entity
OurCustomAdapter.createNew().getCustomDomainObject();

Response from colleague:

My PR was then blocked because: "We wanted to keep this class as decoupled as possible. Therefore, we used dependency injection. Now it’s not only coupled to what’s needed for minimum, but to AdapterFactory, AdapterType, and ServiceConfiguration as well."

When I asked what benefit this provides with an actual example, the only thing we could come up is "that it is better testable" (you can mock those dependencies).

But IMO this argument does not apply, if you really want to mock those adapters. You still can use the old constructor, it still exists.

Some alternative solutions floated around. E.g. using a factory. But they were then discarded, because if the factory uses those adapters you can't test the factory...

I would greatly appreciate some additional unbiased opinion.

Glorfindel
  • 3,167
SomeJuniorDev
  • 39

2 Answers2

4

Where you went wrong was with the name.

The name createNew() makes it seem like you've found the "one true way" to construct this object. If that were true these crazy builders wouldn't even been needed. What you need is a name that represents this way of constructing it. One that allows someone to come up some other way of constructing that's no more difficult than coming up with another name. Sorry but createNew() doesn't give anyone that freedom. It clearly says this is the only way.

As for testability, this is behavior free construction code. It don't need no stinking unit tests.= What it needs is readability, a good name, and a good place to live that isn't too domineering.

As for the coupling, well fine. Carve it out its own place to live in its own class. Then its individual dependencies wont get their stickiness on anything else. Tie that individualism to the name you give it. Just put it in a sensible package so people can find it. Done this way if someone wants to create a different factory with different dependencies they have room to do it.

Now that seems like a fair bit of work. It is. So you need to justify it. Unfortunately just spotting repeated code isn't enough justification. That's because repeated code isn't a sin. It's repeated ideas that's the sin. The difference can be spotted by thinking about change.

If a likely change were to cause a need to correct some of this repeated code but not the rest then the code is independent. It should be allowed to vary independently. That means duplication is fine. It's when change would require the same correction in every copy that you've violated the DRY principle.=

Also, before you assume that static builders are the only way to do this stuff, check out this way to separate the concrete implementation from the more abstract call sequence.= Sometimes it's nice when a builders implementation is something you can pass around.

candied_orange
  • 119,268
2

We can't see the detail on this code, but I think what you have done is show how bad the builder pattern is. Or perhaps just the mix of builder, service locator, factory and DI that seem to be in use here!

Could this not all be done with DI?

C# example, sorry I missed that this was a java question and I'm gobsmacked by how crap java is with DI

//perhaps you need named dependencies for these?
services.Register<AMISAdapter>(new AMISAdapterForWhateverCode()); 
services.Register<BDISAdapter>(new BDISAdapterForWhateverCode());
services.Register<LDISAdapter>(new LNISAdapterForWhateverCode());

//pull dependencies automatically from the previously registered classes for these
services.Register<UserAdapter>(); 
services.Register<LocationAdapter>();
services.Register<OurCustomAdapter>();

It seems that java wants you to use annotations on all your classes in both the popular DI frameworks, springboot and Guice. Obviously this is pretty crap, I have no idea why they think this is acceptable.

Pico Container seems to be better : http://picocontainer.com/introduction.html with a simple container.addComponent(type) syntax, equvilant to c#'s service.register<type>() and no annotation requirement.

Edit! I just read that the latest springboot doesnt require annotations for constructor injection.

Ewan
  • 83,178