One of the main reasons I blog is to remind myself how easily one can use software development techniques to make bad decisions. I do it myself all the time…
In a recent project, I was working with Spring Data which is able to create powerful CRUD operations over data sources if you just create an interface:
interface FooRepository extends CrudRepository<Foo, String> {
}This oversimplification shows that my Foo objects could be stored and retrieved, IDed by String .
I can call save and findById on the CrudRepository along with a bunch of other functions I wasn’t intending on using.
We hit a couple of requirements, though:
- We wanted to validate the
Fooobjects before saving them - The ID field, though inspired by one of the business fields, needed to be calculated – so we had the application set the logical ID and then calculated the actual ID from it
As Spring was going to create the actual instance of the above using its Spring Data magic, I felt that the best way to add in my extra behaviour was to drop in some default methods:
interface FooRepository extends CrudRepository<Foo, String> {
default Foo doTheRealSave(Foo foo) {
Validation.validate(foo);
foo.setId(calculateIdFrom(foo));
save(foo);
}
default Optional<Foo> findByRealId(String id) {
return findById(calculateIdFrom(id));
}
}So, I’ve added some methods which do the extra stuff I want, and call out to an ID calculator, and a validator to weave that behaviour in. These default methods are available on the object that Spring creates automagically. Clever, right!?
The worst code is clever code!
What I Did There…
I realised, after I had to rig up an awkward unit test, some of which was mocked and some of which needed these default methods… I’d created a weird side-door into the object. It was simultaneously providing two interfaces:
- The real
CrudRepositoryinterface, which anyone could call from my repo - The two special repo methods which I preferred people to call
I couldn’t stop someone from misusing this object.
I also had lumped together in a single class two unrelated rules – id construction and validation. In fairness, it wasn’t possible to construct an ID on an invalid object, but that wasn’t excuse enough.
Refactor It Then
After a chat with a colleague, who suggested the handy axiom…
If it’s important enough to spend this much time discussing if it’s right, then it’s important enough to spend a similar amount of time fixing.
… I decided to rework this.
Each of the two additional facets could be thought of as a layer the requests needed to pass through. This seemed like something that was essentially a chain of responsibility or decorator pattern.
I started the refactor, trying to work out how to construct a decorator, and of what.
A Startling Discovery
I realised, as I put my decorator together over CrudRepository, there were many other functions relating to this behaviour that I hadn’t bothered to provide for in my original hacked-together implementation.
In my haste to poke some behaviour in, I’d created an inconsistent interface. A better design forced me to notice this.
Some number of unit tests and improvements later, and the chain is complete. The validation on save was the easy part, the physical vs logical ID scheme was something that needed a more sophisticated layer, which was a good thing to have in its own component.
Yet Nobody Uses The Other Features…
The original code met all known use cases.
It was just an accident waiting to happen. A few hours of frustration caused by a partial solution forced into a hole where it didn’t belong.
By removing the side door, and making the front door do its job consistently, it’s not possible for some future unaware developer to corrupt the state of the system, or get confused around why certain edge cases don’t seem to work.
While I’d intended to create a front end for this repo, I’d actually created a situation where the most important data coming in a side-door and essentially being treated as an edge case.
Though the final code is larger, and has more design patterns in it, it’s cleaner, more extensible and doesn’t have to be explained away.
Reducing surprise through better design is always worthwhile.
| Published on Java Code Geeks with permission by Ashley Frieze, partner at our JCG program. See the original article here: A Weird Side Entrance Opinions expressed by Java Code Geeks contributors are their own. |
I suspect however that the real problem is not in your code or your desire to make this work. I suspect that the problem is that you need TWO types of identification that are in fact identical. I think that is a data design red flag. If you can calculate one ID from the other and vice versa then one of the IDs is unnecessary. Maybe you should fix that instead of retrofitting your code.
Interesting point. In this case, the biggest problem was that the physical ID, which was necessary to locate a file in S3, was sometimes too big to be used as a logical ID. It was possible to great a one-way hash that could be used as a logical ID, but for the majority of cases that made the data less human readable. The idea of the wrapper was to create the right logical ID just-in-time, allowing the outside world only to deal with the physical ID. That said, you should ask the above question whenever faced with a multiple ID… Read more »
Your article is only half finished! You never presented the solution. That’s like showing half a movie & saying the good guys win