Warning: this blog contains ugly code requiring horizontal scroll bars. Viewer discretion advised
Yesterday, we found the following badass query in our
We looked at the above code and said huh?
So many joins!
Could it really be that hard to get this information?
No, there had to be an easier way.
My pair programmer that afternoon, Adam Sroka, and I puzzled our way into this logic and the code that calls it.
For a while, we just went in circles.
Then we got somewhere.
We sketched out some pseudo-code and agreed that it could work.
We already had good test coverage for this code, so we felt free to refactor.
We did not start by refactoring the badass query.
We began by working on the code that calls the query.
That code looked like this:
The problem with the
uniqueIdsFor(...) method is that it relied heavily on the badass query in the
We reasoned that
ViewableAlbum already knew the information that the code was working so hard to re-acquire via the query’s joins.
To try out our experiment, we first test-drove a new method on the
That new method reused the existing
getPageCompletionFor(...) method, which itself uses simple HQL (Hibernate Query Language) based criteria methods:
Now we were ready to try our experiment. So we went back to the
uniqueIdsFor(...) method and rewrote it to use a simple loop:
Yes, it performs a query for every page, yet we reasoned that since our albums have less than 150 pages, the performance would be fine (and if not, we could optimize later).
This refactoring positioned us to make some deeper design changes that were needed to continue developing this new feature we’re working on.
It felt extremely good to kill off a badass query that reeked of Conditional Complexity. Now we’ve just got a simple loop, calling a simple query.
What's the moral of this story? Don't be afraid to get rid of big hairy badass queries that someone else wrote some time ago. You are smart enough to produce simpler code!