Code Review Checklist

February 16, 2006

Following is a check list I refer to often which catches many issues often:

1. No errors should occur when building the source code. No warnings should be introduced by changes made to the code. Also, any warnings during the build should be within acceptable boundaries with good reasoning.

2. Each source file should start with an appropriate header and copyright information. All source files should have a comment block describing the functionality provided by the file.

3. Describe each routine, method, and class in one or two sentences at the top of its definition. If you can’t describe it in a short sentence or two, you may need to reassess its purpose. It might be a sign that the design needs to be improved and routines may need to be split into smaller more reusable units. Make it clear which parameters are used for input and output.

4. Comments are required for aspects of variables that the name doesn’t describe. Each global variable should indicate its purpose and why it needs to be global.

5. Comment the units of numeric data. For example, if a number represents length, indicate if it is in feet or meters.

6. Complex areas, algorithms, and code optimizations should be sufficiently commented, so other developers can understand the code and walk through it.

7. Dead Code: There should be an explanation for any code that is commented out. “Dead Code” should be removed. If it is a temporary hack, it should be identified as such.

8. Pending/TODO: A comment is required for all code not completely implemented. The comment should describe what’s left to do or is missing. You should also use a distinctive marker that you can search for later (For example: “TODO:”).

9. Are assertions used everywhere data is expected to have a valid value or range? Assertions make it easier to identify potential problems. For example, test if pointers or references are valid.

10. An error should be detected and handled if it affects the execution of the rest of a routine. For example, if a resource allocation fails, this affects the rest of the routine if it uses that resource. This should be detected and proper action taken. In some cases, the “proper action” may simply be to log the error and send an appropriate message to the user.

11. Make sure all resources and memory allocated are released in the error paths. Use try-catch-finally in C++/C# code. Is allocated memory (non-garbage collected) freed? All allocated memory needs to be freed when no longer needed. Make sure memory is released in all code paths, especially in error code paths. Unmanaged objects such as File/Sockets/Graphics/Database objects in C#/Java need to be destructed at the earliest time. File, Sockets, Database connections, etc. (basically all objects where a creation and a deletion method exist) should be freed even when an error occurs. For example, whenever you use “new” in C++, there should be a delete somewhere that disposes of the object. Resources that are opened must be closed. For example, when opening a file in most development environments, you need to call a method to close the file when you’re done.

12. If the source code uses a routine that throws an exception, there should be a function in the call stack that catches it and handles it properly. There should not be any abnormal terminations for expected flows and also the user should be informed of any un-recoverable situations.

13. Does the code respect the project coding conventions? Check that the coding conventions have been followed. Variable naming, indentation, and bracket style should be used. Use FXCop and follow C++/C# coding conventions/guidelines within acceptable limits.

14. Consider notifying your caller when an error is detected. If the error might affect your caller, the caller should be notified. For example, the “Open” methods of a file class should return error conditions. Even if the class stays in a valid state and other calls to the class will be handled properly, the caller might be interested in doing some error handling of its own.

15. Don’t forget that error handling code that can be defective. It is important to write test cases for error handling cases that exercise it.

16. Make sure there’s no code path where the same object is released more than once? Check error code paths.

17. COM Reference Counting: Frequently a reference counter is used to keep the reference count on objects (For example, COM objects). The object uses the reference counter to determine when to destroy itself. In most cases, the developer uses methods to increment or decrement the reference count. Make sure the reference count reflects the number of times an object is referred. Similarly tracing is important in code to validate the flow.

18. Thread Safety: Are all global variables thread-safe? If global variables can be accessed by more than one thread, code altering the global variable should be enclosed using a synchronization mechanism such as a mutex. Code accessing the variable should be enclosed with the same mechanism.

19. If some objects can be accessed by more than one thread, make sure member variables are protected by synchronization mechanisms.

20. It is important to release the locks in the same order they were acquired to avoid deadlock situations. Check error code paths.

21. Database Transactions: Always Commit/Rollback a transaction at the earliest possible time. Keep transactions short.

22. Make sure there’s no possibility for acquiring a set of locks (mutex, semaphores, etc.) in different orders. For example, if Thread A acquires Lock #1 and then Lock #2, then Thread B shouldn’t acquire Lock #2 and then Lock #1.

23. Are loop ending conditions accurate? Check all loops to make sure they iterate the right number of times. Check the condition that ends the loop; insure it will end out doing the expected number of iterations.

