4 ms·
From my experience doing reviews and having my code reviewed the most important thing is to understand: - What the intention of change is (what is this tryin
by ddlatham 12y ago
From my experience doing reviews and having my code reviewed the most important thing is to understand:
- What the intention of change is (what is this trying to achieve)
- What is the code actually doing
There are three levels of review I typically observe:
1. Skim, find one or two comments to leave. LGTM!
2. Review each line or method in isolation. Recognize style and
formatting errors. Identify a couple local bugs.
3. Understand the purpose and the code.
#3 takes time. It's what it takes however to find the issues where the implementation doesn't actually accomplish what was set out to do (at least in all cases). To recognize where the code is going to break when integrated with other modules. To suggest big simplifications.
#3 is also where you get one of the biggest review benefits - shared understanding of the code base.
In my experience developing software, it's easy to have a sense of ownership and care most about the code one personally writes. That is often reinforced by the development process - you are responsible for delivering an issue. However, we're much less likely to feel responsible for the code we are reviewing. We don't answer for that deadline. As a result, it's often seen as a favor to review someone's code (even when the policy is for all code to be reviewed). It's also often not time allotted or planned for. So the tendency is to rush them.
Do other people observe the same thing? Have people used a process which gives more incentive to invest time in code reviewing well?
- colinbartlett 12y agoMy experience is similar. On some times, I have become very frustrated that I always contribute a #3 but all I ever receive is a #1 of my code.
- hckr1292 12y agoDitto! I see code review as an opportunity to learn from other members of the team and also to make sure I don't have to fix something tricksy in the middle of the night. I think that others are more (rationally) motivated by the desire to iterate quickly and deliver a large volume of features and fixes in a short window of time.
- pimlottc 12y agoI love and fully support the idea of code reviews, but I personally find it nearly impossible to review at anything less than the third level. As a result, code reviews take a lot of time and energy for me, and so I hypocritically tend to end up doing very few. It's just very hard for me to give my endorsement to a piece of code if I haven't had a chance to go over it completely. Allocating more time for review would help, but I agree it's largely a matter of incentives. When push comes to shove, delivering the issues assigned to me is always going to take priority over reviewing someone else's code. I think that's only natural; issues resolved is generally the most visible metric for developer performance. I'm not sure how to change that in a meaningful way.
- dustingetz 12y agoArchitectural or design problems need to be fixed before merge. Code review incentives only align if the patch isn't merged until it passes review (e.g. Linux). If patches are merged before review, a code review is really just "hey FYI I did this, any glaring problems?". You may need some other methodology to fix architectural or design problems before the review. Pair programming does this for some teams, for other teams its only hiring developers they trust.
- _asummers 12y agoI usually do the review in 3 passes doing all 3 exactly in that order, with the exception of "style" being "internally consistent to the rest of the code" (though those aren't too far from each other!). 3 absolutely takes the most time, but usually results with the absolute best benefits and is necessary for major functional changes, particularly across networks or anything related to security.
- nazbot 12y agoYup - it's also similar to how great developers don't debug or code by braille (did that work? nope. How about this? Nope. Ok lemme try this...) but rather will sit and think through a problem. #3 is where a lot of tension can arise during code reviews. Some people, usually those who aren't familiar with the process, will wonder why you are taking so long to review the code or why you are asking questions that are 'out of scope'. I find that larger organizations are often more competent at code reviews. In the startup world #3 is a rare, rare thing.