Hacker News new | ask | show | jobs
by nsfmc 4809 days ago
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 a good point of showing that they act as a template for the reviewer rather than as strict set of guidelines.

in a situation where "we're all consenting adults here," having a set of things to evaluate features or code style makes it easy for anyone to know what to expect when checking in a change or when reviewing one for the first time. it may not work for you, but it certainly adds consistency when they're used non-dogmatically.

1 comments

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 get bogged down in syntactical stuff and miss something important. However, with a little personal checklist in front of me, I can go "Okay, I'm done looking at syntax. Now let's make sure that there's no off-by-ones." Leads to better organization of thoughts.

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.