Today I was rewriting a function. The function makes an HTTP request with a form-encoded request body. But after preparing the body of the reqeust, it runs it through a regular expression to replace /%5B\d+%5D=/ with []=.
There’s no comment explaining why this is done, so I have to guess.
I have a couple guesses.
- The API we’re calling doesn’t like the array indexes (the
\d+part in the regular expression). - The API we’re calling doesn’t properly unescape the
%5Band%5Dsequences. - Maybe both?
I really suspect it’s #1. If the case were #2, surely we’d just replace all occurrences of %5B with [ and %5D with ], not only the occurrences with digits between.
But that leaves the question: Why don’t we replace matches with %5B%5D=? Why do we un-escape these characters, too?
So what did I do?
I copied this dubious behavior into my new function. Even though it’s sloppy and confusing, it’s probably harmless. And removing it is sure to cause problems, since I don’t know exactly what behavior it’s trying to correct for.
What’s the lesson? Two, I think:
- Make your code obvious. If you can’t explain why the code exists with the function name, use comments.
- Rewrites are dangerous. Code, no matter how poorly organized and ugly, contains months or years of accumulated bug fixes and wisdom. As tempting as it is to rewrite, do it carefully, lest you re-introduce bugs you don’t understand!
If you enjoyed this message, subscribe to The Daily Commit to get future messages to your inbox.
Top comments (1)
There is a bug in Excel that results in incorrect leap year calculations. The bug was originally introduced in Lotus 123 and Microsoft intentionally copied the bug so as to be able to claim compatibility. Go figure....
The bug is that centuries should NOT be leap years even though they are divisible by 4 UNLESS they are also a millennium. 1896, 1904, 1996, 2000 and 2004 are leap years. 1900 was not. Fortunately for me it is unlikely to significantly impact my work in the run up to 2100.