<

7 Common Mistakes Made By Programmers in C#

Published on
11,957 Points
5,157 Views
8 Endorsements
Last Modified:
Approved
Community Pick

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.  

Note:  These mistakes are not the worst, or most fatal mistakes, but they show up in a lot of C# code and
they should be avoided.

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;

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);

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;
          }
      }
}

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)
      }
  }

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();
  }

Open in new window

And check out the timing data that shows in the following figure:
Loops : Which is more efficient?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;
}

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);

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();
      }
}

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
      }
}

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";
      }
}

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";
      }
}

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]
          }
      }
}

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
          }
      }
}

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.
8
Comment
Author:starlite551
9 Comments
 
LVL 38

Expert Comment

by:PaulHews
I think in step #3 that the reason it takes almost double the time is because you did not call Reset or Restart on the stopwatch object.  You can verify this easily by using:

System.Threading.Thread.Sleep(1000);

instead of your looping code.

Once you actually reset your stopwatch you will probably find that foreach is actually a lot faster when looping through datatable rows.   I did anyway, when I tried it with a watch.Reset(); in between the two loops.
0
 
LVL 75

Expert Comment

by:käµfm³d 👽
How are you defining "efficiency" for #1? Using string.Format(), while it may be mutable under the hood and I agree cleaner, from what I can see in Reflector, it results in two additional method calls, not to mention an internal loop. The method calls alone would imply there is more overhead in calling Format() than merely doing a string concatenation.
0
 
LVL 75

Expert Comment

by:käµfm³d 👽
You could actually modify #1 to be "String concatenation in SQL queries." Since this type of query building leads to SQL inject-able code, parameterized queries should be favored.  :)
0
Cloud Class® Course: C++ 11 Fundamentals

This course will introduce you to C++ 11 and teach you about syntax fundamentals.

 
LVL 12

Author Comment

by:starlite551
thats right actually.. coz such queries are not practically used Parameterized queries are type safe and ensure that correct formatted data is save to back end.. Avoiding Injections of any sort..
0
 
LVL 12

Author Comment

by:starlite551
I was trying to explain general concatenation tip for strings.. I apologize for using it in a query.. Instead i should have used it in some other string.. :)
0
 
LVL 49

Expert Comment

by:DanRollins
I think it's a good example.  I use that syntax myself often to build SQL queries -- but only where I know the data did not come from user input :-)

By the way, starlite551, can you verify what PaulHews has pointed out?  Is it possible that your timing of for vs. foreach is wrong?  If you can verify and report the results as an Author Comment, it would be a good update to this article.
0
 
LVL 8

Expert Comment

by:Volox
For #6 the new syntax in .Net 3.0 could be used to define an Auto-Implemented property with a private set accessor.   See http://msdn.microsoft.com/en-us/library/bb384054.aspx

Example:
public class MyAccount
{
      public string AccountNumber
      {
          get; private set;
      }
      public MyAccount()
      {
          AccountNumber = "XXXXXXXXXXXXX";
      }
}

Open in new window

0
 
LVL 12

Author Comment

by:starlite551
@Volox :  You are absolutely correct!
0
 
LVL 15

Expert Comment

by:x77
About 7#.

You are right, using numbers to access a data column is not the best option, but using name is not the more eficient option.

the more eficient option is index by DataColumn.
This is importan only on loops.
Note that indexing by string, uses Dictionary to get the datacolumn.

You can code:

      public void MyMethod()
      {
          DataTable dt = DataAccess.GetData();
          DataColumn COL_EMP_ID = dt.Columns["EmpId"];  
          foreach (DataRow row in dt.Rows)
          {
              int empId = Convert.ToInt32(row[COL_EMP_ID]);           }
      }
   
0

Featured Post

Cloud Class® Course: MCSA MCSE Windows Server 2012

This course teaches how to install and configure Windows Server 2012 R2.  It is the first step on your path to becoming a Microsoft Certified Solutions Expert (MCSE).

Join & Write a Comment

Did you know PowerShell can save you time with SaaS platforms? Simply leverage RESTfulAPIs to build your own PowerShell modules. These will kill repetitive tickets and tabs, using the command Invoke-RestMethod. Tune into this webinar to learn how…
Key to your CPU's ability to stay cool is to use the right amount of thermal paste and apply it correctly. In other words you want as much thermal conductivity between CPU and the cooling block. Use a quality thermal paste and apply it in a manner…

Keep in touch with Experts Exchange

Tech news and trends delivered to your inbox every month