PHP Code Review Checklist

I recently created a code review checklist for a PHP based project I was working on. If you want to be pedantic, it’s actually a list of questions that the reviewers should ask themselves as they look through the code.

This isn’t an exhaustive list and it’s not meant to be. The only thing you will get from making an all-encompasing code review checklist is a list the your reviewers will ignore. I’m depending on my team to be smart, autonomous, rational, and wise. Perhaps I am idealistic, or maybe I’m just lucky to work with a great group of engineers. The final version is still under development, but I wanted to share what I think is important when doing a “formal” code review.

Questions to consider while reviewing code:

1. Is the code readable as written? Does it require additional comments, better naming, or general refactoring to be easily understandable? Does the code generally conform to the style of coding for the project?
2. Does each function contain a (very) brief comment describing functionality, inputs, and outputs?
3. Does new code keep business logic away from presentation code?
4. Is identical functionality repeated multiple places in the code base?
5. Are there any code blocks that should be abstracted into functions or classes for maintainability?
6. Are there any hard coded constants that could possible change in the future?
7. Are all reasonable error conditions handled?

These questions are shaped by my personal experiences and the kinds of projects that I work on. Some of them won’t be applicable to another project, or even if they are applicable, they might not be necessary. They are also very subjective. Decisions made while writing software are rarely black and white. What is readable code for one person might be opaque for another coder who has a different skill set. “Reasonable” error conditions may differ based on how critical this application is. Consequently, I think code reviews are best when they are discussions and opportunity for learning.

There was another group of questions that I thought were important, but aren’t really code related. They probalby won’t become part of our checklist, but I think they are interesting none the less.

1. Are the requirements formalized in writing and tracked/managed properly?
2. Are the requirements clear and unambiguous?
3. Is there a testable scenario that can exercise the new/changed requirements?
4. Was the code tested by someone other than the author?
5. Did the modified code satisfy all the current system requirements?
6. Did the test scenarios exercise all the changed code paths? If not, is there a good reason for that?

For me these questions are more like project wide diagnostic tests. If you are running over schedule or you are getting a lot of bug reports back from the downstream users, asking these questions may help explain why the code creation process isn’t performing as well as expected.