24. Check for code paths that can cause infinite loops? Make sure end loop conditions will be met unless otherwise documented.

25. Do recursive functions run within a reasonable amount of stack space? Recursive functions should run with a reasonable amount of stack space. Generally, it is better to code iterative functions with proper/predictable end conditions.

26. Are whole objects duplicated when only references are needed? This happens when objects are passed by value when only references are required. This also applies to algorithms that copy a lot of memory. Consider using algorithm that minimizes the number of object duplications, reducing the data that needs to be transferred in memory. Avoid entire copying objects onto the stack instead use reference objects (most of the time default in C# for large user defined objects)

27. Does the code have an impact on size, speed, or memory use? Can it be optimized? For instance, if you use data structures with a large number of occurrences, you might want to reduce the size of the structure.

28. Blocking calls: Consider using a different thread for code making a function call that blocks or use a monitor thread with a well-defined timeout

29. Is the code doing busy waits instead of using synchronization mechanisms or timer events? Doing busy waits takes up CPU time. It is a better practice to use synchronization mechanisms since they force the thread to sleep without using valuable cpu time.

30. Optimizations may often make code harder to read and more likely to contain bugs. Such optimizations should be avoided unless a need has been identified. Has the code been profiled? Check if any over optimization has led to functionality disappearing.

31. Are function parameters explicitly verified in the code? This check is encouraged for functions where you don’t control the whole range of values that are sent to the function. This isn’t the case for helper functions, for instance. Each function should check its parameter for minimum and maximum possible values. Each pointer or reference should be checked to see if it is null. An error or an exception should occur if a parameter is invalid.

32. Make sure an error message is displayed if an index is out-of-bound. This can happen in C# too for dynamically created lists, etc.

33. Make sure the user sees simple error messages, not technical jargon.

34. Don’t return references to objects declared on the stack, return references to objects created on the heap. In C# whenever new is called a new heap object is created, but if an object is passed by copy then the stack object would disappear on return resulting in invalid data.

35. Make sure there are no code paths where variables are used prior to being initialized? If an object is used by more than one thread, make sure the object is not in use by another thread when you destroy it. If an object is created by doing a function call, make sure the object was created before using it. The VS.NET C# compiler ensures this so don’t ignore this warning.

36. Does the code re-write functionality that could be achieved by using an existing API/code? Don’t reinvent the wheel. New code should use existing functionality as much as possible. Don’t rewrite source code that already exists in the project. Code that is replicated in more than one function should be put in a helper function for easier maintenance. The existing code/library routines may be already optimized for this operation.

37. Bug Fix Side Effects: Does a fix made to a function change the behavior of caller functions? Sometimes code expects a function to behave incorrectly. Fixing the function can, in some cases, break the caller. If this happens, either fix the code that depends on the function, or add a comment explaining why the code can’t be changed.

38. Does the bug fix correct all the occurrences of the bug? If the code you’re reviewing is fixing a bug, make sure it fixes all the occurrences of the bug.

39. Is the code doing signed/unsigned conversions? Check all signed to unsigned conversions: Can sign completion cause problems? Check all unsigned to signed conversions: Can overflow occur? Test with Minimum and Maximum possible values. Sometimes a downcast from ‘long’ to ‘int’ could mean loss of data, use the larger data type preferably.

40. Ensure the developer has unit tested the code before sending it for review. All the limit and main functionality cases should have been tested.

41. As a reviewer, you should understand the code. If you don’t, the review may not be complete, or the code may not be well commented.

42. Lastly, when executed, the code should do what it is supposed to.

Thanks to the authors at http://www.macadamian.com/index.php?option=com_content&task=view&id=27&Itemid=31

Advertisements

.NET: Solution-pattern for long-running UI responsive applications

September 26, 2004

Many a time we face this problem of updating the UI while the worker/IO thread is still performing some time consuming background action, fetching results and trying to change the UI while the UI also needs to be “freeze-free” and responsive to a user. Sometimes the UI also needs to support a “Cancel/Close” operation.

There are various solutions to this problem in .NET Winforms — the most commonly used one is the lock mechanism – so as to enable the worker thread to safely update the UI data.

But there is a much simpler scalable solution using message passing, as .NET has 2 nice features — 1) in which a control can tell us whether the caller of a function is a UI thread(control creator thread) or not by using the InvokeRequired property. If the caller is not a UI Thread obviously we will need to make calls to the control through the Invoke method, which is a synchronous method of a control to execute a function on the thread that owns the control’s underlying window handle(UI thread), and the feature 2) in which any delegate(function) can be called asynchronously [using a generated worker thread (from the .NET thread pool) to execute the function asynchronously] by using the BeginInvoke function.

