C# Antipatterns

Question

To cut a long story short: I find the Java antipatterns an indispensable resource. For beginners as much as for professionals. I have yet to find something like this for C#. So I'll open up this question as community wiki and invite everyone to share their knowledge on this. As I am new to C#, I am strongly interested in this, but cannot start with some antipatterns :/

Here are the answers which I find specifically true for C# and not other languages.

I just copy/pasted these! Consider throwing a look on the comments on these as well.

---

Throwing NullReferenceException

Throwing the wrong exception:

if (FooLicenceKeyHolder == null)
    throw new NullReferenceException();

---

Properties vs. public Variables

Public variables in classes (use a property instead).

Unless the class is a simple Data Transfer Object.

---

Not understanding that bool is a real type, not just a convention

if (myBooleanVariable == true)
{
    ...
}

or, even better

if (myBooleanVariable != false)
{
    ...
}

Constructs like these are often used by C and C++ developers where the idea of a boolean value was just a convention (0 == false, anything else is true); this is not necessary (or desirable) in C# or other languages that have real booleans.

---

Using using()

Not making use of using where appropriate:

object variable;
variable.close(); //Old code, use IDisposable if available.
variable.Dispose(); //Same as close.  Avoid if possible use the using() { } pattern.
variable = null; //1. in release optimised away.  2. C# is GC so this doesn't do what was intended anyway.

Answer 1

Rethrowing the exception incorrectly. To rethrow an exception :

try
{
    // do some stuff here
}
catch (Exception ex)
{
    throw ex;  // INCORRECT
    throw;     // CORRECT
    throw new Exception("There was an error"); // INCORRECT
    throw new Exception("There was an error", ex); // CORRECT
}

Answer 2

GC.Collect() to collect instead of trusting the garbage collector.

Answer 3

I see this one way too much, both in Java and C#...

if(something == true){
  somethingelse = true;
}

with bonus points if it also has

else{
  somethingelse = false;
}

Answer 4

using Microsoft.SharePoint;

'nuff said

Answer 5

I see following code a lot:

if (i==3)
       return true;
else
       return false;

should be:

       return (i==3);

Answer 6

Insulting the law of Demeter:

a.PropertyA.PropertyC.PropertyB.PropertyE.PropertyA = 
     b.PropertyC.PropertyE.PropertyA;

Answer 7

Throwing NullReferenceException:

if (FooLicenceKeyHolder == null)
    throw new NullReferenceException();

Answer 8

This is true I seen it with my own eyes.

public object GetNull()
{
     return null;
}

It was actually used in the app, and even had a stored procedure to go with it too, an sp_GetNull that would return null....

that made my day.

I think the sp was used for a classic asp site .. something to do with a result set. the .net one was someone idea of "converting" the code into .net...

Answer 9

int foo = 100;
int bar = int.Parse(foo.ToString());

Or the more general case:

object foo = 100;
int bar = int.Parse(foo.ToString());

Answer 10

I have found this in our project and almost broke the chair...

DateTime date = new DateTime(DateTime.Today.Year, 
                             DateTime.Today.Month, 
                             DateTime.Today.Day);

Answer 11

Quite often I stumble over this kind of var-abuse:

var ok = Bar();

or even better:

var i = AnyThing();

Using var that way makes no sense and gains nothing. It just makes the code harder to follow.

Answer 12

• Lack of null test before delegate invocation.
• Not knowing when and how to use 'as' with a null check vs. a cast with a try/catch.
• 'throw exception' vs. 'throw' within a catch block.
• Instantiating a large number of strings instead of using StringBuilder.
• Deep nesting of using blocks.

Answer 13

Not understanding that bool is a real type, not just a convention

if (myBooleanVariable == true)
{
    ...
}

or, even better

if (myBooleanVariable != false)
{
    ...
}

Constructs like these are often used by C and C++ developers where the idea of a boolean value was just a convention (0 == false, anything else is true); this is not necessary (or desirable) in C# or other languages that have real booleans.

Updated: Rephrased the last paragraph to improve its clarity.

Answer 14

Public variables in classes (use a property instead).

Unless the class is a simple Data Transfer Object.

See comments below for discussion and clarification.

Answer 15

I have actually seen this.

