While doing code reviews, you must have found common issues in the code. I am gonna list down some common problems in the developer’s code written in .Net. I have experienced these issues quite recently, so thought to jolt upon these. These could also help building up code review guidelines as well. This post has grown quite long, my apologies for that 
but certainly this subject can be extended to as much as possible and its quite little as far as this subject is concerned.
Unused Code
As we progress in our development, we face more un-used code. That un-used code not only produces huge size as well as make the code less readable. It produces burden on the compiler as well. .Net assemblies are dynamic, there is no way that compiler could get rid of the code automatically. We will need to use some third party tools for that. Either you may use http://www.preemptive.com/dotfuscator.html OR www.xenocode.com.
Throwing Exceptions
Exceptions are either not caught properly or used but improperly.
- First let me tell you what do I mean by saying that exceptions are not caught properly. You will experience no try/catch OR empty catch block OR Exception class being caught OR the exception not logged at any place. If we do not log exception at any place, we will never be able to trace what went wrong in the production environment. Secondly, make it sure that you should have alternate mechanism for logging exception. For instance, you were logging in event viewer and it becomes full. Now when you will log another, you will experience an exception in your Logger class which is logging so you should have try/catch block even in your logger. At many times, developers argue a lot how come Logger class may also get crashed. The answer is yes, logger class can also get crashed so look for alternate mechanisms as well. Otherwise, be ready to play a blind game with production issues
- When I say exceptions used improperly, I mean to use “throw” instead of “throw ex” (where ex is the object containing exception). You should ask what is the difference between them ? So here we go. The difference is that “throw” preserves the original stack trace whereas “throw ex” truncates the stack trace below the method in which the “throw ex” call is located. It means if exception has occurred in a method being called, using “throw ex” we will never know where actually exception has been generated.
Multi-dimensional vs Jagged Arrays
A jagged array is a type of array whose elements are arrays as well. Benefit is that, each array within jagged array can be of different sizes, leading to less wasted space for some sets of data. Example is as follows:
Private jaggedArray As Integer()() = {New Integer() {1, 2, 3, 4}, _
New Integer() {5, 6, 7}, _
New Integer() {8}, _
New Integer() {9}}
Private multiDimentionalArray As Integer(,) = {{1, 2, 3, 4}, _
{5, 6, 7, 0}, _
{8, 0, 0, 0}, _
{9, 0, 0, 0}}l
So as you see in case of multi-dimensional array, we need to initialize the same set of values. So preferable use jagged arrays.
Commented Code
Test code is commented out during development and never removed. Make sure to remove such code to improve readability and formatting of your code. Otherwise, it will cause problems during maintenance.
Overridden Methods Instead Of Default Parameter Values
Microsoft says that methods that use default parameters are allowed under the Common Language Specification (CLS); however, the CLS allows compilers to ignore the values assigned to these parameters. Code written for compilers that ignore default parameter values must explicitly provide arguments for each default parameter. To maintain the behavior that you want across programming languages, methods that use default parameters should be replaced with method overloads that provide the default parameters. The compiler ignores the values of default parameters for C#. (Ref: http://msdn.microsoft.com/en-us/ms182135.aspx)
Use Static Methods Wherever Applicable
All the methods which do not access any instance variables should be marked as static or shared. It will improve performance.
Objects Instantiation
Objects are instantiated in loops which runs several times, thus creating more and more memory. Avoid creating new object on each loop iterations OR nullify the previous one so that on next iteration same instance is being used. Be careful in case of string += operation which will always create two string instances in memory. Instead use StringBuilder.
Int16 OR Int32
At many times, developers will prefer using Int16 over Int32 if they are executing a loop that involves counter less than 32,000 or something similar. But on the contrary, MS says something else. Runtime optimizes performance of Int32 and recommends it for counters and other frequently accessed operations. (Ref MCTS Self-Paced Training Kit (Exam 70-536): Microsoft® .NET Framework 2.0—Application Development Foundation). Int16 is good if you want your data structure to be as lean as possible. Otherwise, prefer using Integer. Similarly for floating points, Double is the most efficient data type to be used.
Returning Method
Make sure function returns from each code path. All code paths should return something or the other. Otherwise, you may face problems during different scenarios. It will be possible if each method is simple, as we move towards more complex methods it will be difficult to monitor each code path returns something.
Comments Phobia
Usually developers are faced with a phobia not to write down comments. With Visual Studio 2005 and onwards it has become very easy to write down comments before namespace, class, method etc. Use shortcut of ”’ and block of comments will be available. You may just fill in the spaces to complete your comments block which will be very beneficial for others. Moreover, it could be sued to generate documentation later on.
Dispose or Not to Dispose
There had been a lot of debates lately between me and a lot of developers. That gives me inspiration to further look down at this issue. I have came across certain findings as well in that regard as well. Certainly GC collects all managed objects. First lets look at the difference between Dispose and Finalize:
Dispose is the normal method by which unmanaged resources are freed up. It is called in the normal way from user code, often via a Close method.
Finalize is a safeguard to clean up resources in the event that the Dispose method is not called ( a programming error ).
GC calls finalize method but do not call Dispose() method. Dispose method is available with only objects that EITHER has some unmanaged resources associated with it OR for objects that encourages its child classes to implement it.
Having said that, it is apparent that if an object has a Dispose() method, it means it is recommended to call it. If you are too much curious why you should do it, use Reflector to look at the code of the assembly that what its Dispose is doing. I would always recommend using Dipose().
In the following are some links that you may visit to read more about discussion about it:
http://forums.asp.net/p/466400/593963.aspx
http://bytes.com/forum/thread279478.html
admin .Net .net coding, code guidelines, code review, coding compliance, coding specialist
Recent Discussions