Your Code Sucks
In the early 1200s, Muhammad, Shah of Khwarezm (city of Samarkand, in modern Uzbekistan) was approached by ambassadors, seeking to establish new trade routes with a warlord from the east along the silk road. Insulted, the shah had the ambassadors beheaded. In 1220, this same warlord sacked the city of Samarkand, slaughtering everyone who fled to the keep and pressing tens of thousands of residents into his service. The warlord’s name was Genghis Khan and it was a bad idea. The sultan’s reaction was not appropriate to the situation at hand, and it caused Genghis Khan to declare war, a war that ended up killing the sultan and everyone around him. It’s not just the message that has to be delivered that gets you into trouble, it’s the way you deliver it.
While your coworkers aren’t Genghis Khan, the same basic dynamic of human behavior still applies. You periodically will have to tell someone that there is something wrong with their code, and the way you do it can have a profound and remarkable impact on the way your team interacts from there on out. People are protective of their code, thinking of it as a reflection on their quality as a person, and you WILL get bad reactions when you criticize that code in the wrong way. And those reactions may linger for years, slowly eroding team morale and cohesion, and probably causing even worse code to be written.
However, you also can’t let bad code linger (if it is truly bad), because that also degrades the quality of the codebase and can lead to problems for the entire team, not just the person who wrote the bad code. Bad code also tends accrete more bad code over time, as people often end up writing bad code to mitigate the problems caused by bad code. When it finally comes time to rip out the bad code completely, it is often buried so deeply in the system that it can’t be removed without extreme effort. This dynamic, applied several times in a single codebase, will eventually mean that the codebase has to be rewritten entirely – and if you didn’t address bad code last time, it will happen again in the new codebase.
It’s not hard to find things to criticize about other people’s code (or your own code from 6 months ago). However, it’s much more difficult to present a working solution and get everyone on board without upsetting anyone. Even if no one gets upset, you still want to make sure that any resolution of a coding problem leaves the codebase in a better state than you found it. Done properly, every time you find bad code, your team has an opportunity for growth. Done poorly, every time you find bad code is an opportunity for conflict. If you want to have a long and productive career, you need to master the art of telling people their code sucks in a way that helps fix it.
Avoid your initial reaction
Whether you are correct or not, your initial react has a fair chance of being wrong, overly emotional, or to be based on insufficient data. Even if you make a great and perfectly accurate snap decision, the speed of the decision will be taken as evidence that it is a bad decision.
Even if you are correct, you still need to spend some time thinking of valid reasons someone would write bad code, simply to be in a good headspace to correct a team member. Remember that if you had all the information to determine instantly whether a piece of code is bad in a given context, it’s because you wrote it.
Version control history is your friend. Make sure that the person you are blaming is the one that actually wrote it, rather than just the last to touch it.
Determine why the code was written that way
Most bad code isn’t written that way just because the developer is lazy, uninformed, or actively malicious. Something else is usually in the mix. You should leave your job if you start actively suspecting that this statement isn’t true.
Changing requirements and overly tight deadlines create a huge amount of bad code. Under pressure, many developers will write bad code that “works”, with the intent of fixing it later. But later never comes. Sometimes bad code is also written to blend in with other bad code in the system. Good developers are often forced to follow bad standards. The people in charge are not always the people who should be.
It’s also entirely reasonable for developers to have outdated knowledge. Things change quickly and dark matter developers (aka, people with social lives) aren’t living and breathing code 24/7. They miss things. The code might also not be bad – it could just be done in a way that you aren’t used to and may be that way for a good reason.
Have a better solution in mind
All the criticism in the world is useless when you can’t articulate a better solution. Find one or shut up until you can. This exercise also has another benefit. It forces you to give real (objective) reasons for why a piece of code isn’t optimal, instead of subjective ones. This has the effect of making discussions with reasonable people more reasonable and discussions with less reasonable people more obvious.
This is also a good time for you to critically examine your reasoning in regards to the “better” solution. If your solution is only “better” because it addresses potential issues that will never actually arise, it’s not better, it’s just “gold-plated”.
This is especially helpful when either you or the other developer are new to the team, as previous teams may have had a different set of constraints, which changes the meaning of “good enough”. This forces you to be able to articulate your reasoning and assumptions, so you can find out which are wrong.
Determine whether your better solution was actually workable at the time the code was written
With your “better” solution in hand, now you need to know whether that better solution was reasonable when the code was written. There may have been other constraints in place when the bad code was written (or potentially just stuff that the developer didn’t know), and it’s important to make sure you know what those were.
It’s a lot easier to come to somebody with suggestions for improvement when you can acknowledge that they didn’t have a lot of choice when they wrote the earlier code. A bit of empathy helps here. Further, if there is a repeated pattern of bad situations leading to bad code, being aware of it in general can help you avoid writing bad code yourself. It’s also worth bringing up to management if you can get away with it.
Figure out how you could refactor the code
If you still like your “better” solution, you need to figure out how to get there from here. Unless the code you are looking at is very small, this is something that will have to be done in stages. Figure out several steps to get the code into a better state, with the code being in a working, stable state at the end of each step (so you can be interrupted and return to it).
Most of the reasons that code is bad have to do with how that code interacts with other code in the system, either due to calling that other code or being called by it. As a result, you need to carefully consider how far out into the rest of the system your changes will spiral and make plans for limiting scope.
If you don’t have tests around the existing code, now is a good time to add tests for the existing version of the code. This lets you take smaller, more controlled steps when working on a fix with another person, especially if they don’t like the entire set of your suggestions for a fix.
Now approach the person at the right time in a non-threatening manner
Don’t tell them there is a problem in their code. Instead, ask if you can run something by them, or if they can help you understand something. It’s more important at this phase to keep from making them defensive than it is to be right.
Don’t do this when they have a looming deadline, personal/work drama going on, right before they go on / return from vacation, etc.. Don’t make this harder than it has to be by being oblivious to the political landscape.
It’s also generally best (unless there have been issues with this person in the past) to have this conversation in a one-on-one environment. A team environment is going to make people defensive at best and isn’t useful for this sort of thing. It’s also better to do this verbally, face to face, instead of over a chat client or as a comment in a pull request.
Remember to be respectful. After all, we all write the occasional bit of garbage code and when you talk to someone about theirs, you are training them in how to treat you when they find your mistakes.
Mind your wording
Avoid strong, subjective value judgements of someone else’s code. Calling it “terrible”, “garbage”, or “spaghetti code” may FEEL accurate (and we might agree with you), but it isn’t a useful way to actually get it fixed. Unless you are in a management role, your goal is to solve problems, not to browbeat other people into not causing problems.
Generally speaking, if you word your suggestions for improvements in such a way as to describe them as opportunities instead of improvements, it’s easier to get the other person on your side.
Also avoid accusatory wording. Asking what the reasoning was for a structure, rather than saying “why”. “Why” is more likely to provoke a defensive answer than something less subjective.
Work towards your answer without giving your answer
While earlier we suggested that you figure out exactly how you would do it, in general it’s not the best idea to simply show up with the fix. “I reject your code and replace it with my own” comes across as extremely cocky. And it’s a lot riskier if you are wrong.
Instead, ask questions to lead them through the thought process you had when you were looking at their code earlier. Not only is this likely to still lead them towards the same general answer, but it also makes it more likely that they will consider things in the same way in the future.
This also forces you to check your assumptions at each stage. An incorrect assumption early on can lead to a wildly different answer. If people initially get the impression that you are just throwing out poorly thought out solutions to get attention, it makes it harder to get your point across. This approach also keeps you from running roughshod over your more timid teammates, which helps avoid resentment.
Ask good questions
Be specific. Don’t ask “is this code good” – instead ask “is this code good for (x) situation”. Start with enough information to get an answer. It’s very irritating when the only answer is “it depends”. You want those variables out of the mix so that you can get a real answer.
Prefer questions with straightforward answers rather than essay questions. Answering questions is probably not high on someone’s list of priorities, so make it efficient for THEM if you want an answer. Make it obvious that you’ve put the work in to be able to ask the question. Remember that the person answering is also having to make assumptions about your knowledge level – you’ll get more useful answers if their assumptions are closer to correct.
What to do once you agree.
Just because you have a better way of doing something doesn’t mean you need to fix it right now. If your team is under deadline pressure, or has a lot of people working near the problem code, it’s usually better to leave it alone until that isn’t the case.
You also may have to agree to a partial fix to get rid of the worst aspects of the problem, with a better fix later. Don’t let a desire for perfect solutions stop good solutions from happening.
At some point, you’ll have to let management and product owners know about the problem so that you can make sure that they don’t find out the hard way. Depending on your organization, this might need to happen AFTER the fix occurs (especially if they are feature-driven and hostile to code cleanup efforts).
Tricks of the Trade
Watch what you allow yourself to get attached to in you life. While attachment is not inherently bad, it will cause problems when you become attached to the wrong things and wrong people in your life.