Tag: criticism

How to criticize code

Dear new developer,

Criticizing code (and software solutions in general) is an important skill. It helps transmit norms, increase team knowledge, and improve solutions. But it isn’t something that comes naturally; at least, I had to learn how to do it. Similar to learning how to edit other’s essays, you must learn how to critique team member’s programming.

First, understand the context. There are two types that matter. The first is the business context. What problem is this system trying to solve? What limits does the business have (time, money, labor, understanding)?

In a startup, some of the code being written will be quick and dirty, because the startup is trying to find something that people will pay money for. Rapid exploration is more valuable than perfect architecture and maintainability.

You also need to understand the technical context. What is the scale/timeframe/level of the team’s knowledge? All of these things should inform your critique. I expect different things from an adhoc script in a data pipeline than I do from an embedded agent shipped to customers with little chance of future updates.

If you are part of a team, you may know these contexts, but they shift over time. So it’s always worth double checking before you begin a review to make sure you aren’t holding outdated assumptions. This can be as simple as a quick conversation with the author or team lead. It’s also helpful to regularly have these so that the entire team can be in the same mental space in terms of the engineering trade-offs they make daily.

Next, determine “which hill you want to die on”; that is, what is important enough for you to comment on. Which aspects of the code are important to you, based on personal experience, team goals, or ethical considerations? If you don’t know what the team values, ask! And then write it down so that others can benefit.

What you want to do with a review or critique is walk a line. You want to question and provide actionable feedback so that you and the author can improve the code and understanding of the system. But if you provide too much, the author may be overwhelmed or, worse, brush off your comments. You can ask what kind of feedback the author prefers; some folks have thicker skin than others.

Don’t focus on the small stuff. It feels like an easy win, but it really isn’t helpful. If you must give such feedback, preface it with “nit”. In fact, if you can automate away questions like spaces vs tabs or where curly braces go by using a linter that runs before a PR can be opened, all the better. Intent and high level feedback is where most of the value of code critiques lives.

Finally, a note on tone. You can be harsh with feedback, but don’t mix up someone’s implementation with who they are. They likely do that enough themselves; I know I sometimes get wrapped up in my code and take it personally when it is found wanting. The tone I find works best is one of assuming positive intent, but asking tough questions along the way. Trust that your team member has done the best they could, and try to focus on improving it and educating the both of you.

The end goal is to bring the code up to the current standards, or slightly beyond, of the codebase and team. Attacking anyone personally, even if they made a really ignorant implementation decision, actively undermines that goal in the future. I find that harsh feedback delivered in a kind tone is more effective than the reverse.

Sincerely,

Dan

Don’t Shit on Someone Else’s Work

Dear new developer,

There will come a time when you are looking at a system and trying to understand the choices behind it. You may be looking at a particular class, a subsystem, or a more fundamental choice, like the language or the system architecture.

And you’ll wonder what the hell the initial implementer was thinking. You’ll wonder why this system is still in production. You’ll wonder why someone didn’t fix this.

You will be tempted to trash talk this piece of work to your colleagues.

Don’t do this.

Why?

It’s not helpful. Now, it’s perfectly acceptable to point out issues and ask questions about why choices were made. It’s a good idea to suggest improvements, whether those be code changes, removal or replacement of portions of the system or something else. But complaining about the existing system doesn’t do any of that. It’s just complaining.

It displays a lack of empathy. Chances are you don’t know the constraints and pressures that the original implementer and past maintainers faced. Making judgements based on code without knowing the constraints is like hiring someone based on the color of their shirt–you just don’t have all the information needed.

Trust me, in the future you’ll face constraints, like lack of knowledge, time or labor or money, that will cause you to make choices that are suboptimal. At least once in my career I’ve come across a boneheaded piece of code. Cursing under my breath, I wondered “who wrote this crap?”. As I pulled up the commit log, my face fell as I realized that I had. Doh!

(What should you do if the code or system doesn’t work at all? In that case, the questions about constraints and understanding how a failed system was built are even more important. But again, complaining doesn’t help anyone at all.)

In short, when you are working in a codebase and trash talk it, you are the critic, not the man in the arena.

Sincerely,

Dan