7 Common Mistakes Made By Programmers in C#

Introduction

Making mistakes is inevitable in programming.  Even a small mistake could prove to be very costly. The wise thing is to learn from your mistakes and try not to repeat them.  In this article I will be highlighting the mistakes which I consider to be the most common mistakes made by C# developers.  

1. Use Format() for efficient string operations

The String datatype is always an immutable type in the .NET Framework.  When a string is modified it always creates a new copy and never changes the original.  Many developers use a VB- or Javascript-style technique like this to build up a string:

string updateSQL = "UPDATE EmpTable SET Name='" +name+ "' WHERE EmpId=" +id;

Select all Open in new window

That code is really messy, and prone to typing errors.  What's more, since the string is immutable, it creates three unnecessary garbage string copies in the memory as a result of multiple concatenations.

The better approach is to use string.Format() since it internally uses StringBuilder which is mutable:

string updateSQL= string.Format(
                          "UPDATE EmpTable SET Name='{0}' WHERE EmpId={1}",
                          name, id);

Select all Open in new window

It also paves the way for clean, easy-to-read code.

2. Avoid deeply-nesting exception handling

Developers who intend to write nested methods end up doing exception handling for each methods as shown in the below code:

public class NestedExceptionHandling
                      {
                            public void MainMethod()
                            {
                                try  {
                                    ChildMethod1();
                                }
                                catch (Exception exception)  {
                                    //Handle exception
                                }
                            }
                            private void ChildMethod1()
                            {
                                try  {
                                    ChildMethod2();
                                }
                                catch (Exception exception)   {
                                    //Handle exception
                                    throw;
                                }
                            }
                           private void ChildMethod2()
                            {
                                try  {
                                    //some implementation
                                }
                                catch (Exception exception) {
                                    //Handle exception
                                    throw;
                                }
                            }
                      }

Select all Open in new window

So what would happen with the above code if the same exception were handled multiple times and moreover catching many exceptions?  Throwing it will definitely add a performance overhead.

I try to avoid this by putting the exception handling in a single place; that is in the MainMethod as shown below:

public class NestedExceptionHandling
                        {
                            public void MainMethod()
                            {
                                try  {
                                    //some implementation
                                    ChildMethod1();
                                }
                                catch(Exception exception)  {
                                    //Handle exception
                                }
                            }
                        
                            private void ChildMethod1()
                            {
                                ChildMethod2();      // note: No per-method try/catch
                            }
                              private void ChildMethod2()
                            {
                                //some implementation (no per-method try/catch)
                            }
                        }

Select all Open in new window

3. Avoid using [i]foreach[/i] on huge collections

Most developers prefer to use a foreach loop than a for loop because foreach is easier to use. This will however prove to be costly when working with collections with a large amount of data. Consider the below sample in which I am looping through the same DataTable using for and foreach.

static void Main(string[] args)
                        {
                            DataTable dt = PopulateData();
                            Stopwatch watch = new Stopwatch();
                        
                            // ------------------------------------------- for loop
                            watch.Start();
                            for (int count = 0; count < dt.Rows.Count; count++)
                            {
                                dt.Rows[count]["Name"] = "Modified in For";
                            }
                            watch.Stop();
                            Console.WriteLine("Time taken in For loop: {0}", watch.Elapsed.TotalSeconds);
                        
                            // ------------------------------------------- foreach loop
                            watch.Start();
                            foreach (DataRow row in dt.Rows)
                            {
                                row["Name"] = "Modified in ForEach";
                            }
                            watch.Stop();
                            Console.WriteLine("Time taken in For Each loop: {0}", watch.Elapsed.TotalSeconds);
                        
                            Console.ReadKey();
                        }

Select all Open in new window

And check out the timing data that shows in the following figure:
As you can see, the foreach loop is slow -- it takes almost double the amount of time as that of the for loop. This is because in the foreach loop dt.Rows will access all the rows in the datatable.

For bigger collections, always use a for loop in cases where looping is required.

4. Use TryParse() to validate primitive data types

Many developers are not aware of the built-in methods available for validating the primitive data types like System.Int32 and end up doing a custom implementation. The function below iis a typical custom implementation to validate whether the given string is numeric or not.

public bool CheckIfNumeric(string value)
                      {
                            bool isNumeric = true;
                            try  {
                                int i = Convert.ToInt32(value);
                            }
                            catch(FormatException exception)  {
                                isNumeric = false;
                            }
                            return isNumeric;
                      }

Select all Open in new window

