When I’ve had to take over maintenance of code that isn’t mine, and is poorly commented and documented, which is usually the case, I work through the code and comment it. That eventually leads to refactoring, adding unit tests as needed, and so on…and eventually the code that wasn’t mine is now mine.
But there’s no time in the schedule for that? Either that’s not true (the long way round is usually the shortest way home); or it’s time to find another project. And if you’re the sort of person who has the skills Graham describes, that shouldn’t be hard.
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.
Yeah, I completely agree with you. I might be a bit "scarred" by my previous team. We were working on CMS websites for a fairly large number of different clients. Every task was estimated by the hour, and if we went over that estimate it usually meant we were working for free. I mean I still got paid but my company didn't. It didn't help that all the solutions were either developed by Juniors or inherited from other departments/companies. Literally every moderately popular frontend tech was represented. React, Vue, AngularJS, jquery, nextjs, svelte, and more. Let's just.make our lives as difficult as possible eh, what should we use next? The back ends were all the same CMS luckily, but of course some genius had to go and add Sanity to the mix to pad their resume.
So I frequently had to learn a new js framework while dealing with shitty, unreviewed and untested code that barely worked or just kind of seemed to work, while under time pressure.
In that environment, my disdain for bad code really just didn't work out. Though I feel like I was a bit unfairly treated at times as well.
There was this one app that had an integration against a third party API. They had used a library to interact with the API, this library was purchased for a fairly significant amount of money from some university and it was complete garbage. All it did was get data twice a day and store it in a db, then the CMS had a daily scheduled job that would get the data from that db and build the content in the actual CMS db. It was like 30k loc, a lot of which was dead and the rest of it was shitty and redundant manual xml parsing and completely unnecessary db interaction. Not to mention the (probably minor) cost of running this pointless db.
So there's a problem with it and I spend days bughunting, I can't figure out what's wrong. I try telling the team that I think I can build a better solution from scratch faster than I can fix this bug. No bueno. So over the weekend, on my own time, (really just a few hours) I throw together a quick POC. I don't need the intermediate DB, the CMS can just get the data from the API once a day during the scheduled job. So 90% of the work is just making DTOs to deserialize the xml into, using whatever xml serialization library i found.
I finish it, hook it up, it's not totally done but it pretty much works already. On Monday I tell the team what I've done and just get completely shut down. They don't even want to see it, just nope.
So that code is gone and they're still maintaining that piece of shit library for no reason. Oh well, luckily it's not my problem any more.
Sorry about the rant, and thanks for the support. It's nice to know I'm not alone in seeing the value of quality.
But there’s no time in the schedule for that? Either that’s not true (the long way round is usually the shortest way home); or it’s time to find another project. And if you’re the sort of person who has the skills Graham describes, that shouldn’t be hard.