bool isAvailable = CheckIfAvailable();
if (isAvailable.Equals(true))
{ 
   //Do Something
}

beats the isAvailable == true anti-pattern hands down!
Making this a super-anti-pattern!

Answer 16

object variable;
variable.close(); //Old code, use IDisposable if available.
variable.Dispose(); //Same as close.  Avoid if possible use the using() { } pattern.
variable = null; //1. in release optimised away.  2. C# is GC so this doesn't do what was intended anyway.

Answer 17

Private auto-implemented properties:

private Boolean MenuExtended { get; set; }

Answer 18

Declaring and initializing all local variables at the top of each method is so ugly!

void Foo()
{
    string message;
    int i, j, x, y;
    DateTime date;

    // Code
}

Answer 19

Two string anti patterns
Anti-Pattern # 1
Checking strings for null or empty

//Bad
if( myString == null || myString == "" )
OR
if( myString == null || myString.Length == 0 )

//Good
string.IsNullOrEmpty(myString)

Anti-Pattern # 2 (only for .NET 4.0)
Checking strings for null or empty or white space

//Bad
if( myString == null || myString == "" || myString.Trim() == "")

//Good
string.IsNullOrWhiteSpace(myString) 

Answer 20

Needless casting (please trust the compiler):

foreach (UserControl view in workspace.SmartParts)
{
  UserControl userControl = (UserControl)view;
  views.Add(userControl);
}

Answer 21

if(data != null)
{
  variable = data;
}
else
{
  variable = new Data();
}

can be better written as

variable = (data != null) ? data : new Data();

and even better written as

variable = data ?? new Data();

Last code listing works in .NET 2.0 and above

Answer 22

Speaking with an accent always caught me.

C++ programmers:

if (1 == variable) { }

In C# this will give you a compiler error if you were to type if (1 = variable), allowing you to write the code the way you mean it instead of worrying about shooting yourself in the foot.

Answer 23

Not using ternary's is something I see converts to c# do occasionally

you see:

private string foo = string.Empty;
if(someCondition)
  foo = "fapfapfap";
else
  foo = "squishsquishsquish";

instead of:

private string foo  = someCondition ? "fapfapfap" : "squishsquishsquish";

Answer 24

Accessing modified closures

foreach (string list in lists)
{
        Button btn = new Button();
        btn.Click += new EventHandler(delegate { MessageBox.Show(list); });
}

(see link for explanation and fix)

Answer 25

For concating arbitrary number of strings using string concatenation instead of string builder

Exampls

foreach (string anItem in list)
    message = message + anItem;

Answer 26

is this considered general ?

public static main(string [] args)
{
  quit = false;
  do
  {
  try
  {
      // application runs here .. 
      quit = true;
  }catch { }
  }while(quit == false);
}

I dont know how to explain it, but its like someone catching an exception and retrying the code over and over hoping it works later. Like if a IOException occurs, they just try over and over until it works..

Answer 27

The project I'm on had fifty classes, all inheriting from the same class, that all defined:

public void FormatZipCode(String zipCode) { ... }

Either put it in the parent class, or a utility class off to the side. Argh.

Have you considered browsing through The Daily WTF?

Answer 28

Massively over-complicated 'Page_Load' methods, which want to do everything.

Answer 29

Using properties for anything other than to simply retrieve a value or possibly an inexpensive calculation. If you are accessing a database from your property, you should change it to a method call. Developers expect that method calls might be costly, they don't expect this from properties.

Answer 30

Found this a few times in a system I inherited...

if(condition){
  some=code;
}
else
{
  //do nothing
}

and vice versa

if(condition){
  //do nothing
}
else
{
  some=code;
}

Answer 31

I've had this one before:

AnEnum e = AnEnum.Abc;
int i = (int)e;
// lots of code
AnEnum f = (AnEnum)Enum.Parse(i, typeof(AnEnum));

Answer 32

if (state == ((int)RowState.Active).ToString())
else if (state == ((int)RowState.NotActive).ToString())

state is a string post value that contain a value from the enum RowState.

Ultimately this is the way we use to check against the value.

Answer 33

The main issue with .NET seems to be the fact that there are many developers coming from VB 6.0 or (even worse in my opinion, because they mistakenly BELIEVE that they know what to do while VB 6.0 programmers are at least humble enough to be willing to learn something new) Java / C++.

