Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Using checklists for code review

blog.rbcommons.com

Post reply on HN
Print view View original

11–20 of 26 posts

Re: Using checklists for code review

#11

Post by obviouslygreen »

logn wrote: post #6

As much as people feel negatively about Java, it really has fantastic static code analysis tools. PMD, FindBugs, CheckStyle are all very helpful when properly configured. They're basically on the checklist level. I think checklist level thinking for code reviews is bad though. It ends up being a pretty soul destroying experience when someone tells you that you need to validate arguments to all methods as not null (fo…

As much as people feel negatively about Java, it really has fantastic static code analysis tools.

My first experience with Java was at a job in high school... and my only reference was a book that weighed half as much as I did. Needless to say it left me with a very, very poor impression of the language, which I'm only recently learning is very much wrong.

I definitely prefer writing Python, but the more I actually learn about Java, the worse I feed for being such a senseless detractor for so many years.

Re: Using checklists for code review

#12

Post by fyi80 »

logn wrote: post #6

As much as people feel negatively about Java, it really has fantastic static code analysis tools. PMD, FindBugs, CheckStyle are all very helpful when properly configured. They're basically on the checklist level. I think checklist level thinking for code reviews is bad though. It ends up being a pretty soul destroying experience when someone tells you that you need to validate arguments to all methods as not null (fo…

The checklist motivates the automation. This advice is for people who have neither, to get started.

Re: Using checklists for code review

#13

Post by fyi80 »

mjs wrote: post #4

We've tried using checklists a bit for code review, and hit the problem that our checklists would be pretty much guaranteed to have at least one item that was arguably inapplicable to the changeset. So you'd have some changeset that was very small, or not contain UI elements, or did contain UI elements, or was related to build tools, and so on. And so we'd get into a situation where we'd have to decide whether a part…

"Not applicable" or "0/0 UI elements are correct" is a fine response to checklist.

Re: Using checklists for code review

#15

Post by azov »

I have yet to see a code review checklist that I find useful. There are just too many things that can go wrong with software, so the list ends up either too long to be practical, or too generic to be helpful.

What I do find useful is to have a good variety of skills on the review team. Different people tend to check for different things. Some are just naturally good at spotting, say, concurrency problems, others have a pet peeve with names that aren't clear, etc. But someone who doesn't have an eye for concurrency problems won't magically start noticing them in a pile of code because there's a box saying "check for race conditions" somewhere on the list.

Re: Using checklists for code review

#16

Post by mtodd »

chipx86 wrote: post #9
nsfmc wrote: post #7

Earlier quoted context omitted.

i disagree. fundamentally the point of these checklists is to standardize the sorts of things you evaluate during the code review. if feedback is inconsistent, you end up seeing different standards applied to several parts of the codebase which can (and does) manifest itself as an inconsistent product. the overall appearance of the checklist does make it seem like it would be problematic, but i think the author makes…

Exactly. Forcing a checklist upon all reviewers isn't what the article is all about, but rather it's about one developer's way of organizing his own personal process for doing code review so he doesn't forget something. If I have a huge change in front of me, and I know that I tend to look for certain things, I could certainly try to look for all those in one go. That's error-prone, though. Might forget something, or…

Lists in general are great for organizing thoughts. Being able to check items off the list helps focus thoughts and offload the overhead of keeping track of progress to something better suited to the task.

Re: Using checklists for code review

#18

Post by dagw »

warmwaffles wrote: post #17

Couldn't a checklist also be the tests written for the feature? This ensures that future revisions don't muck with the already working system.

Some things are hard to write tests for. Basically most things where the criteria is "does it look right"

Re: Using checklists for code review

#19

Post by Joeri »

logn wrote: post #6

As much as people feel negatively about Java, it really has fantastic static code analysis tools. PMD, FindBugs, CheckStyle are all very helpful when properly configured. They're basically on the checklist level. I think checklist level thinking for code reviews is bad though. It ends up being a pretty soul destroying experience when someone tells you that you need to validate arguments to all methods as not null (fo…

To be fair, there are similar tools for most languages.

One of the most comprehensive tools for doing automated quality reviews of code is Sonar ( http://www.sonarsource.org/ ). It supports pretty much all programming languages (thought not all for free).

For example, for PHP code sonar runs a combination of PHP_CodeSniffer, PHPMD, phpDepend and phpunit and integrates the reporting of all those tools.

Re: Using checklists for code review

#20

Post by zimpenfish »

mjs wrote: post #3

There's a nice list of checklists at http://www.projectcheck.org/checklists.html including a "checklist for checklists" (!) http://www.projectcheck.org/checklist-for-checklists.html Great article on the use of checklists in hospitals, by Atul Gawande: http://www.newyorker.com/reporting/2007/12/10/071210fa_fact_...

Gawande's book on Checklists is excellent but I suspect more of an expansion of the NYR[1] article than a standalone work.

(Offtopically, "Complications" is also worth a read.)

[1] how do you abbreviate The New Yorker?

Post reply on HN