As stated in my last post, I recently started a new gig, and I'm running into many of the same Rails anti-patterns I ran into when I first started with my old team, the biggest being the rampant use of Service Objects.
To explain to you why this is bad, I first need to describe, for the uninitiated, what service objects are and the problem they attempt to solve.
A service object is a PORO (Plain Old Ruby Object) that abstracts one responsibility away from your domain model... it has one job and it's meant to do it well. The idea is to take all of your business logic out of your model, so it can remain as thin (or thinner) than your controller. In theory, this sounds like a good idea, right? Your code gets encapsulated into tidy little bite-sized, easy-to-test bits. In theory. This of course assumes that the developer working on the project adheres to the single-responsibility principle and never tries to shoe-horn in additional functionality. This is where most implementations of service objects I've encountered in the wild break down, as inevitably, the business team will request new functionality that seems small enough to just tack on as an afterthought... this adds side-effects, which leads to patching, quick-fixes, and work-arounds, until your nice little single-responsibility PORO has evolved into a lovecraftian tentacle-monster of spaghetti-code. It's difficult to read, nearly impossible to debug, and has introduced needless complexity to your application. Worse, it may even be impacting your performance. To give an example:
The app I'm currently working on generates statistics for mailing lists. This, under the current model, has a call to a microservice, which is hooked into an api, which passes the call down to a service object, which is meant to generate statistics for each mailing that is sent out by an organization.
This service object calls another service object and passes each individual mailing as a new object, to another service object to generate the statistics needed for each individual mailing. For each individual mailing, it's making individual calls to query objects (similar to service objects, but meant specifically to execute a specific query - which these were, by the way, not doing) to see which email it was delivered to, whether that email has been read, whether delivery was bounced etc... all along the way, individual calls are being made to the database for each tiny bit of information that needs to be compiled into bigger units of information. It was like a Rube Goldberg device in code form.
For a mailing that was only sent out to say, 30-50 users, the cost of this operation may have seemed almost negligible. However, when our organization started to incur larger clients who had larger lists of subscribers, lists numbering in the thousands, the page began to fail, horribly... timing out as tens of thousands of calls might need to be made to the database for each line in a ten-record paginated call. It was painful to watch, and even more painful to debug. Problem #1 I was facing was that the app was not working, but Problem #2 was that the previous developer, while adhering to this weird service object pattern, had no such qualms about domain name-spacing, or even naming routes, controllers, and models in a way that was anything close to coherent. Half the service objects were crammed in with the models, while the other half were in the services directory, and which got stuck where appeared to be arbitrary. I found myself having to start at rake routes, searching for which controller action was matched to which path, when it should have been obvious from the name. From there, I immersed myself in the foul sewage of service object after service object, objects which initiated struct after struct in an endless cycle of obfuscation by abstraction, passing huge parameter hashes from one service till the next, with no clear indicator where the actual data was coming from, and how it was being obtained. I inevitably reached the bottom... multiple query objects which needed to be called individually for each email address to which a given mailing was sent, whose results would then be tabulated and passed back up the chain. The business logic used to obtain the numbers I needed was buried so far that the business team I was working with could not tell me what data I needed to query for each statistic, and the code was so complex it wasn't doing me any favors. When I eventually deciphered how the numbers were obtained, I chucked out the lot of it.
You may think that's a bit extreme. Someone put a lot of hard work into abstracting all those bits of code away from the domain model, after all. Yeah, and they did a right good job of breaking the app, without even realizing it. Even worse, they wasted my time by creating a codebase so dense, it took me nearly a week to discern the root cause of the issue.
It's the wasting of my time I deem to be unforgivable here, in case you're wondering.
Done well, any pattern you implement should result in your codebase being easier to navigate and harder to break. If it does the opposite, you're not only wasting your time, but the time of every developer who comes after you, your business team, and your clients. You're being too smart for your own and everyone else's good, and over-engineering your solution.
So, if I'm chucking out the baby with the bathwater, what do I replace it with? Well, I looked at the base of the problem... here I have this page that needs to display a data structure we don't have readily available in the db, because it's comprised of counts of values that are stored in other database tables. Approaching this within the existing service object architecture resulted in the aforementioned thousands of database calls for each individual data-point in each individual row in a ten-row table. Rails sucks at doing this on its own, but SQL eats problems like this for lunch, and asks for seconds.
Once I knew exactly how the data-points related to each other, it was easy to create a SQL view, and an ActiveRecord class based on that view, which I could query to get the all data I needed. This cut my DB round-trips down to exactly one, and reduced my query time exponentially. I was now getting speed gains of anywhere between 50-75%. What's more, there was no complex business logic to abstract away from the model... it was read-only, with any business logic baked into the SQL view, so only needed the queries that were already provided by ActiveRecord::Base. Best of all, the page was no longer breaking every time our largest client attempted to access it.
I'm not saying that every bad service object architecture could be replaced with custom database views. I'm damn sure not saying they should, either. Doing so would be irresponsible, and would make me guilty of doing the same thing the previous dev did... blindly adhering to a practice, and implementing said practice badly. (I really don't even want to blame the previous dev for this so much... from what I was told, he only had a few months to throw all of this together, and was working on his own... and crunch-time development is a rant for another day.) There's an old adage that goes: "When you're holding a hammer, every problem looks like a nail". Developers that blindly stick to design patterns without understanding why they should be implemented, and when they should be avoided, are looking at their problems as nails, and their pattern as a hammer, when in truth, sometimes you need a screwdriver. Sometimes you need a chisel. Hell, sometimes, there are only tiny nuances, and all you need is a smaller hammer. Examine each situation prior to implementation... try to understand how each part of your application will be used to solve it, which parts will be stressed and how to avoid those stresses, and how to make your code's intent readily apparent to anyone who might need to maintain it.
Cover image "Spaghetti and sausages" by HatM is licensed under CC BY-NC-SA 2.0
All Images obtained from https://search.creativecommons.org/ , and attributed separately in their 'alt' text
Further Reading:
Top comments (1)
This seems more like a case for actual code reviews or pair programming or some other form of training. Before service objects, this would have been a 1000 line class or even a thousand line method.