People too ignorant towards modern paradigms, people plastering their code with ugly P/Invoke in the worst C++ - style possible. :-(

Answer 34

Ignorance is bliss (know your framework):

TimeSpan keyDays = new TimeSpan(Licence.LicenceExpiryDate.Ticks);
TimeSpan nowDays = new TimeSpan(System.DateTime.Now.Ticks);

int daysLeft = keyDays.Days - nowDays.Days;

Answer 35

When coding a property, just automatically giving it a getter and setter without thinking about its use. Often the get or set is not used and the property should be read (get) only or write (set) only.

Answer 36

I just saw a few lately.

Never Ending Param Chain

public string CreateJob(string siteNumber, string customer, string jobType, string description, string reference, string externalDoc, string enteredBy, DateTime enteredDateTime)
    {
        //recordtype = 0 for job
        //load assignments and phases set to false
        return Create(0, siteNumber, customer, jobType, description, reference, externalDoc, enteredBy, enteredDateTime, false, false);
    }

public string Create(int recordType, string siteNumber, string customer, string jobType, string description, string reference, string externalDoc, string enteredBy, DateTime enteredDateTime, bool loadAssignments, bool loadPhases)
{
    _vmdh.Fields.FieldByName("WDDOCTYPE").SetValue(recordType, false);
    _vmdh.Fields.FieldByName("NMDOCID").SetValue(-1, false);
    _vmdh.Init();           
        ....
        ...
        // And it keeps going
    }

Wonder what happens at form close

 private void frmAddImages_FormClosing(object sender, FormClosingEventArgs e)
{
    if (DialogResult != DialogResult.OK)
    {
        if (IsDirty)
        {
            e.Cancel = !(MessageBox.Show("Are you sure that you want to exit without saving", "Form Not Saved", MessageBoxButtons.YesNo) == DialogResult.Yes);
        }
    }
    }

Stringly Typed

switch (cbDateFilter.Text)
            {
                case "This Week":
                    dt = DateTime.Now;
                    while (dt.DayOfWeek != DayOfWeek.Monday) dt = dt.AddDays(-1); //find first day of week
                    dtFrom.Value = DateTime.Parse(dt.ToString("dd/MM/yyyy 00:00:00"));
                    dtTo.Value = DateTime.Parse(dt.AddDays(6).ToString("dd/MM/yyyy 23:59:59"));
                    break;

                case "This Month":
                    dt = DateTime.Now;
                    while (dt.Day != 1) dt = dt.AddDays(-1); // find first day of month
                    dtFrom.Value = DateTime.Parse(dt.ToString("dd/MM/yyyy 00:00:00"));
                    dtTo.Value = DateTime.Parse(dt.AddMonths(1).AddDays(-1).ToString("dd/MM/yyyy 23:59:59"));
                    break;

                case "This Quarter":
                    // if at end of Quarter then we need subtract -4 to get to priv Quarter
                    dt = DateTime.Now;
                    while (dt.Month != 7 &&
                        dt.Month != 10 &&
                        dt.Month != 1 &&
                        dt.Month != 4) dt = dt.AddMonths(-1); //find first month, fiscal year
                    while (dt.Day != 1) dt = dt.AddDays(-1); // find first day on month
                    dtFrom.Value = DateTime.Parse(dt.ToString("dd/MM/yyyy 00:00:00"));
                    dtTo.Value = DateTime.Parse(dt.AddMonths(3).AddDays(-1).ToString("dd/MM/yyyy 23:59:59"));
                    break;

Answer 37

Using (bad)

IEnumerable<Bar> foo = ...
if (foo.Count() > 0)
{
    ...
}

instead of (good)

IEnumerable<Bar> foo = ...
if (foo.Any())
{
    ...
}

to test whether an IEnumerable contains anything. Count() has to enumerate through the entire collection with MoveNext(), while Any() only has to call MoveNext() once.

Answer 38

Overuse/abuse of object initializers for everything, probably because of laziness:

var person = new Person
{
    FirstName = "joe",
    (... lots of setters down here)
};

without realizing that this is almost as bad as making all the fields public. You should always take care into creating some valid constructors, that initialize your objects to a valid state.