Code Review Guide

FreeEMS topics that aren't specific to hardware development or firmware development.
Post Reply
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Code Review Guide

Post by Fred »

This thread will serve as a guide to those reviewing code, be it mine, or that of anyone else. It'll start off simple and grow with time and experiences shared. I'll put some basics in here now as a start. Feel free to post with your ideas and code review requirements and I'll add them as I deem appropriate.

Pre-Review

Before starting your review, open a text editor or pull up a piece of paper and prepare to make notes as you go such that you don't forget anything that you come across. If you're unsure it's valid to ask questions and get the developer to defend their course of action. Thoughts unsaid are potential future bugs, let them know.

Commit Comment

This should be the start of your review procedure. The comment should be as concise as possible, but no more. It should very briefly say what was done, and in more detail why it was done. The commit comment should match the commit contents. Keep what was said here in mind while reviewing the rest of the work. Don't let inappropriate, insufficient, poorly written, misleading, partial, etc comments slide, pull the dev up on it.

Formatting/White Space

Should almost NEVER be included with code change. If formatting is needed, perform it before making any code changes and commit it separately with an appropriate comment. If you find a commit marked as formatting/white space with code change in it, or vice versa, do not let it slide. This is poor practice.

Coherency

The commit shouldn't do too much, nor should it do too little. Sometimes it's OK to do a few different tweaks in the same commit, provided that they're clearly labeled in the comment. Mostly, though, it's not. The commit should do one thing, and should do it completely. If the block of work is really big, then it might make sense to break it down into smaller commits which perform aspects of the overall change. If this is done, each should ideally produce working software post completion and not break the build or functionality. If it is a must to commit broken partial work, then those commits should be held back locally to your box and pushed as a set once completed.

Style

The contents of the commit should maintain the prevalent style of the code base, unless their purpose is to change that. Variable naming, location of parenthesis, tabs vs spaces, etc should all be clean, consistent and correct for the project in question. Coding standards vary per language and project, but check to make sure that the diff looks clean and the result looks even cleaner.

Comments

There should be sufficient, but not excessive, in source comments. Comments should ideally only be included when the purpose of the code is non-obvious. Obvious code that is documented with near-identical comments will often evolve and leave the comments behind. Speaking of which, comments should be updated with the code, such that they continue to match. Excessive comments just obscure the code when trying to read and understand it. Insufficient leave you wondering what is happening. As with commit comments, code comments should mostly say why. It's OK to say what, if it's not clear, though that should mostly be done with the code itself.

Testing

Hopefully this was mentioned in the commit comment as being done. Any required testing, to whatever extent is possible by the reviewer, should be verified. Specifically, any functionality included in the commit comment should work as advertised/desired. Any related functionality around the fringe of the work done should STILL work afterwards - no regressions.

Tracker

The commit that the work was completed in should be on the tracker in the issue relating to the work. And vice versa, the commit comment should include "closes #1234" if it is appropriate. It also pays to prepare yourself for the review by reading over the material in the tracker if there is an associated issue.

Functionality

Though you may not be an expert in the field of your review, you should try to walk through the logic of what has been done and confirm that it appears to make sense and is consistent.

I look forward to your feedback on this hastily assembled thread!

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Code Review Guide

Post by Fred »

I'll add that I'd like to see more code review going on. I typically review Ben's code in OLV anyway. I've recently been asking others to review my firmware code too. It would be good to see the loader, mtx, and the tools get reviewed on a commit by commit basis, too. Let's make it happen, people! :-) You don't have to be an expert to do this to a superficial level, which alone, is very helpful.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Code Reviewers and Review Requests

Post by Fred »

Code Reviewers and Review Requests

New thread here with people and required reviews: viewtopic.php?f=41&t=2142
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
Post Reply