Often, when writing software, we find ourselves trying to do too much behind the scenes, without thinking if we really need our code to be that clever. This can be a problem especially on user-facing applications, like web and mobile apps. In most of the cases, these bits of business logic being handled way too smartly will eventually lead to a lot of confusion when using the app.
I find this problem particular common when dealing with web apps using Ruby on Rails. In frameworks like Rails, it’s common to accept that the framework will do too much for us “automagically” without us having to worry about what and how it’s being done. Don’t get me wrong, I find this very useful in most cases and it’s one of the main reasons why Rails got so successful and how it just makes so easy to bootstrap and MVP a new web application, with so much being done by the framework behind the scenes.
But there’s a catch. For us developers working with Rails after a reasonable amount of time, this idea of clever and magic code get so trivial and accepted in our minds, that it’s very easy to transport and leak those same patterns to our user-facing applications unconsciously, without stopping for a moment and questioning: “Wait, should I be really doing it like this? It looks clever and useful… but at what cost?”
The problem with code too clever is that, when it leads to confusion (which in most cases it does eventually), you will have a particular angry user on the other side. For us developers, users of frameworks like Rails and other gems, it’s a common picture to see our angry faces when faced with these situations. But since we’re devs, we’ll just open the related source code, investigate it further, find out what’s going on, grumble something like “duh?” or “wth”, and move on with our lives. End users of our apps, on the other hand, will either get very confused and angry, drop a bad review for the app, call the support, never use the app anymore or all at once.
The simple feature request and the magic solution
Suppose you have an app to create reservation requests at medical clinics, passing a selection of required parameters, like personal data, description of the issue or illness, specialty, insurance type, special care requirements, etc. These requests can then be accepted or denied at a later stage. Suppose now that a few of the parameters have special rules, e.g., for some type of specialties, the user is supposed to mark whether the request is an emergency and give the emergency reason. For other types of specialties, the emergency parameter does not make any sense and should not be given.
John Dev is implementing this feature, particularly the part where the reservation model is created and stored, and wonders how he should approach this rule of the urgency reason being applied only for some of the specialties, but not all. He comes up with the bright idea of, in the step of creating the record, checking which specialty was given, and if the specialty is not included in the list of specialties that accept urgency, he would just reset the urgency reason back to nil and proceed with the record creation. Rails is so nice to provide these handy hooks in the model, right? He’d just need to stick another hook in the model to provide the reset behaviour.
before_create :reset_urgency_reason_if_not_accepted
private
def reset_urgency_reason_if_not_accepted
sely.urgency_reason = nil unless specialty_accepts_urgency?
end
“This looks like the best approach”, John Dev thinks. First, it would require less complexity in the view code, by avoiding any JS handling the urgency reason field based on the selected specialty. Second, it would facilitate the user’s life, by letting the user pick and enter any data, and everything would be handled magically behind the scenes in the backend, without the user ever needing to worry about any of these nasty rules. Win, win!
Not so fast John Dev
Let’s take a closer look on what John Dev did.
He decided to hide the business logic and complexity away of the users eyes, by handling it with the handy ActiveRecord hooks that Rails provides. Actually, this is one of the patterns I see more commonly being repeated in most of the Rails apps I worked with until now.
It’s understandable and I agree it’s very tempting to solve it like that, since it’s so easy to handle these kind of situations in the provided hooks at the record level. The more experienced reader will say something like: “Hey, but everyone knows that we should keep our ActiveRecord models skinny and avoid using hooks as much as possible.”
In fact, it doesn’t matter. Even if it was abstracted away in some other class, like in its own command / service object or any other pattern. The problem is not so much where John Dev decided to hide and handle that business logic, but the fact that it is being handled without the user’s knowledge. My example made use of the hooks because it’s one the Rails features that is more often used and abused, whenever we stick any business logic we need to handle and which is associated to the record lifecycle.
The problem of the confused user
The first problem with the mentioned approach is that it leads to a lot of confusion in the usability. The user selects a specialty and, since the field for the urgency reason is still there, the user might also mark the reservation request as urgent and provide a reason. Since there is no feedback at the view telling that the urgency is only handled for only a subset of the available specialties, it would be a big surprise for the user to only realize later that the urgency that he/she entered never saw the light of day. Since everything is being handled automagically at the record creation, although the data is consistent and all the business rules are being met, the lack of transparency leaves the user blind on what really happened. The support team (if any) will be the one to take the load.
Many people will object that this is a very poor design decision for this feature, by leaving the urgency field enabled even though a specialty which does not accept urgency was selected. A smarter decision would be to handle that on the JS side and show/hide the field depending on the selected specialty, right? Well, partially. We’ll see next that this approach has also its share of problems.
The problem of the swallowed bugs
As suggested before, John Dev decides to ehnance the approach by only displaying the urgency reason field if a specialty that accepts urgency is selected, keeping it hidden otherwise. Now the user is not confused anymore and everything looks just fine. Well, until someone introduces a bug.
We all know that code changes. And changes a lot. It’s also agreeable that all projects have different test coverage quality and, even inside the same project, test coverage quality differs at the back and front-end level. It’s very easy to think that, sometime in the future, someone can just introduce a bug where the hide/show feature on the urgency field stops working. Hopefully this will be covered by tests somehow. But then, it might also not be covered by tests, especially if the project is not yet very mature. If this takes too long to be spotted by a QA session and slips through unnoticed, we’re back in the previous situation. The confusion will be back and the not-so-good news will come from the users again.
Another situation is where the show/hide feature might not be broken, but because of some very strange edge case (which we all know it happens from time to time) the urgency reason is arriving at the model creation with some data. We most likely have a bug in our system. But since this data is getting resetted at the record creation, we might never know what is the root cause of the problem, because no one will ever notice that. Until it affects another part of the code, when it might be too late.
The point is that it can feel very brittle to handle this at the view side, and bugs can go unnoticed because we’re making sure to handle buggy data and trying to keep the state consistent.
The problem of the confused co-worker
We have to think on other very particular users of the app, which are our dev colleagues. Ideally, they are not only end users of the app, but certainly users of the source code and the past design decisions. I bet that there’s hardly no greater frustration then trying to use a particular piece of code, expecting that it behaves in a way, but surprisingly discovering that something else unexpected happens. We waste so many time debugging and investigating the source code just to find the culprit.
It’s not hard to imagine that someone in the future would need to create this record for any other feature, or maybe even using it in the tests, passing specialty and urgency as parameters, only to find out that the provided urgency data is not being persisted correctly, but constantly empty, no matter what. Hopefully it would require one file to be opened to find out the answer in the hooks. But if more layers and indirections are being used, or if the reset logic is not in the hooks, but abstracted away elsewhere, we can imagine many more files being opened until the poor co-worker finally finds out what’s going on.
Less magic, more transparency
It’s very easy to conclude that the best approach here would be to handle this case with a validation at the model and let it fail in case the urgency data is set together with a specialty that does not accept urgency.
validate -> {
errors.add(:urgency_reason, :invalid) if urgency_reason && !accepts_urgency?
}
For some, it might be the obvious decision from the beginning. But maybe because of the simplicity of this example. Sometimes it might not be that obvious when the feature is more complex. We can easily lose our way through the implementation, over-engineering and over-complicating it by trying to be too smart and too proactive when handling the edge cases.
In most of the cases, it’s better to be explicit and a little bit dumb and just fail loud in case of an unexpected combination of data or an unexpected scenario.
In this particular example, letting the user see on the page that the urgency is not valid (or should not be provided) when that particular specialty is selected, would the best way to handle it. Combining with the show/hide feature on the field would be a plus. We facilitate users’ lives and at the same time, we’re sure that, if anything goes wrong, we know that the user will see the error, hopefully with a very descriptive message, and will be able to figure out the way around it.
Sometimes though it’s not feasible to show error messages for the users, particular when it’s hard to provide them a way to recover from the situation when something goes wrong. In this case, it’s always best to fail loud instead of trying to recover and keep the consistency of the data in a smart way. In the case where we have the validations on both front and backend, if for some reason an invalid data slips through to the backend, it’s better to see a bug report with the validation error, instead of having a smart code in place handling it and having the issue go unnoticed. Both approaches keep the state correct, but with the first one we know that something went wrong and where to look for the root cause.
If all of that sounded too trivial to you and you knew from the beginning that a validation would be a better approach, I’m happy to hear that! If you don’t agree or see any other problems with the suggested approach, please let me know. Always happy to discuss.
Top comments (0)