Oh boy. You’re opening a can of worms with this question! Here’s my story.
So, essentially a bunch of us working on a project were all young, junior engineers. That in itself isn’t a problem if you have strong oversight and management, but we also lacked that.
Our manager at the time was what you’d call a “Wants-to-be-technical” manager
. As a manager, your job is no longer to code—it’s to manage. But instead of managing they were always just coding, and subsequently the rest of us didn’t have any supervision, direction, anyone to review PRs, etc.With a lack of direction, everyday was constantly a firefight. Sometimes we’d work on a task for a few days just to scrap all that work because it turned out not to be what a customer wanted. Comically disastrous.
Anyway, you had all of these junior developers sort of just slapping together features in an application that slowly ballooned out of control. Because we were rushing just to get stuff working, there was ludicrous code and bad practices popping in everywhere:
- Changing global variables from random places in code. Generally speaking, if you use global variables (strong if. Global variables are just bleh in general), you want to make them immutable. Otherwise, you get to do what I did, which is stay up until 3am digging through thousands of lines of code and trying to figure out where this one variable keeps getting changed and breaking everything else.
- Using pass-by-reference. It’s generally bad practice to modify a parameter within a function in Object-Oriented Programming, instead preferring pass-by-value (where the function returns a new copy of an object with desired changes). There were several functions where people passed a list into the function and directly modified the list. I had wasted hours and hours trying to figure out why values were changing or disappearing, seemingly randomly
- Swallowed exceptions/error message. If you’ve coded before, you should know about error-handling and throwing exceptions. If you have a web application, you have to do two things:
- However deep down the stack your error occurs, you have to propagate something useful back up to the original entrypoint so you can return a sensible error message to the end user.
- You also have to properly log your errors in your stack so that your engineers can find these errors and fix / debug them later.
We did neither of the above… there was one specific, hilarious example I remember where I was trying to figure out why I wasn’t getting an error message on a negative-test scenario. I went into the code, and someone had literally caught the exception, and then did nothing. No upward propagation. No logging. No error message or return code. Just nothing. - Overzealous separation of concerns. Generally, it’s best practice to separate concerns and functionality. For example, all code associated with products in a shipping system should generally be put in product-related classes. But here’s the thing: practice is different than theory. Some of the engineers were overly-religious about separating things, to the degree that there were sometimes classes with just 5-line functions. This meant that a simple web API call sometimes went dozens of classes and functions into the call stack, making debugging just a royal nightmare.
- “Alright, I’m making a call to fetch a list of products… wait. This goes into a product labeler class, that just returns the label, but in order to format this label it goes to another utility class to sanitize some data, that it fetches from a database class, which calls another utility class to universalize outbound REST calls…” ← so on and so forth…. (just an example of what it was like to debug our code. Actual functionality / names were anonymized)
There’s a bunch more stuff, but this answer is getting a bit long. Overall, what was the culmination of the above? Near the end of my time at the company I tried to pick up an effort to refactor a bunch of this code.
How’d I do? Utter failure. I found a 500-line function that I tried to split up into multiple functions. Upon trying to refactor this one function, functionality in about 20 other places broke.
I abandoned that branch after poking at it for ~4 days.
So if there’s anything to take away from this:
- All of the engineers have to be in agreement about architecture and practices. OR, you have to have someone overseeing all of them to help guide, review PRs, etc.
- If you’re a manager, then manage. Stop coding. That’s not your job anymore.
- It might take more time now, but architecting things and being pedantic now really, really saves you a lot of time, frustration, and money down the line.
Footnotes