Code Review Checklist
blog.fogcreek.com
blog.fogcreek.com
From reading this, I see how bad most software process check-lists are. They tend to be laundry lists mixing obvious minutia and vague uncheckable goals. They are not practical executable tools but well meaning documents that are ignored at the error prone moments when a check-list has the most potential impact.
A check-list for a good check-list:
1. Plan for a specific trigger giving a single pause point of up to 30-90s at which point the check-list is opened and executed
2. Decide whether it is a DO-CONFIRM or READ-DO list
3. Design to engage the brain rather than switch it off
4. 5-9 items at most
5. Don't include any action that is rarely forgotten i.e. the criteria for inclusion is the probability and cost of error. This is very different from listing the most important steps or attempting to fully document a process.
6. Regularly test and measure the check-list for effectiveness
I would say Fog-creek's article is in the "read once" training material category and not a practical check-list. For a real check-list, I would interview the team and examine recent commits and look for the 5 most serious real issues you are experiencing that you need to eliminate from future code. That is a now a review check-list you can mandate, measure and iterate.
For example, instead of having a check for whether braces are in the correct spot, have a check for whether 'astyle' or a similar code formatter has been run.
The point of a checklist is to complement expertise. Anything that can be automated should of course be automated and shouldn't be on the checklist. What goes on the checklist are things that humans have to be doing, that are bad when overlooked, and which sometimes get overlooked. It strikes me as likely that, in most situations, there are some such things for code reviews - although what they are may differ from context to context. Trying to find patterns in them would be a fantastic project.
For example, authors regularly forget to remove commented out code but a reviewer gets slapped in the face by it so "no commented out code" would be in a policy document but no need to add it to a check-list you expect a reviewer to consult when reviewing code.
If designing for the professional already trained in your policies, it means you can concentrate on the shorter-list of high impact things reviewers are found to overlook which are probably project specific subtleties like how argument sanitation or error handling needs to be done.
I think both have their place, but people usually think of the former when discussing introducing checklists into processes, and that's usually inappropriate unless you have people performing tasks outside of their domain.
- What the intention of change is (what is this trying to achieve)
- What is the code actually doing
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 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?
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.
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.
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.
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.
#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.
But... on one hand it talks about programmers making 15-20 common mistakes. Fine create a checklist for these issues. And then at number 1 it asks "does the code work?".
Of course it's common that the code doesn't work, but it may not be a binary decision - requirements ambiguity, corner cases, dependent on data, etc.
I have also have issues about the Documentation section - none of that will necessarily help "Stop more bugs"...
I don't see the purpose of the article is clear and I don't think it's particularly consistent between the title, checklist and explanatory paragraphs.
https://wiki.apache.org/hadoop/CodeReviewChecklist
https://gist.github.com/bwest87/10049924
http://www.mindfiresolutions.com/Code-Review-Checklist-238.p...
I am very procedure oriented and have always liked checklists and templates. I have just created a github repo to share checklists and templates here:
https://github.com/godber/software_dev_templates_and_checkli...
Feel free to contribute if you think its a good idea.
I've started it out with the Fog Creek checklist ... if Fog Creek guys don't like my having included it, I'll happily take it down.
500 lines of code = "looks fine."
Code reviews.
On some of the items a "yes" is good, and on some a "no" is good. A little more thought could have made the list much easier to use.
All in all, it's subpar compared to github pull requests or Reviewboard.
I'd be so much happier if we could get rid of the entire notion and do up front design and pair programming.
Also, oh my god that checklist. Anyone that has a business dedicated to delivery is going to have to throw that thing out the window.
Personally I enjoy reviewing and getting reviewed. In both cases I have learned a lot.
I think it also depends on the type of code and the consequences of failure. I have seen good success reviewing data migration and modification code for medical records because that stuff was considered important data and there was a culture there such that if something went wrong, they would ask, "why wasn't this caught in review?", so the process was taken seriously.
I agree with the pair programming sentiment though - I think it could be looked at as a form of code review with a fresher context since you see mistakes as they are made rather than puzzling out the thought process of the other coder when it's cold.
Long ago I learned a couple simple rules that get my defect rate very close to zero: asserts everywhere, and step through every line of code. Seriously. Write two lines of code, fire up the debugger, and step through them, inspecting each variable. Any tiny 'unit' that you write should go through the debugger, where unit might be that 5 line function you are writing, or a bug fix, or whatever. It just catches a vast amount of the bugs if you are writing in a modular, perhaps functional style. Tons of side effects, well, it's less good at that. asserts do 'okay' at finding those things, but of course the time to find those bugs is longer.
This buys you a few things. First, you are 'reviewing' your code while you write, which is when it is freshest in your mind. It is hard to mentally model the behavior of the computer, hence code reviews are mentally difficult. The debugger ALWAYS shows you what the computer will do (bar things like nasty thread interactions). The debugger does not lie. You get to see nasty things like Nan or underflow that might just pass silently depending on your error handling, which is again hard to find in a code review. And, of course, the sooner you find bugs the cheaper it is to fix - so the best time is about 10 seconds after you typed that line of code. After that the costs grow either proportionally or exponentially.
I know very few people with that uses this discipline, but it changed my life when I took it up. Any time I slip and fail to do it, I rue it.
Pair programming get you similar benefits. I find it harder to think/talk/respond to another person when I am deep in thought (different brain pathways, or so it feels), so I'm more a solo programmer by nature. However, I would still step through the code when pair programming; I guarantee that your mental model of your code is likely to be flawed. The second person will reduce, but not eliminate that threat.
I understand not all code bases/IDEs/whatever allow this (long startups, 5+ minute compiles, etc), but it it is available, avail yourself of it. If you program in a dynamic language and find yourself using a REPL all the time to test your code you are doing a form of this, but it is still better to test the actual code in place.
However I am not as fond of pair programming for ensuring quality of a change. I find that often it is easy when pair programming to go down the route that looks good at the time and it is hard to step back and evaluate the whole change. Where as being able to look at the end result without any intermediate steps often makes it easier to find flaws.
I think that with code reviews, I find them easier to do if I view the coder as an adversary. They have produced flaws and I need to find them.
I think code review might be the next big thing programmer culture adopts as abolutely necessary. Like we are currently adopting continuous integration and have been adopting version control. This means version control is even more important but we have consensus so there is little to discuss. (just some central vs distributed tradeoffs)
As always, there are some really great and insightful comments and suggestions here, but I'm a bit surprised about the overwhelming negativity around code reviews and this post.
FogCreek's post is just a starting point, and they said so right in the post. It wasn't intended to be taken as the end-all-be-all of code review checklists, but that seems to be getting ignored, sometimes in favor of unconstructive criticism.
It's a decent starting point, and the main takeaway is that there are lots of angles to think about when reviewing commits, every single time, not just whether the code works. It's easy to forget all the angles because code reviews are hard to always do well, so having a checklist, even with redundant items or minutiae really isn't the awful idea it's being made out to be.
The included video, which based on comments I'd be forced to guess almost nobody watched, also clarifies their process and intent, and addresses some of the comments here, for example pair programming. Joel mentions pair programming being the most extreme form of code reviews. In my own experience, he's spot on when he says pair programming is really useful in certain situations, and very hard to use frequently.
One angle that hasn't been discussed that that a long checklist is actually a good thing when it comes to justifying time spent code reviewing to managers (as well as fellow coders). In my own experience, I've witnessed the attitude that code review time is unproductive, and the thinking that code reviews shouldn't take long. Having a specific process that demands a minimum amount of attention is a good thing on its own, completely outside of the benefits to the code and code quality.
Edit: spelling
Personally, I found the very idea of a checklist itself to be a helpful idea that I'd like to use at work. The specifics are going to depend a lot on the language, project, and team.
RE: long checklists -- I think that most of us commenting on this thread are very passionate about code reviews, whereas our teammates are less so. I think a short code review checklist is more likely to engage coworkers in code review period.
We have had good success using pronto to automatically run Rubocop, Rails Best Practices, and Brakeman running on CircleCI against every code commit on Github. Then, we still have another team member review a branch before approving a merge, but we know that all tests pass, coverage has not dropped, and there are no obvious errors.
Creating such an checklist is great exercise though - writing my own made me think heavily on what is important. Then I realized writing it down is not enough - how do I know it is not bunch of made up rules? So I have also tried to provide rationale for each topic. At the same I have tried to keep the list actionable.
You can judge if I di well here: http://www.michalkostic.com/post/104272540521/code-review-ch...
Nowadays you also have software (e.g. Hound) that does some basic checks for you.
It really feels like a glorified TODO list (or TODO list manager), but I do think that there is potential here. I would recommend adding features that separate it from the usual TODO list, e.g.:
- Add features of flowcharts
-- conditionals (if this then go there)
-- subtasks
- Allow for finer control over the contents of the headers and items
-- Currently, it looks like you can add descriptions which display directly beneath the item or header (and is very clean looking!), but it would be nice if I were able to add html content (consider allowing markdown?)
- I may be using it incorrectly, but I started creating a list and accidentally started the list but kept adding items anyway. Consider adding the ability to add the new tasks from an already-started list to the parent list from which it was created.
- Other things? What are your currently-planned unimplemented features for this application?
I could see myself (and my team) using this application if the bullets above were addressed.
Here is my comments on your suggestions,
-- Flowcharts I think I need a bit more data on how users use the app and in what direction I should take the app and features before implementing this, but it's very possible I will in the future.
-- Conditionals Yes, I'm thinking of adding something like this, but on a higher priority I will add Triggers, so a checklist can be started automatically based on for example, a time interval or a callback over HTTP. I will also add Outcomes, so you can set up a action that should happen when a checklist is completed, like trigger another checklist.
-- Finder control over headers and items I will work on this, with the goal of making as simple as possible to get started with the first checklist, but over time be able to use more advanced features for creating items.
-- Extended item details Markdown is in the backlog. Alos item audit trail, like, Frej changed the description from ... to ... (5 min ago), and so on, so you have a complete track record of changes.
-- Merge items from "started-checklists" to parent checklist. This is something I'm going to implement pretty soon, so that, when a person perform a checklist and discovers a the need of a new item, or an obsolete existing item, they should not need to make the change first on the active checklist and the same changes on the parent checklist.
-- On the roadmap for the near future Triggers (se abow), Outcomes, Audit trails for checklists & items, "News feed" and Notifications center is things I have on the roadmap.
Thanks again for your thoughts!
/Frej
> Does the code work? Does it perform its intended function, the logic is correct etc.
I believe this concern lies with testing (automated or manual). I don't believe a code reviewer should even have to know about the requirements of the feature/bug, and should focus more on the design/quality vs. whether it works.
Mostly because it's almost impossible to know this by just reviewing the code, without having to run it -- and I certainly don't think a code reviewer should have to execute the code.
Also, perhaps you need more practice reading code. So you are not so reliant on a "TEST PASS" output from a tool to know if a particular section of code is likely working.
I'm just saying it's not the concern of a code review.
Second, generally[1] code is read and edited many more times than it is written. Poorly designed, but working code today is setup to be broken when the first edit comes around.
[1] Some code is clearly 1 off and throwaway. This is not the code I'm talking about.
If you believe that, then either
- The code is badly designed/obfuscated enough that its behaviour is not easy to see (something that should definitely raise a concern at a code review)
- The reviewer doesn't know enough to figure out what the code should do (which in and of itself is a huge WTF.)
In some sense, testing (especially the black-box type) is "dumb". It cannot discover failures at edge cases which could be seen from reading the code and thinking about its behaviour, something that should definitely happen at a code review. It is useful as a sanity check, but cannot replace reasoned inspection.
No.