9

I work as a developer in a small team containing 5 developers and a team lead. I have been on a project since its beginning so I understand the code better than others.

It happens sometimes that, I have given a code review, pointing out the mistakes my coworker made, and suggesting some solutions as well. He does some workaround and team lead merges the changes, since the task is completed.

Most of the time, my review points are towards the overall quality and functioning of the code, so even if he ignores them, it may not cause impact immediately but leaves a narrow path to move forward. This leads to a never ending cycle of bugs and fixes.

I'm getting frustrated seeing my review comments getting ignored and causing bugs.

  • 3
    Have you talked to your manager about this? Does your employer have any guidelines for code reviews? Does your organization have any guidelines for performance or maintainability for new development? – dwizum Dec 3 '19 at 15:51
  • What is the view of your lead on code review? Do they require it? – Matthew Gaiser Dec 3 '19 at 15:55
  • 1
    Do you use tests? If so you should suggest adding a test which demonstrates the potential/existing issues with the code. Hopefully the developer implements the test and can clearly see the issue – dustytrash Dec 3 '19 at 16:48
  • Does your project follow a code standard? If not, they may be assessing your recommendation as your coding preference so only taking parts important – Chris Dec 3 '19 at 16:59
  • 1
    I don't want to create a problem - often, the creation (or not) of a problem lies in the way things are brought up - not just because they are brought up. In other words, think about how you can frame this as a constructive conversation, instead of something more likely to create a problem (i.e. complaining or something). – dwizum Dec 3 '19 at 18:37
15

The most important thing to keep in mind is that code quality is dependent on the criteria you are measuring against.

In a vacuum, essentially every piece of code can be improved. If you have no standard to measure against, you can run the risk of either falling into a bottomless pit of optimization, or at best ending up disagreeing with team members over what counts as "good enough."

In business, software is usually a means to an end. It exists to fulfill some sort of business need or facilitate a business process. The software does not exist to be perfect, it exists to solve a problem. The standard that matters is usually "good enough" to solve the problem at hand. Of course, there are subtle implications around performance, quality, and maintainability, which it sounds like is where your concern lies. These gray areas are why standards matter.

Where does that leave you? What should you do to address your problem? I would suggest the following steps:

  • Determine if there are standards in your development team, in terms of code quality, performance, or maintainability. If there are, make sure your reviews follow them. Do not suggest things that aren't mentioned or enforced, or you may run the risk of "wasting time" on changes that aren't seen as important.
  • If you disagree with the standards, or they are not complete, address that issue directly - don't try to enforce your own ideas through rogue reviews.
  • If there are no standards, discuss the specific reviews where you and your coworker(s) disagreed with whomever is approving the new code (i.e. your team lead). Ask them how they make decisions on approving these merges. Treat their decision making process as the standard against which you should conduct code reviews.

If you've done all that and there are still running into issues, consider the following:

  • In your code reviews, reference the standards you are measuring against. It's easy to take code reviews as personal criticism. You can avoid that trap by keeping it about the code, and not about the people or the skills those people have (or don't have).
  • Have a casual chat with your coworkers. Approach it as a learning experience for you - not them. Ask them to walk you through their approach and why they made certain decisions. You may find that you end up understanding why they did something, instead of feeling like you need to correct them. Or, you may find that by asking them to explain something out loud, it becomes obvious to them that they need to make a correction (and you may not even have to be the one that points that out.) Sometimes, when someone is asked to explain their own work, it helps them see the problems in it.
  • Have a chat with your management. Explain that you're worried about how code reviews are being interpreted. Again, don't approach this discussion as if you have the answers and you're trying to correct someone else - approach it as "here's a problem, how can we proceed?"
  • 2
    Also, when trying to make a review be only about the code, it helps not to use the words "you" or "your" (especially "your code" and similar phrases). They make people feel like they need to be defensive, and create an "us vs you" mentality that doesn't help anyone. Instead it's "the code" or "our code", and "we need" to do things. – Player One Dec 3 '19 at 23:25
  • Great point. Or you can take it one step further, and simply not use pronouns. Just say, "this code does X" or "Maybe this code should be changed to do X." – dwizum Dec 4 '19 at 12:54
  • 1
    I like using "we". I have no empirical evidence, but anecdotally it seems like I always get better results in code reviews when I use it. I think it helps people realise that we're all in a team sharing the same goals. Obviously some reviews are still difficult, but I feel like "we" and "our" are great options to use whenever I can. – Player One Dec 5 '19 at 10:25
4

I've hit this quite a few times, the original coder sees the work as 'done' and is under pressure to get on with the next thing. Often your suggestion appears to be little more than a different style with no real benefit. Occasionally they just don't understand your suggestion, or it uses a technique that they've not used before and don't want to take the time to learn.

In these situations, I've offered to make the changes myself, and we'll both look through it afterwards and discuss which we like better. Sometimes I've discovered that my version wouldn't work, or that it ended up longer and more complicated. Some times there's so little difference that I've had to admit that it was just a style difference. However, ideally the new version is so much shorter, clearer and more robust that it's obvious to everyone that it's better, and it can serve as a model the next time anyone has a similar task.

It's hard enough for the other guy to admit that he was wrong and you were right, so help him out and don't make him feel stupid.

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.