The following code is a general pattern to solve the above problem based on an example in .NET guru Chris Sells book “Windows Forms Programming in C#”. I’d like to thank the author for his example and insight into this problem solution.

Platforms

Tested on .NET 1.1/2.0 and Windows NT, 98, 2000, XP, 2003

Type of Sample:

Create a new WinForms Project in VS.NET.

Main Components of the example:

Drag the following components from the toolbar on to the form

ProgressBar opProgress; //a progressbar indicating job progress

Button longOpButton;//button to start/cancel the operation

TextBox resultsBox;// a text box – read only – for scrolling through results

Label maxOpsLabel;// a label to indicate the operations complete till now

and add the following constant for the num of times to repeat the operation

///

/// This is the maximum amount of times the job will execute

/// Change this if you want better control over the operation.

///

const int MaxDigits = 10000;

Enum for the Operation States

///

/// The states of the Long Operation

///

enum OpState

{

Pending, // No Long worker operation running or canceling

InProgress, // Long worker operation in progress

Canceled, // Long worker operation canceled in UI but not worker

}

OpState state = OpState.Pending; //initial state

Custom EventArgs to be passed to the ShowProgress Handler

/// 
/// class to hold custom Progress event arguments 
/// 
class ShowProgressArgs : EventArgs 
{ 
	public string results; 
	public int totalDigits; 
	public int digitsSoFar; 
	//should the operation be cancelled 
	public bool cancel; 

	public ShowProgressArgs(string results, int totalDigits, int digitsSoFar) 
	{ 
		this.results = results; 
		this.totalDigits = totalDigits; 
		this.digitsSoFar = digitsSoFar; 
	} 
}

ShowProgress delegate and function used to display progress

//delegate that takes a sender and an instance on the custom arguments object. 
delegate void ShowProgressHandler(object sender, ShowProgressArgs e);
/// ShowProgress makes sure the UI thread will handle UI changes(progress updates,etc) 
/// If ShowProgress is called from the UI thread, 
/// it will update the controls, but if it's called from a worker 
/// thread, it uses BeginInvoke to call itself back on the 
/// UI thread. 
void ShowProgress(object sender, ShowProgressArgs e) 
{ 
	// Make sure we're on the UI thread 
	if( this.InvokeRequired == false ) 
	{ 
		resultsBox.Text = e.results; 
		opProgress.Maximum = e.totalDigits; 
		opProgress.Value = e.digitsSoFar; 
		this.maxOpsLabel.Text = e.digitsSoFar.ToString(); 

		Application.DoEvents(); 
		// Check for Cancel 
		e.cancel = (state == OpState.Canceled); 

		// Check for completion 
		if( e.cancel  (e.digitsSoFar == e.totalDigits) ) 
		{ 
			state = OpState.Pending; 
			longOpButton.Text = "Calc"; 
			longOpButton.Enabled = true; 
		} 
	} 
	// Transfer control to the UI thread 
	else 
	{ 
		//send message to UI thread synchronously 
		Invoke(new ShowProgressHandler(ShowProgress), new object[] { sender, e }); 
	} 
}

PerformJob delegate and function which is the Long operation

