How should I informally criticize my coworker's code?

Question

We don't have any sort of formal code review system in place. I'd like to have one in the future, but policy change can be incredibly slow to implement.

In the meantime, I keep having to work with and fix code that I don't think would have passed code review if we had it.

Is it appropriate to go directly to the coworker responsible? Would it be presumptuous to offer suggestions ("I'd have done X this way instead"), or should I just let them know their code has issues ("X is difficult to read")?

I'm not trying to shame anyone, I just want them to stop writing code like this in the future.

Answer 1

Two strategies

1. Socratic. Talk over the code with your co-worker, asking for explanation on the confusing parts. You have to tactfully "play dumb", which is a needed skill for a good coder anyway. Ask questions that cause self-revelation like "Is that a fast way of doing sub-queries?", "How do you keep track of all these short and similar variable names?", "Could we use a helper function to replace these three repeating parts?", etc. The trick is to "fool" them into thinking they had the idea of how to make it better, which you then reward with utterances like "yeah, that would be really nice/fast/clear".

2. Humor. Criticism can be hidden behind sarcasm and jokes. Silly names and analogies are clutch. "Can you give these vars more meaningful names, we aren't all Johnny Mnemonic like you", "Is your tab key broken?", "Holy repetition batman!", etc. If you're obviously joking, you can more easily get away with criticism, as long as it's fairly indirect. Mix in some self-deprecation as well, ex: "Oh, I hate these kinds of adapter routines, they always made me look like a noob when that complicated nesting had an unhandled exception, so now I flatten the queue before I hand it the callback by...". Make sure not to try this out in front of other workers, that could be intimidating with the social aspects on top of the incoming fodder.

No matter how softly you deliver the bad news, expect some defensiveness and backlash. Programmers are insecure because their entire jobs centers on making choices, so they put themselves out there hundreds to thousands of times in any given application's development. Be kind and conciliatory and ready to offer stuff like "Not the end of the world", "I've seen worse", and so on if needed.

Answer 2

Wait for another bug. According to your story, it will happen. Then ask him some help, ask him to explain what he did, how and why, because you don't understand the bug.

Then, explain your opinion and show him what you think he should have done. Do it with respect and don't forget that code style is personal and because you would have done it differently doesn't mean your way is better (even if it is, he won't think so).

Answer 3

Ask if you can work with them for a short time. You want to show them something that you're having difficulty with, and want to work with them and their code, so you can together come to a solution. (In other words, you're not criticizing their code up front, but asking them for help.)

Then do some pair programming, showing them what you need to do, and when you hit some of their code that causes you pain, explain the pain. Tell them what it seems to you would be a good solution, and ask if they see reasons why your solution has problems.

Approach this as a team problem to solve, and not as something where you have the solution, but where you need their help to come to a solution that both of you can live with. Be open to reasons they code the way they do, which will make it easier for them to be open to seeing your issues.

In the end, they may change. Or you may change. Or perhaps nothing changes. But you'll be communicating better (providing you always listen to them), and it will be easier to make suggestions and work together when you've done it before.

Answer 4

One thing to keep in mind: just because you're in a large, slow-to-adopt-things organization, doesn't mean your team has to be slow-to-adopt. I work in a large organization that doesn't have formal code reviews (and probably won't for at least several more years)... but I managed to sell our boss on doing code reviews for just our team. You might want to start talking with your boss to see if you could get a makeshift review process set up for just your area?

As for handling things now? First step is getting a read on how the coworker will interpret critique. I've got one coworker that outright refuses change. And I don't bother sending him code feedback - it'd be completely pointless. But another one of my coworkers loves improving his skills, and I wouldn't hesitate.

Also, here's my general recommendation: make it casual, and make it include the good things you noticed (keep in mind, positive feedback is more important than negative feedback; you'd hate for them to stop doing the good things, right?) For example, here's an email I might send that second coworker.

John,

Hi - I was doing some bughunting, and I found a few things in Something.cs

First, your function names are awesome - it let me know exactly what they were doing, and it cut down my bug-hunting time by quite a bit. Same with the variable names, for that matter.

But I noticed that you've got a loop in the GetPermissions() function that doesn't exit out correctly if the user's on VPN. Could you take a look at how you wrote that while() loop, especially the part where it returns instead of breaks on some of the conditions? Maybe the loop code's getting complicated enough to justify breaking it into its own function?

-- Kevin

Answer 5

Keep things objective and judgement-free

Don't play dumb, don't try to use humor, just discuss the objective facts without making any sort of judgement.

If it's about a bug you're fixing, something like

Hey X, I was looking for the solution to bug #123 and I think I've found it, would you mind having a look at it together?

If the response is some variant of "I don't want to" then leave it alone and hope that formal code review practices will be put into place soon.

If the response is affirmative, once again just discuss the facts without any judgement:

If I read this code correctly, here we foo the bar by fizzing the buzz. I'm thinking that when the foo is X and the bar is Y, this could cause the result that we see in the bug. Do you agree?

This keeps things open. The answer might be that your thinking was actually wrong and you didn't actually find the cause of the bug, or things may be more complicated than you thought. It also makes it really easy for the other party to agree with you and start talking about the solution instead of dwelling on the problem.

For general code issues, the template is similar.

Hey X, I was working on feature #123 and I came across the code for Y, would you mind if we looked at it together?

and

Right now we're fooing the bar by fizzing the buzz. I'm worried that [insert concern here], do you think it would be better if we nurble the blurble instead?

If the difference between your solution and their solution is just stylistic , there are no code style guidelines agreed upon which the current solution violates, and the current solution doesn't cause any issues, just let it go. That doesn't mean that you should always let it go if code is hard to read. "I was having trouble interpreting this bit of code, I'm worried this will become a problem in the future during maintenance" is a valid concern.

Answer 6

Work with your manager to institute a formal code review process. There are many free software applications out there, or you could make it as part of the pull request process when using git.

When doing a code review, don't be pedantic. Don't nitpick small, trivial details like whitespace or alignment. You should use an automatic code formatter that triggers when the code is saved in the ID or as part of a precommit hook. This also includes Linters with automatic fixers (for example, if you're using mixed quotes, it should automatically convert them all to the same type. Reduce the stress on your developers to memorize everything)

Encourage your team to do more peer programming or a "watching" code sessions. A lot can be learned from simply watching how some uses the terminal like new commands and techniques. Allow them to interrupt and ask questions like "what's joe" or how did you get your git log to show that?

Take this as an opportunity to walk through critical thinking process together. Like walk through the steps you go through to solve a problem.

Don't be Condescending: Most developers already have imposter syndrome, berating and belittling them only reinforces it and demotivate your coworker.

Celebrate the little wins: It may sound stupid, or undeserved, but compliment your coworkers regularly for small achievements and recognize how they are important part of the team. You may say "but I only compliment people when they really deserve it or people will think its fake" numerous studies in management theory show quite the opposite, productivity goes higher and people go the extra mile to meet your requirements.

Think like a mentor, not like a teacher. Think about how we can improve this coworker like a team with that member. Be willing to be a fount of information or a reference. Point them to the line number of documentation they should be looking at. Show them how they can teach themselves to be better (blogs you read, share reddit posts, tutorial videos, or O'reilly books you found really valuable to read). It may seem pedantic, but do sit down with them and patiently show them how you go through your thought process for googling problems. Start an office wide, "best programming books list" and get your manager to buy them and have them in an office library. Define clear coding practices your office adheres to.

When all else fails, find a new job. Maybe its a reflection of your work environment and that you've outgrown it and may be time for you to look for new works. If you feel like you're the smartest person in the room, its time to find a new room.