7

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.

  • 6
    Are you a peer or above or below them in the hierarchy? – thursdaysgeek Jan 16 '18 at 19:30
  • @thursdaysgeek We're peers. But this can happen with coworkers that are my senior or junior, too. Which is why I want a formal review system so badly, since everyone would benefit from an extra set of eyes. But until we have that, I'd like a way to handle it in the meantime. – FirstLastname Jan 16 '18 at 19:44
  • If they're not your junior, the only thing to do is go to your boss and ask for formal coding guidelines. Otherwise it's just your subjective opinion vs. there's. Maybe they think your code is difficult to read. – AffableAmbler Jan 16 '18 at 19:50
  • @AffableAmbler Readability isn't the only thing at issue. Say their code introduces a bug that I've been assigned to fix. Unless I tell them something, they won't realize and will likely make similar mistakes in the future. Should I really just keep quiet as I wait for a formal system (which will take months)? – FirstLastname Jan 16 '18 at 20:09
  • 2
    Possible duplicate of How to avoid bad practices of work by employees – IDrinkandIKnowThings Jan 17 '18 at 20:43
12

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.

  • 5
    I strongly disagree with the "humor" way. For him you are making fun of his work and if he is a little insecure, of his skills. – LP154 Jan 17 '18 at 6:17
  • 1
    @LP154: it can be hard, needs funny>mean, so if you are not a joker, maybe not the best time to start. I would say one could always be direct but most coders have trouble being direct with humans... – dandavis Jan 17 '18 at 6:20
  • If he has trouble being direct with humans, IMO he will have trouble making a joke that his coworker won't take the wrong way – LP154 Jan 17 '18 at 14:21
  • Nice answer. For #2, I would add that this would probably depends on the nature of the OP's relationship with the dev, if they are already casually joking around in the office, this would be great. But if they have this both awkward professional relationship I would suggest to stick with #1. – Isaiah3015 Jan 17 '18 at 14:23
  • agree about humor. apply as needed. if rash develops, discontinue immediately. – dandavis Jan 17 '18 at 19:21
5

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).

2

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.

1

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

0

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.

  • I've changed the wording, as "do you mind?" takes a negative answer as permission to proceed ("not really" means "no, I don't really mind"), whereas an affirmative answer indicating that the respondent does indeed mind ("Yes, I mind"). Feel free to amend my changes. Either way, Cronax, delete this comment. – Mathieu K. Jan 18 '18 at 3:15
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.