DaedTech

Stories about Software

By

How to Address Your Coworker’s Bad Code (Part 1)

Editorial Note: This post was originally written, some months back, for the SmartBear blog.  You can read the original at their site, and. while you’re at it, I recommend checking out other posts there as well.  There are a lot of good authors over there, and I’m flattered to be included with them.  This is part 1 of 2.  I’ll publish 2 here later as well, but it’s already published on their site if you want to read it immediately.

Ugh. You’re sitting at your desk, trying to chase down a bug that’s been reported, when it happens. The hunt takes you into some method that inspires you to do a double take. It’s about 1,200 lines long, it has switch statements nested three deep, and you think (but aren’t sure) that it does the same thing two or three times in a row for no particular reason. You look at the source control history, see that this is another “Bob special,” and start thinking about finally having a long overdue talk with Bob so that you don’t have to keep cleaning up these messes. That sure won’t be a fun talk. So how do you approach it?

ScaryComputer

Philosophically Speaking

Let’s be clear about something up front. Getting really good at telling teammates that their code is littered with problems is like getting really good at breaking into your car after locking yourself out of it: it’s tactically useful in the moment but indicative that you need a better overall strategy. Your goal shouldn’t be to master gently telling coworkers about their bad code but rather to make the mastery unnecessary. And I say that not as some kind of meta cop-out, but rather to put your strategy into context as an attempt to start or further a relationship.

When you’re part of a team, someone on your team committing bad code is a failure of everyone on the team—you included. So as you prepare for the intervention you’re planning with the person in question, keep in mind that you aren’t some kind of neutral crime scene investigator, sizing things up antiseptically. You’re part of the problem, and you share in the responsibility. Your team. Your code. Your problem.

The good news is that if you approach this conversation constructively, you’re taking the first step toward fixing the problem, the code, and thus the team. So the key is making it constructive.

How Not To Make It Constructive

Before I go into detail about how to approach this conversation, I’ll give you a quick rundown of things not to do.

  • Don’t have the conversation when you’re frustrated or angry. Instead, wait until you’re calm and rational.
  • Don’t get into this unless there’s a demonstrable problem. If you and he just have different casing preferences or something, the tension you create is probably going to nullify the benefits of standardization. Cosmetic coding standards and other relatively minor concerns can and should be addressed with automated static analysis.
  • Don’t rely on seniority or status in any way. There’s no faster way to breed resentment than forcing people to do things they don’t agree with “because you say so.”
  • Don’t expect to revolutionize someone’s entire approach in a single sitting and make the conversation a marathon affair. You want to have a clear and relatively concise message so that you get your point across without exhausting the other person. Improvement will happen over the long haul.
  • Finally, don’t say that the code is “bad.” That’s a useless, subjective way to categorize. Everything in software is about tradeoffs, so what you want to do is show Bob that he’s paying for quick and dirty coding with maintenance headaches for the team.

Build Your Strategy

You’ve already prepared a bit by reading what not to do, so now it’s time to complement that with what to do. There need to be three main components to this preparation: (1) the gaps you want to address, (2) the support for your argument, and (3) the outcome for which you’re hoping. Those three things are going to frame the discussion you intend to have.

The gaps are actual, specific problems with Bob’s code. You don’t want to stroll over to Bob’s desk, pull up a chair, sit on it backwards and say, “So, Bob, you’re pretty bad at this programming thing…great talk!” You need to decide what tangible items you want to address during this discussion. What’s the most egregious source of problems? Is it the gigantic methods? The nested switch statements? The duplicate code? Pick one or maybe two of these things to cover. Just as you don’t want to be critical and vague, you also don’t want to be critical and devastatingly specific, reading off 95 of Bob’s greatest coding flaws like some kind of departmental Martin Luther. There may be 95 things wrong with Bob’s code. But if you want to fix all of them, it’s important to lay the groundwork for a mentoring relationship because you’re definitely not going to fix all of them today.

Let’s say that you’ve decided to focus on method length as the topic to address. The next thing to do is build support for your argument. It’s a lot more credible to cite some supporting studies or widely respected industry figures on the matter than to march over to Bob and declare that his methods are too long. Build a case with evidence for the principle that you want to cover, and then also find specific, problematic instances in the codebase to discuss. The last thing you want is to be hand-wavy about the problem—you want to be able to point at it and say, “for instance, this right here is a really big method.”

Having picked your issue and built a case, the last thing to do is choose an outcome toward which to steer the conversation. So you’ve shown Bob a giant method that he wrote and convinced him of the evils of giant methods. “Uh, okay,” he’ll say. “So what now?” Decide ahead of time that you want to work together to break the method into X number of smaller methods or that you want to leave the code in a state where no refactored method is longer than Y lines. Whatever it is, pick something actionable so that you and Bob can cap the conversation off with a joint win.

At this point, you’re ready to have the hard conversation.  If you do it right, it won’t be nearly as hard as you might think, and it will serve as a productive starting point for a series of subsequent conversations that will be easier and perhaps even pleasant.

8 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
N
N
8 years ago

Nice post. Developers have ego. many of them have a lot of ego if they are ‘senior’. I think code review (can be peer code review) can prevent a lot of problems. One problem I sometimes see is with junior developers. some of them came out of college with college ideas like premature optimization. Another problem is if you are joining a team with ‘Experts’ that are nothing but great hackers – they are working there for years and they know all the scripts and manual configuration that you have to run to have a working environment. If you try… Read more »

Erik Dietrich
8 years ago
Reply to  N

I agree. I’d say that all team members being open to giving and receiving frank, but polite, feedback leads to a healthier team.

Ann Loraine
Ann Loraine
8 years ago
Reply to  N

And yet, the new person often knows about tooling that can help the team improve their process. Every time I’ve hired some-one new, he or she introduced some new tech that saved us time and effort. If that happens every time some-one new comes on board, the team starts to see the benefits of change: reduced tedium, more fun features.

Erik Dietrich
8 years ago
Reply to  Ann Loraine

It’s certainly my experience that new blood is vital for ensuring that things don’t get stale. When possible, I like organizations to have developers switch between teams on a semi-regular basis. It keeps things fresh and also incents the teams to do a good job avoiding high bus factor situations.

Sara Allen
Sara Allen
8 years ago

Our team requires code review before sending code out and this is so effective because we know another teammate is going to be looking at your code so you work a little harder so you can present clean code.

Also, we have a loose rule where if something takes too long to figure out, we call a team regroup to discuss and even tandem code to help break mental blocks. Takes a lot of unnecessary pressure off and we learn from each other.

Erik Dietrich
8 years ago
Reply to  Sara Allen

I’ve definitely encountered an observer effect in a good way. You tend to write better code and avoid shortcuts when you know someone else is watching. I experience this in the extreme since I do a lot of screencasts of myself coding 🙂

That’s a good group policy, too. It’s surprising to me how few groups tend to audio code for comprehensibility/maintainability — even ones that practice code review. Often they’re just trying to look for potential defects or conformance to some kind of coding standard.

William Notowidagdo
8 years ago

Good post. I had this kind of problem in the last few months. Pull request, code review and having our continuous integration service running code quality check on every commit to master branch seems the right way for us to improve our code.

Erik Dietrich
8 years ago

Absolutely. Automate all the things! I love this approach when it comes to code inspection/review.