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

31–40 of 61 posts

Re: A bogus study on code review

#31

Post by Clubber »

u801e wrote: post #26
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 w…

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

I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value.

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

Eh, developer should be reviewing their own code fairly thoroughly anyway. If a developer can self review their code, then the second person doing it costs a lot and doesn't get a lot of value. I wouldn't expect glaring mistakes from a developer that has lasted 3-6 months. (it's a requirement for employment, IMO).

Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done. Like I said, I think code reviews have great value the first 3-6 months, then ever decreasing value after that. I'm also assuming competent developers. If an org is hiring cheap coders, they don't really value development efficiency in the first place and my theory doesn't really apply.

Re: A bogus study on code review

#32

Post by sharkdesk »

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…

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company.

Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get executive support to do so. My experience with code reviews is that they are 95% style nitpicks, 4% ways to make code cleaner and simpler, and 1% real bugs which impact end users. I only value the 5% myself. From a cost/value perspective, I haven't been sold on the value of doing code review on everything.

Not all developers feel that way, sure. But it is a mistake to assume all developers are driven by the same things.

Re: A bogus study on code review

#33

Post by sharkdesk »

valuearb wrote: post #28
alangpierce wrote: post #13

Earlier quoted context omitted.

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

Actually, someone is paying the team to solve business problems and create revenue.

Re: A bogus study on code review

#34

Post by watwut »

sharkdesk wrote: post #32
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…

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company. Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get e…

Reviewer must be able to explain and demonstrate why his remark is right. The petty code rewiew problem tend to happen in system where reviewer is assumed right and programmer is assumed wrong.

When original programmer has right to refuse review, the percentage of useless code review comments goes down. Most of them are power trip anyway, if you remove power from it people cease to make them.

Re: A bogus study on code review

#35

Post by Clubber »

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 think they lose their value in any organization. It's not a hard rule, because there are a lot of variables that could increase or decrease their value. I'll give you an example of each:

1. Decreased value: If a person is the primary (only) developer on a subsystem, any outside code reviewer will be less informed of the subsystem he is reviewing. I don't see any value in that other than get the other reviewer up to date on a subsystem he doesn't actively develop / maintain. I guess you could argue redundancies, but if you have a big system, having every developer understand every part of the system is unrealistic.

2. Increased value: if a developer is working on a new subsystem they aren't familiar with, then the 3-6 month window restarts. The 3-6 month window isn't the time of employment, it's really the time of understanding a particular subsystem.

My point is that doing code reviews in and of itself has decreasing value after 3-6 months. The 3-6 months is the time I'm estimating for a competent developer to familiarize himself with an existing subsystem codebase.

Also, any developer that still has issues after the 3-6 window of the same subsystem is a huge red flag. Code reviews will then just be a crutch for him and the entire organization when the real solution would be to get a better developer.

Also, I'm not saying any communication about code loses value. If I want a guy to look at my code and help me work though something (usually by just listening to me explain it), that has value. I'm generally speaking of the formalized code review that must happen after any coding change. That loses value after a developer is acclimated to the subsystem he has been working on, especially if the reviewer doesn't know as much about the subsystem he is reviewing (and has no need to know). I'm a very anti-hard-process guy. A process must have strong value, otherwise it's just another speed bump.

Re: A bogus study on code review

#36

Post by watwut »

valuearb wrote: post #28
alangpierce wrote: post #13

Earlier quoted context omitted.

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

"They should be the best tool you have for teaching and communicating best practices. "

For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice.

Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew.

If you have expectations, tell them in advance. It is just unfair to use code review for that.

Re: A bogus study on code review

#37

Post by Clubber »

sharkdesk wrote: post #33
valuearb wrote: post #28

Earlier quoted context omitted.

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

Actually, someone is paying the team to solve business problems and create revenue.

Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.

Re: A bogus study on code review

#38

Post by gburt »

sharkdesk wrote: post #32
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…

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company. Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get e…

I'm not sure I assumed developers are driven by the same things. A careful reading of my last sentence did not define what it meant for code to be good or by what process you would be proud of the output, I think that is a product for you, your team and leadership to decide. Perhaps my use of the word pride was too strong, the core of my philosophizing there is that software development on a team is a social endeavour and that code review can be a very effective alignment mechanism.

Someone further down the page suggested that the style nitpicks arise from a lack of understanding and familiarity with that piece of the codebase. I think that is often true and a factor to be considered both when allocating code review and when setting expectations regarding code ownership and cross-functional understanding.

I've also provided some suggestions for how to tune down the "95% style nitpicks" elsewhere on this page - it is a problem, but with appropriate tooling and expectation setting, it can be reduced to a small fraction of total code review output. It would be dysfunctional for a team to spend time on that stuff when we've all agreed it isn't high ROI. I agree that code review can be done very poorly, but your observed ratios are not a fixed property of the world of code review.

Let me be clear: I am absolutely not advocating for a process that motivates nerding for the sake of nerding - I push back on a whole wealth of that sort of behavior. I am a business person motivated by producing sustainable teams that produce real value in the form of solutions to problems. I just feel that is a high-dimensional problem and certain kinds of technical debt can come at extreme cost and can, at times, if done correctly, be mitigated with processes like code review.

Re: A bogus study on code review

#39

Post by joshuamorton »

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

In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues.

A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even still most of the style comments I make when reviewing code amount to "be internally consistent" about something, and those are nits.

And that whole "reviews take 5 minutes" doesn't mean that they aren't useful. I often find something that is either confusing, or a typo, or dumb mistake/overlooked thing or a non-idiomatic way of doing something, and leave a comment expressing that. The review still takes 5 minutes, but I've caught a bug or a bit of technical debt later.

As a code-writer, I have more or less the same opinion. I would flatly refuse to work somewhere that doesn't do code review. It makes working in the codebase difficult, and makes personal development more difficult.

Re: A bogus study on code review

#40

Post by u801e »

Clubber wrote: post #31
u801e wrote: post #26

Earlier quoted context omitted.

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

>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. I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value. >The other thing is that its always a good thing to have at least o…

> developer should be reviewing their own code fairly thoroughly anyway

I agree, but sometimes, when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.

For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.

We have since enabled post-push linting on that repo, but I think that had someone else looked over it, they may have spotted that issue before it was merged.

> Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done.

To some extent, you're correct, but only if the code base is relatively static and conventions don't change. That usually doesn't hold true over time.

Post reply on HN