Corbin's Treehouse - Corbin Dunn, Santa Cruz, CA
Plug Bug
Treehouse
Photography
Videos
Projects
Unicycling
About

Code Reviews, Part 2: When to do them, and how to avoid them


Part 1 discussed why you should not be done code reviews.

Part 2: When to do code reviews and how to not do them.

If you are creating mission critical code, then by all means: have every change reviewed by someone else. In other words, if you are doing the drivetrain logic for my Tesla, then have someone else review every single change twice. If you are writing core kernel code for an OS where making a mistake would stop hundreds of other people from getting work done, then yeah, you need to get all your code reviewed. More often than not, the average developer is not in this situation.
 
So, if you are not writing mission critical code, when should you have your code reviewed?
 
Do it when you aren’t confident about your source code change, or simply want a second opinion as to the best way to do something. But, this really isn’t a code review; this is more of code assistance to help you get up to speed on a particular code base or make a decision on how to code something up. If you have questions about some type of change, you should research and figure out what the code does so you fully understand it. You aren’t a good coder if you don’t fully understand your change. You should ask someone else to explain it, and then add a good comment helping you (and future code readers) understand what some code is doing.
 
Do it when you are creating a new API. It is impossible to create a good API by yourself, and it is important to discuss it. 
 
Do it when you are working on code that you don’t know well. Originally I was going to write “when working on code that you usually don’t work on”, but I don’t think that is the right thing to do. You might only occasionally work on some code or be incredibly new to it, but it is quite possible that when you do work on it you are 100% sure that a particular change is what needs to be done. If you are confident that your change is right, then it probably is, and there is no reason to get your code reviewed. 
 
Do it when you are about to ship your product. Don’t risk introducing a problem a day before you ship your product. The closer you are to shipping, the more closely you should have your code scrutinized by more people. This is essential, and the time period on when to start doing code reviews really depends on the scope of the project you are working on.

————————-————————-————————-————————-

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. 




(c) 2008-2017 Corbin Dunn

Corbin's Treehouse is powered by WordPress. Made on a Mac.

Subscribe to RSS feeds for entries and comments.

35 queries. 0.424 seconds.