4 ms·

When talking about code reviews I am constantly reminded of the following 1. How hard it is to grok code that I've never seen before 2. How hard it is to grok

When talking about code reviews I am constantly reminded of the following 1. How hard it is to grok code that I've never seen before 2. How hard it is to grok my own code from 6 months ago Sometimes it may take me a day or more to fully get my head around a new piece of code I am working with. I could not give a thorough AND fair review of another person's code without internalizing it to a satisfactory degree. There is also the issue of software as art vs engineering. Quite often I will take issue with the style or approach of another person's coding simply because I would have done it differently had I done it myself. But that does not mean that their code is wrong. All things considered, I think code reviews can be helpful in certain situations, but I think there are many pitfalls which must be avoided in order for them to justify the cost.

ProAm 13y ago
I've found code reviews are beneficial to help you catch simple bugs, you don't always have to internalize the purpose of the code just make sure the person dotted their i's, crossed their t's and freed all their memory pointers... Or just help junior guys write more efficient code...
hox 13y ago
The cost you speak of is directly trading software quality for "speedier" delivery. Heck, this was even posted just yesterday: http://www.sec.gov/litigation/admin/2013/34-70694.pdf http://www.sec.gov/litigation/admin/2013/34-70694.pdf. One of the big problems was there was no code review being performed. Also think of code review as a fantastic learning opportunity for the developers you are reviewing. Practicing an art can only get you so far as an expert, but being critiqued by others can help refine the rough edges you may otherwise ignore throughout your career. Code reviews don't have to happen in massive context, either; why not follow a practice that every pull request (or whatever your SCM calls it) goes through at least one reviewer before being merged to master? And ensure your pull requests are frequent and preferably 100 lines or less! A little bit goes a REALLY long way. Don't give in to the fallacy that your code isn't either worth it or is good enough to not be reviewed.
martinvanaken 13y ago
I agree with you, and this is the reason why having small features and reviews for all of them helps. Groking a colleague's code is much simple if it is a small piece, and when you are reviewing his code regularly (and the other way around). Our typical PR is 1-2 days of work, so I'm reviewing several by week, something by days. You are of course right that it should not fall into "you should do it my way". Now, when my colleague said "I would have done it differently", I always ask how and why. I will probably not change my code if it is good (or even good enough), but I would have learned something, or got another point of view.
jt2190 13y ago
> Sometimes it may take me a day or more to fully get > my head around a new piece of code I am working with. Are you trying to perform a design review, or a code review? Code reviews (in my mind, at least) concern themselves with the low-level mechanics of the code ("Handle this exception properly","there's a library method for this logic","follow the team's coding standards", etc.) whereas design reviews deal with the "how is this thing supposed to work?". When reviewing code, I avoid dealing with design issues. If I see design issues, I ask the developer for a design review.
- mberning 13y ago
  My time is more valuable than being a pre-compile/pre-deploy sanity check or human lint. When I review code I review all of it, not just the easy parts. If I was going to do a 1/2 ass job of it I wouldn't do it in the first place.
  - jt2190 13y ago
    > When I review code I review all of it... We haven't mentioned timing, but that's the reason I decouple design and code reviews. Design reviews can be done fairly early in the process before the code is complete. If design changes are needed, there is still plenty of time for them. Code reviews can't really be done until most of the code is written, but (as you point out) the kinds of problems they find are much easier to fix, so require less time.
wting 13y ago
There's also a few benefits of code review that you missed: 1. Reduces bus factor. 2. Increase readability. Coding in a company is a communication problem. How you solve a problem may be different than someone else. Outside of simply catching more mistakes, it's possible that you may learn something new or the other person can improve their code. Ideally there should be a team / company coding style simply to ease communication overhead and reduce bikeshedding.

ProAm 13y ago

I've found code reviews are beneficial to help you catch simple bugs, you don't always have to internalize the purpose of the code just make sure the person dotted their i's, crossed their t's and freed all their memory pointers... Or just help junior guys write more efficient code...

hox 13y ago

The cost you speak of is directly trading software quality for "speedier" delivery. Heck, this was even posted just yesterday: http://www.sec.gov/litigation/admin/2013/34-70694.pdf http://www.sec.gov/litigation/admin/2013/34-70694.pdf. One of the big problems was there was no code review being performed. Also think of code review as a fantastic learning opportunity for the developers you are reviewing. Practicing an art can only get you so far as an expert, but being critiqued by others can help refine the rough edges you may otherwise ignore throughout your career. Code reviews don't have to happen in massive context, either; why not follow a practice that every pull request (or whatever your SCM calls it) goes through at least one reviewer before being merged to master? And ensure your pull requests are frequent and preferably 100 lines or less! A little bit goes a REALLY long way. Don't give in to the fallacy that your code isn't either worth it or is good enough to not be reviewed.

martinvanaken 13y ago

I agree with you, and this is the reason why having small features and reviews for all of them helps. Groking a colleague's code is much simple if it is a small piece, and when you are reviewing his code regularly (and the other way around). Our typical PR is 1-2 days of work, so I'm reviewing several by week, something by days. You are of course right that it should not fall into "you should do it my way". Now, when my colleague said "I would have done it differently", I always ask how and why. I will probably not change my code if it is good (or even good enough), but I would have learned something, or got another point of view.

jt2190 13y ago

> Sometimes it may take me a day or more to fully get > my head around a new piece of code I am working with. Are you trying to perform a design review, or a code review? Code reviews (in my mind, at least) concern themselves with the low-level mechanics of the code ("Handle this exception properly","there's a library method for this logic","follow the team's coding standards", etc.) whereas design reviews deal with the "how is this thing supposed to work?". When reviewing code, I avoid dealing with design issues. If I see design issues, I ask the developer for a design review.

mberning 13y ago
My time is more valuable than being a pre-compile/pre-deploy sanity check or human lint. When I review code I review all of it, not just the easy parts. If I was going to do a 1/2 ass job of it I wouldn't do it in the first place.
- jt2190 13y ago
  > When I review code I review all of it... We haven't mentioned timing, but that's the reason I decouple design and code reviews. Design reviews can be done fairly early in the process before the code is complete. If design changes are needed, there is still plenty of time for them. Code reviews can't really be done until most of the code is written, but (as you point out) the kinds of problems they find are much easier to fix, so require less time.

wting 13y ago

There's also a few benefits of code review that you missed: 1. Reduces bus factor. 2. Increase readability. Coding in a company is a communication problem. How you solve a problem may be different than someone else. Outside of simply catching more mistakes, it's possible that you may learn something new or the other person can improve their code. Ideally there should be a team / company coding style simply to ease communication overhead and reduce bikeshedding.