Ratchets in software development

So there's a thing we use at work which I call a ratchet.

In our codebase, there are "patterns" which we used to use all the time, but we decided to stop using them, but removing all of the existing instances at once is too much work. We want to remove all of these instances eventually, and in the meantime we want to make absolutely sure that they don't proliferate via copy-and-paste. So what we have is a ratchet, a script which runs at source code linting time and counts all of these "pattern" instances across the codebase. If the script counts too many instances, it raises an error, explaining why we don't want more of that "pattern". If it counts too few, it also raises an error, this time congratulating you and prompting you to lower the expected number.

This script is intentionally extremely simple. The expected numbers are hard-coded in the script itself. The "patterns" for which it scans our code are not advanced, abstract Gang of Four-style software design patterns but plain text strings.

At the time of writing, the strings are mostly the names of methods whose usage we frown upon. The methods in question aren't our own. They are public methods of first- and third-party libraries we use. We can't apply deprecation warnings upstream. Nor would we. These are perfectly acceptable and normal methods for people in general to use. It's only within the scope of our own specific codebase that we've decided to try to quit using them.

The script carries out extremely basic string matching. There is no source code parsing. So there are some obvious edge cases. What if someone wants to talk about THE FORBIDDEN METHOD in a comment, say? What if it shows up in a string literal? Answer:

  1. [shrug]
  2. It hasn't come up
  3. I guess we'd just raise the ratchet by 1
  4. Oh, but make sure the scanning script doesn't scan itself

One important observation is that this technique does nothing to actively encourage the removal of these old "patterns". Those remaining 67 or so calls to THE FORBIDDEN METHOD have been kind of lingering. But perhaps that's a different problem.

Sometimes, due to extenuating circumstances, we have had to manually raise the count again. This is something we try to avoid, though.

In the very near future I plan to upgrade the script to support regular expression matches as well as simple string matches. It currently isn't very good at raising sensible error messages — you have to read the explanatory comments in the script's source code — and it might be nice if it could ratchet the expected counts downwards automatically instead of demanding that the developer do it. In theory, this ratchet script is not a long walk from a conventional source code linter, and it might be nice if it had a more consistent, flexible interface for adding new heuristics, configuring which source files to inspect or ignore, automatically suggesting fixed code, and so on, and so on...

But on the other hand, I am as conscious as anybody that it would be incredibly easy to divert a huge amount of pointless time and energy into maintaining and improving a "simple" tool of this kind. I think the specifics of our ratchet script (whose content, no, I will not be sharing) are less important here than the generic technique of using basic text scans at linting time to prevent the proliferation of deprecated practices through a codebase.

In general, I dislike having bad practice left over in our codebase. This can be difficult to avoid, but is very misleading for newcomers, and for interlopers — that is, experienced developers from other teams who open pull requests with the best of intentions. "Ah, I see you have diligently followed the example set by your predecessors. Well done, and bad luck. Changes requested." What this technique does is automate what was previously a manual process of me saying "don't do this, we've stopped doing this" in code review. Or forgetting to say it. Or missing the changes entirely, due to the newcomer having the audacity to request their review from someone else.

Another pitfall which I've spotted is that it would be easy to abuse this technique to enforce unnecessarily strict "standards" on a development team who really ought to be allowed some creative freedom. Sometimes it's okay to say "No" to adding a new rule.

*

This felt like a really basic technique when we first adopted it, but on the other hand it's not a standard practice I've heard discussed elsewhere in the same way that, say, linting, unit testing, code coverage measurement and other techniques are. When I asked people about this online, quite a few people found the idea to be novel and expressed an interest in adopting it. Meanwhile, an equal number of people said that they already do something almost exactly like this, or applied a similar technique to the domains of code coverage or performance.

Anyway, it seems to work okay.

Discussion (14)

2021-11-21 03:30:13 by Josh:

What are the methods in question? (Interested in what kinds of purposes this gets applied to.)

2021-11-21 03:30:19 by Max:

What's the advantage of counting instances over simply forbidding to add new usages of the pattern (by looking at `git diff` output or equivalent)

2021-11-21 04:31:02 by Emily:

Max: > What this technique does is automate what was previously a manual process of me saying "don't do this, we've stopped doing this" in code review. Or, more likely, the "eh, that's ugly, but I guess it works" that you'd probably get from me and most of the teams I've worked with unless you did something *really* egregious.

2021-11-21 08:52:34 by skztr:

I don't recall the specifics, but I do recall at one job we ended up adding a hook that would reject commits which added code in a particularly often-used-but-disliked manner. The hook could be bypassed by including a line in the commit message saying "I understand that <the thing we were discouraging> is frowned upon, but I have a very good reason in this case:" This was done with an exact match in the commit message, so sorry to anyone who had already explained, but using sightly different introductory phrasing.

