5 Things you should be doing in your code but probably aren’t
This blog post is long overdue since I did the leg work for it months ago. I did a little experiment with my coworkers effectively crowd sourcing the information gathering step and I am finally distilling my findings.
At InterKnowlogy, the nature of our work changes rapidly. We are effectively a collection of custom software consultants. This means we get exposed to various different technologies, methodologies, and coding styles. Our projects range in duration from short term (4-5 weeks) to very long term (1-2 years or more). Across all these different projects I wanted to find common patterns that appear in the code. I was looking for things that could easily be changed to increase clarity, performance, and maintainability.
While the science of my approach could be debated endlessly let me preface this with “it is only a first pass.” Very simply, I asked my colleagues to take the project they were currently working on and run the built in Visual Studio code analysis tool. I asked them to send me the top one or two Code Analysis warnings that showed up most often. In some cases people ran this across multiple projects within a solution and in some cases they just ran it on the largest or most complex project in the solution. From this decidedly unscientific study I have found five common warnings that showed up consistently across projects.
All of these are simple fixes and generally acceptable, low impact fixes that would only increase clarity, robustness, or performance of your code. Now, that doesn’t mean you should blindly implement these suggestions without considering the impact of them. However, you likely have these “errors” in your codebase right now and might benefit from implementing these fixes.
1. CA1823 – Avoid Unused private fields
This one is easy to spot using a tool like ReSharper (R# for short). R# will gray out fields that aren’t being used anywhere (it will do the same for local variables and methods). In our case some people here are not R# fans so they don’t get this visual cue. Often times this is the result a refactoring. You will change some code in a method and the private field you were using is obsolete now but you never go back and clean up the field definition. This is a simple fix and won’t negatively affect anything. Of course it also doesn’t really hurt anything to not remove the fields but in a very large project it can help increase code clarity and reduce the work the compiler has to do to optimize away the unused fields at compile time
2. CA1305 – Specify IFormatProvider
This one is something I have become accustomed to doing just out of force of habit. If you call a method that has an override that accepts an IFormatProvider and don’t explicitly specify the IFormatProvider or the CultureInfo you will get this warning. Most often I see this in String.Format, String.Compare, and DateTime.Parse calls. The MSDN help has a very succinct explanation of how to deal with this error (emphasis mine):
“.NET Framework members choose default culture and formatting based on assumptions that might not be correct for your code. To ensure the code works as expected for your scenarios, you should supply culture-specific information according to the following guidelines:
-
If the value will be displayed to the user, use the current culture. See CultureInfo.CurrentCulture.
-
If the value will be stored and accessed by software (persisted to a file or database), use the invariant culture. See CultureInfo.InvariantCulture.
-
If you do not know the destination of the value, have the data consumer or provider specify the culture.
Even if the default behavior of the overloaded member is appropriate for your needs, it is better to explicitly call the culture-specific overload so that your code is self-documenting and more easily maintained.”
3. CA1709 & CA1704 Identifiers should be cased correctly & Identifiers should be spelled correctly
This is probably the main reason I started running CodeAnalysis consistently on my projects. I am notoriously a terrible speller and typo king (if you suffer through my blog I am sure you have noticed). CodeAnalysis does a good job detecting spelling errors in identifiers and can help you increase your code clarity and ensure you are exposing public API members with proper spelling and casing (I violated my own rule and forgot to run it on Courier and got busted as soon as someone used it and noticed on the of the public methods was misspelled). The MSDN link I provided explains how the spell checking is accomplished and how it breaks identifiers apart into logical words.
The argument on proper casing is more controversial so I will leave it up to you if you want to listen to what Code Analysis has to say about that. Is “id” or “Id” or “ID” ? (For the record it is “Id” according to Code Analysis)
4. CA1062 – Validate arguments of public methods
I am surprised by how often I see this one. Plain and simple, straight from MSDN: “All reference arguments that are passed to externally visible methods should be checked against null.” Again, R# to the rescue here. It very clearly points out when you should be checking for null and gives a quick key stroke to implement the boiler plate code for you. Do you see a pattern here? R# gives you better programming habits. If you are passing in a reference argument put in a null check before you do anything else. How many times have you gotten a NullReferenceException at runtime? About a billion times and a good portion of those could be prevented by simply getting in the habit of null checking reference arguments. From MSDN:
// This method violates the rule.
public void DoNotValidate(string input)
{
if (input.Length != 0)
{
Console.WriteLine(input);
}
}
// This method satisfies the rule.
public void Validate(string input)
{
if (input == null)
{
throw new ArgumentNullException("input");
}
if (input.Length != 0)
{
Console.WriteLine(input);
}
}
5. CA1805 – Do not initialize unnecessarily
This one is mostly a pet peeve of mine. Again R# to the rescue here. It will simply gray out redundant initialization code. Often times, when I see redundant initialization it points out to me a fundamental misunderstanding of how the CLR works. Again, from MSDN:
“The common language runtime initializes all fields to their default values before running the constructor. In most cases, initializing a field to its default value in a constructor is redundant”
If you are declaring private fields there is no need to initialize them (either in line or in the constructor) if you are just setting them to their default value. This just adds code bloat and the compiler will optimize this away anyways. To be sure you know what your fields default value is you can use the handy Default Values Table from MSDN.
Again, most of these issues are minor and won’t cause you grief if you don’t fix them (although CA1305 has the potential to cause some nasty issues). However, if you get in the habit of at least considering these issues and writing the code the “correct” way from the start you will reduce the overall noise in your code and increase the overall “correctness” of your codebase. Also, if you do intend to run Code Analysis on your project, fixing these five issues first will greatly reduce the number of warnings you see (at least according to my highly unscientific study).
If you didn’t notice the other pattern that emerged here it is that I am a fan of R#. It proactively helps you fix these types of issues at the time you write the code. For me, R# makes me a better programmer and helps instill better habits. Without sounding like too much of a shill for R# (too late?) I highly recommend at least trying it out and seeing if you like it. It took me about 8 months of using it before I finally “saw the light.” Now that I have I can’t live without it.
*Full Disclosure: As a Microsoft MVP JetBrains does provide me with complimentary licenses of their software, including R#. I have been using R# prior to being awarded MVP and my experience has been consistently solid throughout.
posted by Brad at
11:13 AM
3 Comments
