Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

A bogus study on code review

blog.wesleyac.com

Post reply on HN
Print view View original

11–20 of 61 posts

Re: A bogus study on code review

#11

Post by gburt »

Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it.

We've done a few strategies at places I've managed code review:

1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do what it says."

2. Have programmers highlight problem areas (structural/architectural/clarity) in their own code before submitting the PR. This is draws the discussion to the high ROI locations quickly, while not prohibiting the reviewers from catching more minor issues.

3. Clearly indicate that the goal of code review is to, yes, impart style and feel on the rest of the team, but largely to avoid duplication, resolve bugs and improve everyone's skill. It is a within-team visibility and knowledge sharing process that also happens to catch meaningful defects.

Re: A bogus study on code review

#12

Post by maxxxxx »

alangpierce wrote: post #10
maxxxxx wrote: post #5

Earlier quoted context omitted.

A real code review takes a lot of time. With the time available typically you can only find superficial things like style.

It depends a bit on context, I suppose, but if you spend most of your time/brainpower in code reviews on style issues, then I think you're probably not making a very good use of time. I think it's much more important to look at the problem being solved and the solution and to think through whether it makes sense. Often times I prefer to first have a conversation with the submitter where we talk through the structure…

I totally agree with you. However, in my experience the time you get for reviews is often not much and the developer already has moved on to something else and doesn't want to deal with the review. Obviously this is an organizational problem but one I have seen a lot.

Re: A bogus study on code review

#13

Post by alangpierce »

Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

Out of curiosity, what's the context in which you think developers should stop doing code reviews after 3-6 months? Every place I've worked has required code reviews for all changes (except in emergencies or when the author thinks the change is trivial). For example, how big of a company are you talking about, and how critical is correctness and maintainability? My impression is that skipping code reviews mostly makes sense for early startups, internal tools, prototypes, and other situations where velocity is more important than quality, but that most companies should have pretty much all code go through review.

Re: A bogus study on code review

#14

Post by szucs »

gburt wrote: post #11
Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it. We've done a few strategies at places I've managed code review: 1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do…

I found that the best coded reviews are always from small change sets. They tend to have more comments on what the code is doing rather than what the code looks like. Sometimes this can’t be done though.

I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of their work day understanding that part of the code. Keep in mind the culture of the team needs to allow this to happen. It pays off in the long term.

One of the best reasons for code review, hammered into me by two very experienced developers, is high visibility of changes and the opportunity to learn more about what other developers are working on. Don’t underestimate this benefit.

Ultimately every code review doesn’t need to have a high net benefit for it to be good for your team.

On a side note we didn’t have any style guide at my old workplace but found that our code reviews were incredibly high quality for core components. Part of this is culture and the other part is the people you work with.

Re: A bogus study on code review

#15

Post by maxxxxx »

gburt wrote: post #9
maxxxxx wrote: post #5

Earlier quoted context omitted.

A real code review takes a lot of time. With the time available typically you can only find superficial things like style.

Have you ever actually been constrained in how much time you can spend on code review? I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.

In most companies I have seen code reviews were scheduled in addition to the regular workload. This means you have to do find extra time to do them. There are also only a few people who can give a meaningful review of complex code.

I am now a believer in pair programming. Code review is already done during development time.

Re: A bogus study on code review

#16

Post by majormajor »

gburt wrote: post #8

I've read this and similar posts on this study a dozen times in the last few weeks. I think the data is poorly interpreted and what you're really seeing is that shorter pull requests elicit better feedback (in this case "more defects per line"). In my experience running code reviews, shorter pull requests, presumably due to their reduced effort necessary to understand, tend to get better review, review that is more t…

This matches my experience, and also has implications for date-driven projects. People have two typical responses to long code reviews: skim and ignore, or go in depth and ask a lot of questions because there's a lot going on that's new. If it's a critical feature, I try to push the team to not just ignore, so the longer someone works in a silo, the longer it takes to merge those changes back in with everything else, the higher the risk of getting major "why not do it this other way instead" late feedback throwing a wrench in everything last minute.

What I've taken away from recognizing this pattern is to ensure everyone's checking in frequently, even in the early design and idea phase, so that nothing ever turns into a bad surprise in a code review. Depending on the seniority of the dev driving the feature, there isn't a single one-size-fits-all prescription for what's a "too big" code review, but the rest of the team's job is to make sure that whoever is driving the feature never ends up in a position where their technical choices put the delivery at significant risk - nobody wants to feel like their team left them to sink or swim and then only offered criticism when they finally submitted their code for review.

I'm also a huge fan of the described "identify what I dislike about my own code when submitting it for review" practice, too. I'll come straight out and ask "I don't like how I did this, anyone got better ideas?" and I've seen other people start picking this up on the team too.

Re: A bogus study on code review

#17

Post by majormajor »

gburt wrote: post #11
Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it. We've done a few strategies at places I've managed code review: 1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do…

My main strategy is what you call (3). I've found that if I take a strong stance on code style - namely, "I don't care about this style matter, I'm going to approve your reviews regardless of it, and will call people out for blocking code on purely style matters" - then I haven't gotten much pushback, and the teams naturally loosely find a happy medium shared style that includes automated linting type stuff so that there's an objective criteria everyone's on board with but little personal opinion-based quibbling.

I like code reviews to end up talking about "does this functionality belong in this file/block/class/whatever?" because that's what I find turns into tech debt if the whole team isn't on board with the strategy, and so I don't think anyone ever outgrows peer review on that. Those are critically important decisions, and lord knows I've inadvertently gotten it wrong as often as anyone else I've worked with. It's not that any particular dev needs oversight, it's just a really hard problem as shown by the fact that pretty much every codebase at every company turns into a big mess over time.

It's possible one bad actor could still mess things up for a team with that, but then that's a managerial problem. If someone's causing the whole team to slow down, that's a problem that's fairly easy to communicate to them (and if they aren't listening, it's easy to communicate to the rest of the team in terms of "this is why we had to do something, you don't need to be worried yourself").

Re: A bogus study on code review

#18

Post by gburt »

szucs wrote: post #14
gburt wrote: post #11

Earlier quoted context omitted.

Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it. We've done a few strategies at places I've managed code review: 1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do…

I found that the best coded reviews are always from small change sets. They tend to have more comments on what the code is doing rather than what the code looks like. Sometimes this can’t be done though. I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of the…

With some reflection, I think you're correct on the understanding problem.

The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature.

As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment.

For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.

Re: A bogus study on code review

#19

Post by jacquesm »

Clubber wrote: post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

1) but less time than looking for bugs from production symptoms

2) So lay down the law: no bitching over style, add a style verification step to your CI

3) you are still stuck on style.

I suggest you never skip code review, stick to functionality and make sure everybody's stuff (including the gurus) gets reviewed.

Re: A bogus study on code review

#20

Post by valuearb »

maxxxxx wrote: post #15
gburt wrote: post #9

Earlier quoted context omitted.

Have you ever actually been constrained in how much time you can spend on code review? I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.

In most companies I have seen code reviews were scheduled in addition to the regular workload. This means you have to do find extra time to do them. There are also only a few people who can give a meaningful review of complex code. I am now a believer in pair programming. Code review is already done during development time.

We just do code reviews on every commit, it’s just part of our flow. My experience is it’s far more productive than paired programming, our team doesn’t care about style issues and most code is clearly correct. So review of 4 hours of work can usually be done in less than 20 mins.

As far as cutting in the schedule, we get done what we get done. The schedulers can bite me, i wasn’t the one who mislead them to think dev estimates had any accuracy or usefulness.

Post reply on HN