.NET Code Reviews - C#
I am often asked to look into an existing code base to give the company an idea of the quality of the code. This process is called Code Review. What i usually do is look for patterns. I look for what they are doing well as well as what they are doing that may be causing problems.
It is important to ensure for software developers that this is not a blame moment, it is rather a learning opportunity for top developers as well as brand new developers who will learn to be better software developers.
Creating things like coding standards and coding analysis rules can be helpful in organizations but they are not enough for sure. I will be listing today some of the issues that i frequently find during C# code review sessions. Lets get started.
C# Code Patterns
Magic strings and numbers
var stream = new FileStream("log.txt",...) // Not Recommended
static readonly string LOGNAME = "log.txt"; // capital letters
var stream = new FileStream(LOGNAME,...); // much better
string concatenation
var s = "This" + " is" + " a" + " string"; // Creates 7 string variables to be garbage collected
var s = string.Concat("This", " is", " a", " string"); // much better
I usually recommend using StringBuilder for longer strings
const vs. static readonly
static readonly can be set from computed values on runtime. A change in static readonly value residing in a separate library will take effect in the host project referencing this library, whereas in case of constants, you will have to rebuild and deploy the host and the library since constants are embedded in the referencing project library itself and cannot be changed on runtime. Sure, there are useful use for const variables. i.e. MVC attributes values only accept constants.
In the next article, we will talk about .NET Classes and its relevant patterns.
Happy programming till then.
Are you familiar with the term 'Code Smell' Although some coding idioms/styles/constructs work they indicate a strong possibility of further problems. A good code reviewer develops this sense of smell with time ( and sometimes lets out a groan at the first whiffs of bad code ... )
Thanks James.
Enjoyed the article. I work on the security side of the house and analyze code from many different customers. I strongly agree with your statement about ensuring that this is not a "blame" moment for developers. Many times while meeting with development teams to brief them on the code review findings, they are quite on-edge and can border on being hostile. I understand, because nobody wants to see their code on the projector being used as an example of "what not to do". As we say, nobody wants their baby to be called ugly. I always try to incorporate some level of training and education into these meetings, and this defensive mindset is a real hurdle to them accepting and adopting the coding practices that I may suggest. I can easily diffuse the situation by reassuring them that we are on the same team, and that the intent is not to point blame, but rather that it is an opportunity to work together and learn better/more secure ways of accomplishing the task. Changing the defensive mindset opens their minds to receiving and accepting the suggestions, and many will adopt the new practices immediately. Again, great article, and I hope to see more soon.