Part 2: When to do code reviews and how to not do them.
————————-————————-————————-————————-
How do you get away without doing code reviews?
Write quality code and have confidence in it because you understand it.
Writing quality code, without code reviews, involves three key aspects.
First, short methods. Don’t write long methods; they should be brief. If you find yourself doing this (or reading someone else’s code that did it), refactor portions of it and give the method a good name. You want the code to be easily readable and reusable. Long methods require you to remember more things as you read it, and the potential to introduce problems increases as it gets longer and longer. Toss in an early return, and you might introduce memory leaks (also, NEVER do returns in the middle of a method). Short methods present clarity, since a well named method will present its intent and reading a short bit of code will tell you what it actually does. Very few methods should need a preceding comment simply because the method name describes what it does; if it doesn’t, it should be short enough to read and understand it. Debugging a shorter method is usually easier too; you can quickly isolate and determine which method is causing a problem and fix it, or eliminate other bits of code that may not be necessary. Also, you can obviously re-use code more easily if it is adequately refactored. All too often I see code copied, pasted, and slightly tweaked from some other area of the same file. Take the time to communize it.
Here is a concrete example of a hypothetical bad method. I simply expanded out the first case statement from some of my LED Cyr Wheel code:
void CWPatternSequenceManager::loadFileInfo(CDPatternFileInfo *fileInfo) {
freePatternItems();
switch (fileInfo->patternFileType) {
case CDPatternFileTypeSequenceFile: {
char fullFilenamePath[MAX_PATH];
_getFullpathName(_getRootDirectory(), fileInfo, fullFilenamePath, MAX_PATH);
DEBUG_PRINTF(” loadAsSequenceFileInfo: %d at %s\r\n”, fileInfo->dirIndex, fullFilenamePath);
FatFile sequenceFile = FatFile(fullFilenamePath, O_READ);
loadAsSequenceFromFatFile(&sequenceFile);
sequenceFile.close();
break;
}
case CDPatternFileTypeBitmapImage: {
loadAsBitmapFileInfo(fileInfo);
break;
}
case CDPatternFileTypeDefaultSequence: {
loadDefaultSequence();
break;
}
default: {
makeSequenceFlashColor(CRGB::Red); // error of some sort
sequenceChanged();
break;
}
}
}
And here is what it should look like:
void CWPatternSequenceManager::loadFileInfo(CDPatternFileInfo *fileInfo) {
freePatternItems();
switch (fileInfo->patternFileType) {
case CDPatternFileTypeSequenceFile: {
loadAsSequenceFileInfo(fileInfo);
break;
}
case CDPatternFileTypeBitmapImage: {
loadAsBitmapFileInfo(fileInfo);
break;
}
case CDPatternFileTypeDefaultSequence: {
loadDefaultSequence();
break;
}
default: {
makeSequenceFlashColor(CRGB::Red); // error of some sort
sequenceChanged();
break;
}
}
}
There isn’t much of a difference, but the second one is so much easier to read, understand, and debug.
Second, review your own changes before committing. I don’t consider this a code review, since you are just reading what you are about to commit. Your general efficient work flow should be like this: code something incremental; fix a bug, or do small work on a feature. If it is a new feature, don’t feel like you have to get everything done before you commit the code, and try to keep all the code compiling at all times. Once you are ready to commit your code, don’t just blindly do a “git stage” and “git push”. In fact, if you are using the command line then that is probably one of your failure points. You should be using a high-level GUI, like Tower.app on the Mac. This allows you to easily see all your files that you modified and the diffs. Review the files one at a time, and “check them off” as you review them. In Tower.app, a check off of a file simply moves it to the staging area. After you have staged all your files, you can then push your change, and be confident that it is correct, since you reviewed it one last time and thought about it. More often than not, I will accidentally check something in because I simply didn’t follow this rule. I will blindly think “all these files need to go in the commit”, and I’ll batch stage them and miss something obvious (like a silly log message). Conversely, I have frequently caught my own mistakes by reading my own code before committing it. Simply reviewing your own code, right before you commit it, will make you a higher quality programmer.
Third, commit often. Make lots of small changes; don’t do big huge single commits — work towards large changes in small chunks. This allows you to undo things as necessary and maintain stability as you add some new feature. If you are working on a big new feature, then work on it on your main code branch but disable it in some way — ensure the code is still compiled, but not executed. It is quite possible that you might introduce some regression in your old codepath, and that problem will be caught sooner if your code is committed and in use by other developers.
————————-————————-————————-————————-
I’ve been a professional software developer for over 20 years. I started writing shareware on Windows 95 back in high school. While in college I got a job at Borland Software Corporation, first working in Developer Support helping programmers program, then moving up to QA and finally working as a Research and Development Engineer. I primarily worked on Delphi running on various flavors of Windows. After that, I moved on to Apple, learning objective-c while starting to work on the core macOS UI frameworks in Cocoa. I’ve been at Apple for over 12 years, primarily working on AppKit but taking some side time to develop the original iPhone’s UIKit and help with lots of other projects along the way.
