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

21–30 of 61 posts

Re: A bogus study on code review

#21

Post by kazagistar »

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…

The automatic formatter, extensive style guide, and linter helps.

We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else.

Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.

Re: A bogus study on code review

#22

Post by valuearb »

maxxxxx wrote: post #12
alangpierce wrote: post #10

Earlier quoted context omitted.

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.

On my team all code has to be reviewed before it goes to QA. We strive to do it same day, to make it more productive for reviewers and writers of the code.

It’s not too much overhead. Even though most reviews are satisfactory, occasionally it catches something that’s a good learning opportunity, and that’s golden.

Re: A bogus study on code review

#23

Post by gburt »

kazagistar wrote: post #21
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…

The automatic formatter, extensive style guide, and linter helps. We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else. Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.

Another thing to add, is that for "obvious" style suggestions (i.e., changes that everyone is nearly certain to agree on, though they might not agree on the importance), I encourage the reviewer to simply make the change.

This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.

Re: A bogus study on code review

#24

Post by kazagistar »

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

One of the biggest pros is that reviewing code keeps you up to date on the current state of the code base. I have, at least briefly, read and understood every line of code in our teams code base. I might not remember it perfectly, but I still feel like it is fairly easy to dive into and change any part of it, even the parts I haven't worked on in a while. This, plus occasional informal architecture standups, means we have a lot of flexibility in terms of developer allocation for tickets.

Re: A bogus study on code review

#25

Post by valuearb »

maxxxxx wrote: post #5
prepend wrote: post #3

It’s also really important to define defect during these reviews. I worked at a company where code reviews would identify lots of superficial errors like indention, improper documentation, and missing metadata. You want these in quality code based, but if this counts as a defect, it’s not as important as serious bugs being identified and corrected.

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

Not sure why a review would take so much time, mine typically take 20 minutes and I do two a day.

We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done.

What am i missing?

Re: A bogus study on code review

#26

Post by u801e »

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

> Cons:

> 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia.

This can be mostly addressed by using a linter. In our organization, we have a rule that if there is a glaring style issue, we ask whether the submitter had actually run the code through the linter before submitting it for review.

On the other hand, if the linter does not complain about some code, but the style it's written in is not agreed upon, we update the linter configuration to handle that case if the rest of the team agrees. Otherwise we leave the code as is (assuming it works as its supposed to).

> 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine.

I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.

Edit: s/submitted/submitter/

Re: A bogus study on code review

#27

Post by gburt »

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.

My teams also assign code review "in addition" to the regular workload, but this is always in the context of an agile team effort: we get done what we get done [1]. A developer takes a story and works on it until it is solved (or splits it appropriately), then another developer (or a few) are expected to do review.

--

[1] I might be measuring productivity with story points in the background, but there is no "scheduled workload," this is merely an averaging/planning exercise.

The (correct, IMO) managerial effort to improve individual productivity is much softer and more understanding than that and comes with the understanding that your output takes many forms. Some of my best developers directly complete almost zero stories in a sprint because their time is spent on code review, pair programming, direct assistance and architecture discussion.

Re: A bogus study on code review

#28

Post by valuearb »

alangpierce wrote: post #13
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 make…

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable.

Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a more senior devs code can teach a junior a lot, and occasionally catch significant errors.

Re: A bogus study on code review

#29

Post by maxxxxx »

valuearb wrote: post #25
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.

Not sure why a review would take so much time, mine typically take 20 minutes and I do two a day. We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done. What am i missing?

It probably depends on the work you are doing. We do test harnesses for hardware and most reviews are the result of a lot of experimentation and learning about protocols. In order to understand that code you need to know the characteristics of the device you are interfacing with. This may take weeks for a reviewer.

Re: A bogus study on code review

#30

Post by e40 »

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…

I directly manage one team, and oversee another, both use gerrit (the code review from Google, used on Android).

I 100% agree that the shorter the changes, the better the reviews.

I am 100% sold on code reviews, because of:

1. the number of bugs found before they were pushed,

2. everyone writes code for other people now, whereas before we wrote the code for ourselves, and

3. improvement to APIs before they hit the code base and documentation.

Every single one of these things was a surprise to me, and I've been doing this a very long time. I never would have believed how happy I would be with the result.

Regarding other comments on annoying, small comments on style, etc: this is a problem with your people and your team leader. If this happened with my people, I'd have a conversation offline with them and tell this to not use code reviews in this petty way (to push their own styles). One thing it made me realize, every organization needs a style guide, but you need to make sure it doesn't go too far. Give people some latitude, just put the important stuff in it.

Post reply on HN