4

I work for a company where code review is not treated very seriously as far as I can tell but I am also not sure what the purpose of it is.

Currently it seems as though code review is mostly a formality with just one developer giving lengthy and complex reviews. The other three developers rarely give comments and usually approve things in a few minutes. They also get mad if I have more than 1-2 comments on their stuff.

How can I figure out what I am supposed to do here? My manager just sent me a link about code review when I asked which didnt really help as only one person seems to do that.

4 Answers 4

5

Code review is a spectrum and its significance varies by company

In a past organization, I was tasked with code reviewing things that I barely understood. We were also under pressure to deliver features and the way to get praise was to have a higher velocity. So I just sat on the review requests for an hour, did other work, ran the tests, and approved them. I can't remember requesting any changes beyond removing console.logs that got stuck. We also had no real standards for it, so it was often chaotic and different reviewers would have very contradictory feedback. There was one library one dev loved and another dev thought was an anti-pattern, so they battled about whether that should be there.

One of the challenges with code review is that in many organizations, it has been a good way to burn a lot of time and not get credit for that time. Here, I was the new guy who was seemingly churning through work very quickly as a junior. That should have raised red flags that corners were being cut. It didn't, because I suspect it was not considered important. It just made the senior who did give more detailed code reviews seem slow (one of many reasons related to code quality).

In my current organization we are a bit more serious about it, but as soon as there are deadlines, it quickly becomes about giving it a cursory check for errors and sending it through, especially given the relative juniority of the team here now. Even if feedback is given, the PR is usually approved with just a note about possible changes. Urgency is usually conveyed in the request, so whether the person asking says "review this please" or "approve this please."

At a company a friend works with, code review is a contractual requirement, but no PR has ever been rejected or had changes requested as they bill by the feature. Code review is a mere matter of box ticking there. It has no organizational purpose other than meeting a requirement. Approval comes 2-3 minutes after the PR is submitted.

Another friend is at a company where code review can take weeks as it requires so many people to view it, including people not from the team to ensure it is still usable to people who don't know the codebase. There, it is taken extremely seriously and it is expected for developers to spend hours on it if required.

So basically figure out what code review is supposed to mean at your company. In your company it seems like something one developer champions and everyone agreed should be done and could easily be done but really isn't considered important or incentivized. What you want to do in that case is up to you and what your manager wants.

1
  • This. Exactly. "So basically figure out what code review is supposed to mean at your company"
    – iLuvLogix
    Jun 19, 2021 at 10:59
4

Bring it up in a team meeting/retrospective and get your team on the same page about how you use code reviews and what the expectations around them are.

Code review culture varies between companies, and two "strong" companies can have very different approaches to how a code review is done. There are places where code reviews are huge affairs with big bodies of code being put under extensive scrutiny, and there are places where a code review should be as few lines as possible, with big changes spread across a dozen commits and with few brief comments if any per review.

What you describe may be the case of a team lacking in engineering maturity, but it might also be a sign of a close-knit team of strong performers annoyed by what they see as nitpicking.

Whichever that is, you should communicate with your team about it and either adjust to how things are done or suggest another approach and get the team onboard. If nothing else, knowledge sharing is always a good argument for more extensive code reviews especially when you're the new person on the team.

4

My manager just sent me a link about code review when I asked which didnt really help as only one person seems to do that.

Your manager is the person who sets the standards for the team. Since they sent you some fairly specific information when you asked, it sounds like they do have a standard for code review in mind. Therefore, you should follow it.

If other team members aren't following it, perhaps it isn't well enforced. You could bring this up with your manager in response to the link. Something like, "Thanks for the link. I was a bit unsure, because not everyone on the team uses code review the same way. How should I proceed when other team members want to treat code review in a different way?"

This may or may not lead to a wider clarifying discussion with the team. Hopefully it does, and hopefully the team can find practices they agree on. But if not, at least you'll know what your manager is looking for.

5
  • "Your manager is the person who sets the standards for the team.". That is not necessarily true, in self-organizing teams, it may very well be the team that sets the standard. Jun 19, 2021 at 11:16
  • 1
    @MarkRotteveel Every team should have a leader. A team that doesn't have a leader cannot handle the case where consensus cannot be reached, and will begin to break down at that point.
    – B. Ithica
    Jun 19, 2021 at 18:15
  • I have worked in self-organizing scrum teams for quite some time now, and there is no leader. There is a scrum master, which is not a leader. We have a product owner, which only decides on priorities of features, and we have a manager (with half a dozen teams), which doesn't interfere with the day-to-day operations of the team and are more for personnel and personal issues. Maybe the team manager would mediate or break a tie when consensus cannot be reached, but that is very rare. Of course there are some company-wide or department-wide things, but there is no team leader. Jun 20, 2021 at 7:21
  • If things do "break down", we call in the help of a team coach. Jun 20, 2021 at 7:25
  • 1
    @MarkRotteveel "Maybe the team manager would mediate or break a tie when consensus cannot be reached, but that is very rare." It doesn't matter that it's rare; it does happen, and when it happens, the team needs this person with the authority to pick the direction to go. At any rate, in the context of the current question, the team the OP is on is pretty obviously not a "self-organising" one, or they would've said to the OP, when joining, "This is how we've agreed to do code review, please go with it and if you disagree give us your thoughts during the next retrospective meeting."
    – B. Ithica
    Jun 21, 2021 at 7:37
1

Some companies handle code review as a "check box" item. In these companies there is little consistency in the reviews; mostly because review standards haven't been created. This causes a lot of contention around reviews, as one person's review isn't an equivalent to another's; and, often the reviews will have wildly different outcomes based on who reviews your code. In the worst cases, reviews become an enforced "you must write it the way I would have written it" even if that way is objectively, provably worse.

Other companies handle code review as a checkpoint to ensure quality code, and believe in quality code by spending more time and effort to fix issues before they happen. Typically their reviews have standards that are clear, in the sense that when you violate them, it is not a matter of opinion if they were violated. Because they put the work into developing the review, a reviewer can be in violation of their policy, a reviewer can be trained to follow their policy, and their reviews tend to be more consistent as every reviewer is checking the code against the same list of violations that would cause the review to fail.

Your company resembles the first one I mentioned. That one reviewer that writes more isn't guaranteed to be doing a better job (they might be) than the other two.

It is a far, far more difficult thing to change the culture around reviews than you might consider. For example, the two that aren't doing much will not appreciate doing more with no additional immediate benefit, and the one that is doing too much will not appreciate being told to do less. If he is too opinionated in non-helpful ways, he might convert the industry findings into opinions and then complain that some opinions (industry findings, aka "your opinions") are being honored while some (of his) aren't.

Basically, to see improvement in this area, you can suggest improvement; but, odds are that your suggestions will not become implemented and may breed resentment. Handle your suggestions carefully, and make them small and verifiably achievable.

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