Layer code instead of modifying in place

I caught myself doing something shady recently and thought I’d share so that you may avoid doing the same. I ought to have known better, but, I was a few weeks into a new feature branch on Standard Notes, and last minute, I needed an important feature to make everything else work. And it got really, really ugly.

The background of it is I wanted to sync user preferences to a user’s account, rather than saving just locally. This way if you choose to sort your notes by title instead of date, this preference will take effect on other computers you open the app on. The challenge is, there needs to be only 1 user preferences object on the server (the rule is also that I don’t modify any model schemas on the server. I use the generic schemaless Item that Standard File provides that allows for arbitrary content.) This is made difficult by the fact that when a sync conflict occurs, objects are duplicated so that no changes are lost.

So, I needed to introduce the concept of a “singleton” model, where only 1 can exist for a given user, both on the client and the server at any given time. As I mentioned, adding this in was last on my todo list on this feature branch, so I had already depleted my “wise” architectural energy on other parts of the code. So in all my glorious haste, here’s how I put together this so-called singleton feature.

You can see the abandoned pull request here. I’ll spare you the code as it’s too long, but essentially, I have this “service” called the SyncManager. The SyncManager has one responsibility: sync changes you make to local content, and retrieve changes available on the server. And it’s pretty good at doing that one task. What I needed was for this once focused SyncManager to be able to handle items that are marked as “singletons”, and in that case, it should first retrieve pending items from the server, then send up the local singleton model if nothing comes back. This way I ensure only 1 exists.

So I went into this SyncManager and start changing a shit ton of code right in place, adding a bunch of conditionals to teach it this one new trick of how to handle singletons. I modified my main sync function and filtered what gets sent up to the server, changed my pagination function to account for singletons, and a whole bunch of other nasty stuff that I definitely was not sold on. I felt really dirty about the whole ordeal. But hey, I ran it, and it worked.

I played around on this branch for about 3 days after that, to make sure that no matter what I did, only 1 user preferences object existed. I placed a console.log that counted the number of extant user preferences, and it always said 1. Wonderful. I really pulled this off.

One day before I was about to ship, I was playing around with the app, and there it was:

Number of user preferences: 2.

Ahhhh. One job. You had one job.

I scrapped it. I scrapped the whole thing. I literally abandoned that entire branch after I saw that. Because it was justice. I knew the way I built the singleton handling was dirty. And I felt better about scrapping this hastily put together feature than to jeopardize the most important part of this whole ecosystem: the syncing.

It’s been two months since that horrific incident. And I haven’t forgotten the need for syncable user preferences. So, feeling cool-headed and calm, I gave the singleton feature another run for its money. This time, I was going to do it the right way.

Here’s the thing: the SyncManager is really good at doing its one job. It’s really good at syncing things. Rather than teaching it new tricks, and it then doing both of these tasks with lessened zeal, it would be wise to build on top of it. This is Architecture 101, but in the real world, we don’t always do what is right, but what is convenient. (There is a word for this that I’m struggling to recall. If you remember what it is please share it with me. I think it starts with a c.)

So that’s what I did. I left the SyncManager intact, and instead created a new “SingletonManager”. The singleton manager has one job and one job only: make sure that only 1 item exists of any object you are instructed to keep an eye on. It took less than five minutes to put together the logic, and I gave it a run.

Lo and behold, it did its job, and seems to do it well. No matter how many user pref objects I tried to create, it always resolved into 1. And I was completely clear-headed about the whole thing.

The moral of the story is: try not to modify things that already work. Because it’s very easy for those modifications to lessen its ability to do the one thing you already knew it was able to do. Instead, build on top of working components. The SyncManager is really good at syncing. Teaching it new tricks will not only have it struggle to perform that new trick, but also lessen its ability to perform the thing you depended on it to do. I now have functioning singleton models, which will be really important going forward, handled by a class that is really good and specialized at resolving multiple items for which there should only be 1. In code, I think it’s safe to say two specialists are better than 1 generalist. And I will never forget this lesson again.

You can see the new SingletonManager (WIP) here.

Update: the word was expedient. Definitely did not start with a c.

Update 2: A reader on Twitter correctly identified this as exactly the "Open/closed principle": https://en.wikipedia.org/wiki/Open/closed_principle