A Short Cautionary Tale About Refactoring
zigamiklic.com
zigamiklic.com
The job isn’t to make something work. It’s to make it work AND be resilient to a bad refactoring. If you didn’t properly protect your implementation with tests and comments you didn’t implement it at all.
Normally, if unit tests were missing, I would add them before starting refactoring. In this case, to be honest, I'm not sure how I could have written a unit test to catch the issue. An E2E test is the only one that comes to mind. Please let me know if I am missing an easy way to test for a 404 image when offline. Thank you!
Flip ANY of the three, and there is no problem. Had I been the original author I would have gone for the comment, it’s clearly the simplest one in this case.
// Image inlined because error may be due to dropped connection
You don’t have to have tests to prevent bad refactoring. 100% test coverage isn’t realistic for most code bases (or isn’t even a net positive in many cases).
self-documenting code, or tests, or comments would have saved the situation. You don’t need to design a test in this case. A one line comment that takes five seconds to write is enough.
I call this "drive-by" commenting, and I do this so that others don't have to waste their time figuring out the things that I did. I find it particularly important to surface assumptions a piece of code may make, that aren't obvious from its signature. Random recent examples:
- A C++ function that takes two containers and internally does std::transform on them: hidden assumption here is the second container is at least as big as the first one (otherwise std::transform will try to read past its end).
- A C++ function that internally calls std::unique and std::sort: the inputs now must be valid for comparison under == and <, and also the output is now sorted - which may, or may not, be something some users already rely on. Better to either pin this behavior down in documentation, or explicitly disclaim it ("relative order of input elements is not preserved") to leave some wiggle room for future changes.
A lot of such things can be expressed using type systems, compile-time checks and runtime checks, but it's not always obvious to the reader. My approach is: if a function makes an assumption or guarantee in a way you won't see it in its signature, and your IDE won't pick up on it automatically, then it should be described in the comment.
Some such things can also be pinned down in unit tests, to better protect against inadvertent breakage due to refactorings - but I say also, not instead, because day-to-day, nobody actually looks at unit tests related to random functions they call, while a properly configured editor or IDE will show documentation comments of function signatures as you type or browse completions.
A better link: https://fs.blog/2020/03/chestertons-fence/
In other words, it could/needed still to be improved for better maintainability, not just in the obvious way (Chesterton's fence, as mentioned by another commenter)
"The code is the HOW, the comments are the WHY"
Things like, "relative ordering of results is not preserved", or "this function is O(n^3)".
The correct refactoring is one the author fuelled with coffee, which is naturally the real lesson here.
This isn't refactoring per-se it is focusing on the cosmetics of the code at the expense of functionality. There are many other reasons why an inlined image may have been appropriate and the author overruled all possible concerns by going for replacing a large block of 'ugly red' by a smaller block 'pretty green', which is a very bad reason to make a change in functionality, especially without understanding what is going on.
I expected the author to mention adding a code comment so future maintainers don't make the same mistake.
> ...if there is a good reason for the current approach, document it. Add a more descriptive name or a comment to avoid confusion in the future.
- someone should have pushed back hard on the design that embedded such a huge png in the design
- the programmer in the article should have broadcast his finding by submitting a code comment
> Suppose that a great commotion arises in the street about something, let us say a lamp-post, which many influential persons desire to pull down. A grey-clad monk, who is the spirit of the Middle Ages, is approached upon the matter, and begins to say, in the arid manner of the Schoolmen, “Let us first of all consider, my brethren, the value of Light. If Light be in itself good—” At this point he is somewhat excusably knocked down. All the people make a rush for the lamp-post, the lamp-post is down in ten minutes, and they go about congratulating each other on their un-mediaeval practicality. But as things go on they do not work out so easily. Some people have pulled the lamp-post down because they wanted the electric light; some because they wanted old iron; some because they wanted darkness, because their deeds were evil. Some thought it not enough of a lamp-post, some too much; some acted because they wanted to smash municipal machinery; some because they wanted to smash something. And there is war in the night, no man knowing whom he strikes. So, gradually and inevitably, to-day, to-morrow, or the next day, there comes back the conviction that the monk was right after all, and that all depends on what is the philosophy of Light. Only what we might have discussed under the gas-lamp, we now must discuss in the dark.
The flip side is that refactoring once in a while can clean up undefined behaviors that no one is around anymore to understand or explain. There is also a problem with companies that try to make engineers hot swappable and don’t focus resources on retention, often the people who understand the system’s special complexity are long gone.
On a many codebases I've worked on, it seems prohibitively expensive to research the specific context for every nonsense piece of code before refactoring it. Does it count to say "this code exists because the original author added a lot of nonsense - let's refactor"?
I think a more reasonable approach is to change stuff in a conservative way where you limit the blast radius. Like running something like Ruby's scientist library on the new code.
If you can't find a reason (or is not willing to do so), it's practically calling the previous programmer either a bad one, or just doesn't care about anything at all. In that case, refactoring might be the least of your concerns.
Stick around long enough in one place, and you'll end up doing git blame on some gnarly code and discover that the culprit responsible was... yourself.
No. Why is the code nonsense? What caused the author to land nonsense vs something different? It's very possible that the authors have very poor taste and/or decision making, or maybe had different understanding of requirements or potential future direction. Understand how they were mistaken and why the refactored version is better.
Unless you're dealing with code which is literal nonsense (i.e. doesn't compile and/or never did what it was supposed to), assume you're mistaken until you see how the code is mistaken instead.
That code base will rot out of a fear of touching it.
The answer to the Chesterton rule is: put a note on your fence to explain why that fence is there, and under what conditions it can be removed in the future. (With code you also have the luxury of tests to verify the fence is there so long as the conditions for it being needed still remain)
It happens in other fields, too. When I moved to a new city, and my new doctor disparaged a prescription from my previous one, without being able to offer any understanding for why the decision might originally have been made, I promptly found myself a new new doctor.
If the original code hadn’t tested these error paths, the author wouldn’t have found out about this use case until after the first reports came in. Worst case, they’d never know and the customer would get a shoddy experience forever!
But its also possible the app doesnt use bundling and the error module is cached in isolation. The inlining would be fine then :)
This also would have been a good case for a 1 line comment: // embed OOPS image since client may not be connected