Today I intentionally copied a bug

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.

1. The API we’re calling doesn’t like the array indexes (the \d+ part in the regular expression).
2. The API we’re calling doesn’t properly unescape the %5B and %5D sequences.
3. 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:

1. Make your code obvious. If you can’t explain why the code exists with the function name, use comments.
2. 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.