10 Ways To Shoot Yourself In The Foot - Part A

There are several things a software developer can do to make his life much more difficult in the future. One day, some pieces of an old code may make us sorry we haven’t dedicated some more effort when we wrote it, so we have to pay attention and be careful. I am not talking about bad developers who always generate bad code because in those cases, every piece of their code is a disaster. Amit is talking about this issue in Terrible Code Examples - Methods From Hell. In this article I will point out some more .NET advanced issues that even the best developers have to be aware of. There are some things you can do which will cause some problems later on and you will be the one who have to handle with those issues. In other words, I will try to prevent you from shooting yourself in the foot.

Using Non Zero Based Arrays (C#)

VB.NET developers are used to non zero based arrays and are pretty angry that C# arrays have zero based index. I read that they claim that not having non zero based arrays in C# was a dumb design decision. I am not going to argue with those opinions, I am just saying that although most developers don’t know it, we can construct non zero based arrays in C#, but it would be a dumb decision to use them! Lets first see how to do it:

// Construct an array containing ints that has a length of 10 and a lower bound of 1. 
lowerBoundArray = Array.CreateInstance(typeof(int), new int[1] { 10 }, new int[1] { 1 });
// IndexOutOfRangeException the lower bound of the array is 1 not 0.
lowerBoundArray.SetValue(1, 0);

The problem is that you are not developing alone, the other team members will try to iterate the array starting from 0 just because they are used to that. Even if you think that this is not right, there are some basic conventions in C# and you can’t break them. If you do, others will not use your code properly and will have no idea of what went wrong. It is just like a developer who decides to implement a FreeResources method instead of implementing the IDisposable interface. I had to deal with such a case and guess what? I didn’t free the object resources because I didn’t find any Dispose method. I knew about it after searching for the cause of a memory leak. So, please follow the conventions, even if you don’t like them, otherwise you will end up by shooting yourself (or your team members) in the foot.

Read From Resource Files Using The ResourceManager

I already wrote a lot about resource files and string tables in particular. If you read the how to generate public properties for string tables article, you already know that there is an auto-generated class with properties which represent each entry in the resource file. However, some people are still using the ResourceManager class to read from those files. Why is that wrong, you ask? because the parameter passed to the GetString method is a string that shall be exactly like the resource entry. If someone change one of the resource file entries, your code will still compile, but it will crash at runtime! Notice that you might not be the one to face this crash, your customer will. If you use the properties approach, the worst case is that the code won’t compile which means that you will have to fix it.

Catch Exceptions In The Wrong Place

I already talked about this subject in the Software Errors Parade article. Notice to the “Silent Failure” section:

Silent failure may be caused by the irresponsibility of the programmer. Don’t catch every exception everywhere if you don’t handle the error properly, it might lead to data or memory corruption… How many times did you see these kind of code?

private double importantData;
private void ChangeData(int a, int b)
{
    try
    {
        importantData = a / b;
    }
    catch (Exception)
    {
        // You may log here...
    }
}

This code will make a “divide by zero” error go away, but it will cause a much more serious error - data corruption. If you don’t have anything important to do with this error, let higher layers handle it. If you want to catch the exception for logging purposes, you can add a throw; statement after logging (not throw ex because it will reset the exception call stack).

Not Being Aware Of ICloneable Issues

The ICloneable interface contains a single Clone method, which is used to create a copy of the current object. There are two known forms of cloning:

Shallow copy – Creates a new instance of the same type as the original object, and then copies the non static fields of the original object. If the field is a value type, a bit-by-bit copy of the field is performed. If the field is a reference type, the reference is copied but the referred object is not. Therefore, the reference in the original object and the reference in the clone point to the same object.

Deep copy – Creates a new instance of the same type as the original object, and then copies the non static fields of the original object. If the field is a value type, a bit-by-bit copy of the field is performed. If the field is a reference type, a new instance of the referenced type is created. Therefore, the reference in the original object and the reference in the clone doesn’t point to the same object.

So, what kind of copy does the ICloneable.Clone method creates? You don’t know! If you don’t know and use it, troubles will come.

I prefer to avoid the use of ICloneable interface and implement my own interfaces which distinguish between the two forms of copy, in order to be more strict. Despite of what being said, there are cases where you don’t mind what type of copy is being used because the cloned object contains only value types.

Think That Volatile Makes You Thread Safe

Fields that are declared volatile are not subject to compiler optimizations that assume access by a single thread. Multiple threads running on multiple CPU’s will not cache data or re-order operations on a volatile variable, those fields will be used directly. Do you think that this code is thread safe?

public volatile int counter;

void Increment()
{
    counter++;
}

Before answering, let me quote MSDN: “The volatile modifier is usually used for a field that is accessed by multiple threads without using the lock statement to serialize access“. This misleading statement may let you believe that the above code is thread safe, but the truth is that it is definitely not thread safe.

Incrementing a variable requires actually three operation: read, increment and write. Using volatile won’t help us here because the read and the write are separate instructions and another thread could change the value after you’ve read but before you write back. You can use Interlocked.Increment or lock instead.

Summary

Part A of this article is over, I will publish part B in the following days so stay tuned, it will include the other 5 ways to shoot yourself in the foot. I am very curios about your opinions and can’t wait to hear about them. And please, don’t shoot yourself in the foot, it hurts...