Comment by sfn42
I do pretty much the same. If any part of the code makes me go "wtf" and sends me down a rabbit hole to figure out why it's like that, I'll typically write a comment about it to save myself and the next guy from that.
However I have one quibble. Almost invariably when I explore other people's code, there's a lot of these "wtf" moments and some times they end with "oooh that makes sense" but honestly most of the time they end with "ugh, this is stupid".
For example i was tasked with factoring out Newtonsoft.Json in favor of System.Text.Json in an Azure function. I looked through the code and it was some of the worst shit I've ever seen. And completely untested, there were two test classes with a few test functions but they literally tested nothing. The only way you could have failed those tests would be by adding a `throw new Exception()` as the first line of the function the test was calling. Seriously. The rest was a for each loop iterating over a list parameter and the test was passing an empty list.
The core of the whole thing was this big chain of functions where over a dozen different parameters were passed down through about two dozen different methods, some weren't even used, some were repeatedly serialized to json, passed as a string to the next method, deserialized and passed as an object to the next method etc. There was this whole complicated AsyncEnumerable setup that served no purpose at all except complicating the code. There was a lot of other stupid stuff. I really don't think my description adequately conveys how bad this code was.
It was fucking atrocious. A coworker did some pair programming with me and suggested asking the author about it, I said I didn't want to talk to him because I didn't know what to say. I don't think I could have had that conversation without just completely shitting all over his entire project. There weren't any questions to ask, there was no "maybe I just don't understand the reasoning". I understood the code perfectly and it was shit. Plain and simple.
So I did what I tend to do in these situations, which is the subject of my quibble - I rewrote it. Instead of trying to figure out how to do what I needed to do in the context of this complete mess, I wrote some tests to establish the current functionality (surprise, it uncovered multiple glaring bugs - not sure how nobody noticed it wasn't even working properly for months) and then I just deleted all the trash code and wrote it properly. It wasn't a very complicated program, it literally just gets some data from a few API endpoints, massages it a bit and sends it off to an event hub as messages.
So my quibble is that a lot of people don't appreciate this approach. They call it scope creep, I get some task that should be fairly small but it leads me down a rabbit hole where I end up rewriting or refactoring large chunks of code in addition to what I'm supposed to do.
I think it's a nuanced topic, I think my approach is appropriate for some situations and not for others. For example if I'm working on an app that only needs to exist for another 6 months it's reasonable to minimize the effort spent on it. Doesn't make sense to refactor stuff. In the case I just described I think the rewrite was appropriate. My team agree with me on that, though I'm not sure everyone would. And I'm having trouble drawing that line, when should I fix stuff and when should I work around it? I love great code and I hate bad code, it honestly really bothers me when I have to work with some moron's spaghetti. So I think I'm pretty biased towards fixing stuff like that when I see it. I'm happy to be in a team that appreciates this side of me but I'm worried about when I inevitably end up in one that doesn't.
Ultimately you have two choices: you refactor the architecture as needed as part of implementing the current feature...or you end up drowning in technical debt farther down the line.
A properly architected code base makes it easy to make the kind of changes you want to make. A badly architected code base (as you describe) makes it nearly impossible. I've found it's usually by far the quickest thing to fix the architecture and then implement the new feature sanely. (And no, you don't ask permission; you just make everything clean. As I noted above, if your boss doesn't like this kind of thinking, you need a new boss.)
Of course, I'm one of those guys Graham mentions who works by himself most of the time.....YMMV.