/// 
/// delegate to call the Long operation asynchronously 
/// 
delegate void PerformJobDelegate(int digits);
/// 
/// The heart of the long operation 
/// can be any kind of worker thread intensive operation. 
/// Here 9 digits are sent to the display at a time 
/// till the maxdigits are reached 
/// 
void PerformJob(int digits) 
{ 
	StringBuilder calcResult = new StringBuilder("", digits + 2); 
	object sender = System.Threading.Thread.CurrentThread; 
	ShowProgressArgs e = new ShowProgressArgs(calcResult.ToString(), digits, 0); 
	// Show progress 
	ShowProgress(sender, e); 

	if( digits > 0 ) 
	{ 
		const string nineDigitsString="123456789-"; 
		for( int i = 0; i 		{ 
			calcResult.Append(nineDigitsString); 

			// Show progress 
			e.results = calcResult.ToString(); 
			e.digitsSoFar = i + 9; 
			ShowProgress(sender, e); 
			// check for Cancel 
			if( e.cancel ) break; 
		} 
	} 
}

The Operation Start/Cancel Button Click Event handler

/// 
/// This technique represents a message passing model. 
/// This model is clear, safe, general-purpose, and scalable. 
/// It's clear because it's easy to see that the worker is creating a message, 
/// passing it to the UI, and then checking the message for information that may 
/// have been added during the UI thread's processing of the message. 
/// It's safe because the ownership of the message is never shared, 
/// starting with the worker thread, moving to the UI thread, and then 
/// returning to the worker thread, with no simultaneous access between 
/// the two threads. It's general-purpose because if the worker or UI 
/// thread needed to communicate information in addition to a cancel flag, 
/// that information can be added to the ShowProgressArgs class. 
/// Finally, this technique is scalable because it uses a thread pool, 
/// which can handle a large number of long-running operations more 
/// efficiently than naively creating a new thread for each one. 
/// For long-running operations in your WinForms applications, 
/// you should first consider message passing. 
/// 
private void PerformOpbtn_Click(object sender, System.EventArgs e) 
{ 
	// Calc button does double duty as Cancel button 
	switch( state ) 
	{ 
			// Start a new Long worker operation 
		case OpState.Pending: 
			// Allow canceling 
			state = OpState.InProgress; 
			longOpButton.Text = "Cancel"; 

			// Async delegate method 
			PerformJobDelegate PerformOp = new PerformJobDelegate(this.PerformJob); 
			//Perform the Long Operation MaxDigits times 
			PerformOp.BeginInvoke(MaxDigits, null, null); 
			break; 

			// Cancel a running Long worker operation 
		case OpState.InProgress: 
			state = OpState.Canceled; 
			longOpButton.Enabled = false; 
			break; 

			// Shouldn't be able to press Calc button while it's canceling 
		case OpState.Canceled: 
			Debug.Assert(false); 
			break; 
	} 
}

Closing Notes:-

  • Please handle the Form Closing Event///
    /// Handle the closing event of this LongOpUI form
    ///
    private void LongOpUI_Closing(object sender, System.ComponentModel.CancelEventArgs e)
    {
    //send a cancel signal
    if (this.state == OpState.InProgress)
    this.longOpButton.PerformClick();
    }

  • Strategy for waiting till the operation completes//”EndInvoke does not return until the asynchronous call completes.
    //This is a good technique to use with file or network operations,
    //but because it blocks on EndInvoke, you should not use it from threads
    //that service the user interface.Waiting on a WaitHandle is a common thread
    //synchronization technique. You can obtain a WaitHandle using the AsyncWaitHandle
    //property of the IAsyncResult returned by BeginInvoke. The WaitHandle is signaled
    //when the asynchronous call completes, and you can wait for it by calling its WaitOne.”
    //– Source MSDN
    IAsyncResult aResult = PerformOp.BeginInvoke(MaxDigits, null, null);
    //Wait for the call to complete
    aResult.AsyncWaitHandle.WaitOne();
    callResult = PerformOp.EndInvoke(aResult);
    MessageBox.Show(“Result of calling PerformJob with ” + MaxDigits + ” is ” + callResult);
    Polling for Asynchronous Call CompletionYou can use the IsCompleted property of the IAsyncResult returned by BeginInvoke
    to discover when the asynchronous call completes. You might do this when making
    the asynchronous call from a thread that services the user interface.
    Polling for completion allows the user interface thread to continue processing
    user input.

    The .NET ThreadPool has a default limit of 25 threads per available processor.
    Change this setting in machine.config, but still your app may land in
    “threadpool starvation” issues as the threadpool is used for almost all
    async callbacks. Timer-queue timers and registered wait operations also use
    the thread pool. Their callback functions are queued to the ThreadPool.
    You can also Queue Work Items to the ThreadPool. Also ASP.NET WebRequests
    are serviced by the ThreadPool.
    So, sometimes you may need to write your own custom ThreadPool to solve
    these issues with the “free” .NET ThreadPool,
    see a sample at http://www.thecodeproject.com/csharp/SmartThreadPool.asp
    Thanks to the Author Ami Bar for that article.