Maintainable Code > Clever Code
You’re writing some super smart bit of code. You’re sure future developers will marvel at your engineering glory. STOP! You might just be creating tech debt, future bugs, and frustration.
Developers like to show off how smart they are. They create objects that seem like magic. They allow you to write in one line of code, what others might take ten lines to do. In some cases in zero lines of code.
Things just seem to take care of themselves without future developers having to worry about anything. And yet in their pursuit of cleverness they doom the project to future bugs and tech debt. I know. I was complicit in creating a framework just like this.
Let me give you an example from my own sin list.
My custom database ORM for MileSplit was (and is) awesome and fast! I hate most prebuilt database abstraction layers because they are so bloated and clumsy (in their own ways trying to be too clever).
In this framework, when you returned the results from a query it gave you an array of light-weight DataRow objects. These objects basically just held the results of the query with a few little nice things added in. However, they were not full-blown models so you could not actually do anything too meaningful with them, such as modifying them and then persisting the changes to the database.
Smartie pants little me decided HAH! I can fix that! If I want to use those DataRows as models I will do it… automagically!
There is a PHP magic method called __call that allows you to intercept any time someone tries to call a method that does not exist. So I added that to my DataRow object. Now when someone called a method on the DataRow that only existed on the Model, __call would fire and automatically upgrade the object to a model. BOOM! Now you get the best of both worlds!
Or so I thought.
Yes, it worked. And it worked well. But at the cost of bugs and maintainability. Why?
For one, the IDE absolutely choked on it. Any time it thought it was dealing with a DataRow but suddenly you start calling methods that don’t exist… it’s going to do its job and rightfully yell at you. But of course the code isn’t broken. You’re using it the way you intended. So you just start ignoring these IDE warnings. Not to mention you lose the benefit of auto-complete and hinting! And thus you have just lost the power of an IDE.
Second, this creates a kind of tribal knowledge. It was fine when we had one or two developers, but as the team scales or you get some churn it creates this “black box” that is not obvious to use. No one understands what it does, so they use it wrong, and it “just works”… until it doesn’t. Which brings me to…
Thirdly, when there are bugs and you are trying to debug them suddenly you are thrust into this mysterious world. There is a blackhole of trying to find out where this method actually exists and how it gets called. You spend hours trying to troubleshoot something that would have taken minutes if the code was more obvious.
So what do we do about it?
In this specific case, the upgrading of a DataRow to a Model is an excellent idea still. However, it would be much better to create a method to explicitly upgrade it, rather than do so covertly through magic methods. That way our IDE can properly respond to the type change and developers can clearly see what is going on.
Another case that I have run into (which I did not create) was another framework that used Listener objects to do things for you. You’d make a change and suddenly it was being saved to the database, without you ever knowing it. You just kind of needed to anticipate and then have faith it was happening behind the scenes.
It seemed to them as a great idea. Now you had to write exactly zero lines of code to run that logic. However, again now you can’t really troubleshoot by linearly following the path of what’s called and new developers need to be tribalized extensively to figure it out.
Stop doing that.
Favor creating clear, obvious and maintainable code over clever tricks. We all know you’re smart, you don’t have to show us. And we know you think you’re saving lines of code or making things more efficient, but you’re not.
Don’t get me wrong, it’s great to abstract away complex logic. It’s awesome to make intelligent and concise code that feels like magic. But it’s not okay to create a disjointed codebase that requires a lot of training and blind trust. And it’s not okay to do things that break your IDE’s helpful intellisense or make it impossible to troubleshoot linearly and efficiently.
Make your code document itself by doing exactly what it looks like it’s doing.
On a related note, one more thing while I end my rant… and I have committed this one like crazy too…. your method name should indicate what’s going to do. And it should ONLY do that one thing.
STOP with unintended side effects. I call a “getter” method and it ends up setting something. NO! If the method is called isCached it needs to just give me a boolean. If you make it also fetch a value from Redis and then store that value somewhere, I will smack you. (This is another one I did four or five years ago, and I already smacked myself)
STOP with returning something different than the name of the method would indicate. If a method is called getNumberOfStrikes you better just return me a number and not an object or a string.