2021-11-21 13:20:33 by Tim McCormack:

Max: Diff linting is more complex and fragile. We used to use it at work, and switched to amnesty linting: Every existing instance was given a line-end comment saying "this is OK for now", and after that any unannotated instance was considered a lint failure. We had to write that annotation tool, though: https://github.com/edx/edx-lint#using-lint-amnesty

2021-11-22 10:30:51 by Spwack:

When I saw the name of this post, and read the first few lines, I thought this was going to go in the exact opposite direction. At my workplace, we have format standards that sometimes conflict, are half-finished, have been changed at some point in the past, or are just generally rubbish. And the production of format standards only ever "ratchets" in one direction. Nobody wants to be the architect to promote *less* rules, what kind of cowboy would that make them look like!? No, the only "safe" option is to keep all of the existing rules, and occasionally add more. Never remove. It breaks my brain. Anyway, you've been complained at now about something tangentially (at best) to the post. Congratulations! ((Is there an equivalent to "tangentially" for the other trig functions?))

2021-11-24 09:39:13 by LiterallyMechanical:

@Spwack > ((Is there an equivalent to "tangentially" for the other trig functions?)) Hyperbolically?

2021-11-28 04:17:40 by Jake:

Bikeshedding: There's a language I haven't really tried, which is specifically for the concept of matching code patterns in a language - semgrep https://github.com/returntocorp/semgrep. Instead of Sam's amnesty count, it has the feature skztr mentioned - the ability to exclude individual lines from the linting process. OTOH, it's not familiar tech, and anything you write in it will remain obscure juju to anyone unfamiliar with it. I side with a well-understood/well-understandable custom tool over an unfamiliar rules engine any day.

2021-11-29 02:56:29 by g:

Spwack, "tangentially" is only (ahahaha) tangentially related to the trigonometric function called tangent. A line is tangent to a curve if it meets it while going in the same direction. This typically means that (1) it doesn't then go _inside_ whatever region is bounded by the curve, and (2) after meeting it it then diverges again. Something is "tangential" when it metaphorically behaves in this way. I think the tangent _function_ is called that because it answers the following question: if you start at some point on a unit circle, and proceed along a tangent from there until the line segment you've drawn subtends a given angle at the centre of the circle, how long is that line segment?

2021-12-17 05:29:55 by FeepingCreature:

We use D at work. D has a way of annotating a symbol (function, class...) as deprecated. Deprecated means you can still use it, but the compiler will print a warning. There's a switch to turn those warnings into errors. That switch is usually turned on on newer projects. Whenever we need to urgently upgrade a project, and something gets deprecated but we can't fix it, we can just turn that switch off and accept the warnings. But that's kind of a mark of shame.

2021-12-17 07:31:42 by Ingvar:

In a pretty substantial code (well, technically config) base ata previous job, it had started as a free-for-all and we'd all talked about "would it not be nice if this was all lint-clean and enforced at PR time?". But, the problem with that was that almost all existing code failed. So, I wrote a small lint script that always ran the linter (so you could get reports), but checked the file name against a list of regular expressions and if it matched, it simply didn't trigger the "nope, you can't do this" failure in the PR review tool. Then, as we cleaned up subdirectory by subdirectory, we could just refine the list of regexps, so that any changes in an already clean part would need to also be clean. Now, this works on a granularity down to "file-sized", but no smaller. So the applicability certainly would vary from individual to individual.

2022-01-08 01:19:57 by callmesalticidae:

I was a little more than halfway through before I realized this post was exactly what it appeared to be, and wasn't a stealth horror story.

2022-03-15 06:00:28 by Curt J. Sampson:

This is a great idea, and well written up. The particularly good points are that it's properly seen as an assist to help the developers do the right thing that they already want (and know, at least as a community if not always as individuals) to do, that it's not taken too far, and in particular that there's an understanding of the potential for abuse. I've seen far too many good guidelines that turn into terrible rules that make the code worse, with developers seemingly helpless in the face of them. Formatting rules are particularly prone to this, with the original goal (make the code more readable to other developers) often completely lost to, "it's better to have it consistent to whatever a limited degree an automated code formatter is capable, regardless of readability."

2023-06-09 20:28:53 by J:

I've found this extremely late and in the context of the recent "someone bumped the counter by one" tweet, but I've had something similar in a project for a long time but it works by checking the count in the current branch is no greater than the count in the merge target. Less chance for misuse, also for the rare person who really does get to skip CI to break it for everyone else.

New comment by :

Plain text only. Line breaks become <br/>
The square root of minus one: