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.

2
  • You might get some initial downvotes because some people might think this question is "opinion based", but it's actually a good question and an oportunity to get some deeper insight into software engineering, so if that happens, please don't delete it. I also plan to contribute an answer later on, with a slightly different angle than the existing one. Jan 1, 2023 at 13:53
  • @FilipMilovanović thank you for your kind words. And also thanks in advance for your planned answer. Jan 1, 2023 at 20:28

2 Answers 2

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.

9
  • "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." This makes sense, but I think this is the one way to create the object. There are no other alternative sources to fetch the data, and probably never will be (and if for some reason our company decides to replace one of those adapters, I will be happy that I only have to change this one method). Afaik the only reason that we use this builder and do not simply create the dependencies internally is for unit testing. Jan 1, 2023 at 12:12
  • 1
    @SomeJuniorDev Edited to fix 3rd link. Thanks for catching that. I'll admit coming up with names for things when you don't see what their siblings are going to be is difficult. That's where terrible names like "defaultX" and "xImple" come from. Try to capture the reason someone would chose to call this method so it reads well in it's using code. A good name doesn't tell you what something contains. It tells you what kinds of things belong and don't belong inside. It tells you what you can expect because you called it. Jan 1, 2023 at 23:10
  • 1
    And yes, it may turn out that you never see any other way to construct this beast. But they've made clear that dependencies are a concern. So making a separate class whos only job is to take on that burden gives them the loose coupling they insisted on. Your construction is still high up the call stack where it belongs and you aren't taking on a privileged place. Jan 1, 2023 at 23:24
  • 1
    A classic software pattern is to parameterize. But the classic way to manage parameterized methods is to call them with methods that construct those parameters and themselves have few parameters. In the world of dependency injection they call that place the composition root. We're just making you a mini one that you can reuse. Jan 1, 2023 at 23:32
  • 1
    @SomeJuniorDev We're doing it simply because they don't want it's particular dependencies hard wired into the class it's constructing. If you like static methods so much you could make this into one. The point was to move it out of the file. The new class just gives it somewhere to live. Personally I like the abstract factory that you can pass around and control what's being built that way. You need some way to keep the class we're building from knowing how it's built. Or your right back to being coupled. Jan 2, 2023 at 16:09
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.

5
  • Agree. Much of the misery is because getInstance(...) is not helping the generics engine. Jan 2, 2023 at 2:13
  • Did I understand the services class correct: Basically we register on the services class all the adapters we need for our custom adapter. And then when we request OurCustomAdapter this services class will build it for us correct? So instead of a static method createNew() we now have a class which does the creation of the adapter. Wouldn't this still mean that every time I want to use OurCustomAdapter I still have to manually create all the other adapters, pass them to the services class and only then I can create our adapter? What is the benefit of this over the static createNew() method? Jan 2, 2023 at 13:36
  • @ThorbjørnRavnAndersen Could you explain to me what you mean with "... is not helping the generics engine." What is the generics engine? I cannot find anything on google about it. Jan 2, 2023 at 13:38
  • Generics is the way Java was extended to avoid typecasting from Object all the time (because that caused many run time errors). When you see List<String> generics is the part of the compiler that understands that the list contains strings instead of objects. Your code must cast the return value from getInstance instead which is the technical reason why this library code is so gnarly. Jan 2, 2023 at 14:05
  • 1
    @SomeJuniorDev sorry, A: i missed that this was java and. B: i didnt realise java is so behind the times when it comes to DI!! so my example is c#. however Guice is a basically the same ; github.com/google/guice/wiki/Motivation
    – Ewan
    Jan 2, 2023 at 16:44

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.