Since it involves try/catch, it is not the best way.  A better approach would be to use int.TryParse as shown below:

int output = 0;
                      bool isNumeric = int.TryParse(value, out output);

Select all Open in new window

This technique of using int.TryParse, in my opinion is definitely the faster and cleaner way.

5. Inefficient handling of objects implementing IDisposable

In the .NET Framework, disposing of an object is as important as consuming it. The ideal approach would be to implement the IDisposable interface's dispose method in the class, so after using the object of that class, it can be disposed by calling the dispose method.

Below is a sample where an SqlConnection object is created, used and disposed:

public void DALMethod()
                        {
                            SqlConnection connection = null;
                            try  {
                                connection = new SqlConnection("XXXXXXXXXX");
                                connection.Open();
                                //implement the data access
                            }
                            catch (Exception exception)  {
                                //handle exception
                            }
                            finally  {
                                connection.Close();
                                connection.Dispose();
                            }
                      }

Select all Open in new window

In the above method, the connection dispose is called explicitly in the finally block. In case there is an exception, then the catch block will be executed and after that the finally block will be executed to dispose the connection. So the connection is left in the memory until the finally block is executed. In the .NET Framework, one of the basic guidelines is to release the resource when it is not being used anymore.

Here is a better way of calling dispose:

public void DALMethod()
                      {
                            using (SqlConnection connection = new SqlConnection("XXXXXXXXXX"))
                            {
                                connection.Open();
                                //implement the data access
                            }
                      }

Select all Open in new window

When you have a using block as shown above, the dispose method on the object will be called as soon as the execution exits the block.  That ensures that the SqlConnection resource is disposed and released at the earliest possilbe moment. You should also note that this will work on classes which implement the IDisposable interface.

6. Misuse of public member variables

This may sound simple, but it could really lead to the misuse of the public variables declared and could fetch unexpected usage of your class. Consider this example:

static void Main(string[] args)
                      {
                            MyAccount account = new MyAccount();
                            // The caller is able to set the value which is unexpected
                            account.AccountNumber = "YYYYYYYYYYYYYY";
                            Console.ReadKey();    
                      }
                       
                      public class MyAccount
                      {
                            public string AccountNumber;
                      
                            public MyAccount()
                            {
                                AccountNumber = "XXXXXXXXXXXXX";
                            }
                      }

Select all Open in new window

In the MyAccount class, a public variable AccountNumber is declared and assigned in the constructor.  It is desired that the AccountNumber should be read-only and shouldn't be modified but when declared as shown, the MyAccount class doesn't have any control over it.

A better way to declare a public variable like AccountNumber is to use a property:

public class MyAccount
                      {
                            private string _accountNumber;
                            public string AccountNumber
                            {
                                get { return _accountNumber; }
                            }
                            public MyAccount()
                            {
                                _accountNumber = "XXXXXXXXXXXXX";
                            }
                      }

Select all Open in new window

Here the MyAccount class has a good control over the AccountNumber. It is now read-only and cannot be changed by the caller.

7. Accessing a DataTable values using a hard-coded index

I frequently notice that most programmers access data from a DataTable using column indexes as shown below:

public class MyClass
                      {
                            public void MyMethod()
                            {
                                DataTable dt = DataAccess.GetData();  // fetch data from the database
                                foreach (DataRow row in dt.Rows)
                                {
                                    // BAD: Accessing data through column index
                                    int empId = Convert.ToInt32( row[0] );   // BAD: hardcoded row[0]
                                }
                            }
                      }

Select all Open in new window

In the above scenario... What if the column order in the SQL query fetching data changes?  Your application will break for sure!  Always access the values through column names; one technique is:

public class MyClass
                      {
                            private const string COL_EMP_ID = "EmpId";   
                            public void MyMethod()
                            {
                                DataTable dt = DataAccess.GetData();
                                foreach (DataRow row in dt.Rows)
                                {
                                    //  GOOD: Accessing data through column name
                                    int empId = Convert.ToInt32(row[COL_EMP_ID]);  // use defined constant
                                }
                            }
                      }

Select all Open in new window

The code above won't break, even if the database schema is altered and the column order is changed.  Use a constant variable to hold the column names at a single place so that even if the column name changes in the future then you will have to modify the code in only one place.

Conclusion

I hope you can learn from mistakes myself and other programmers have made in the past.  These are the seven most-common mistakes that I have seen -- I'm sure that you have seen others ones, too!   Please feel free to utilize the comments section to showcase the conflicts and let me know what you think are other common issues as well.