14

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.

Improve this question
9
  • 8
    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, 2019 at 15:51
  • 2
    What is the view of your lead on code review? Do they require it?
    – Matthew Gaiser
    Dec 3, 2019 at 15:55
  • 3
    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, 2019 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, 2019 at 16:59
  • 2
    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, 2019 at 18:37

3 Answers 3

Reset to default
24

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?"
Improve this answer
3
  • 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, 2019 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, 2019 at 12:54
  • 2
    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, 2019 at 10:25
10

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.

Improve this answer
-3

How about you enforce a rule where PR can not be merged without your approval. And do you mark them as "change requested" when you make comments? Otherwise they are just optional comments.

Improve this answer
1
  • That's an excellent way to stop a project in its tracks. The obvious solution is that you make the change, and it cannot be merged without my approval. See where you get.
    – gnasher729
    Apr 12, 2022 at 10:35

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .