C# Antipatterns

Question

Possible Duplicate:
Common programming mistakes for .NET developers to avoid?

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

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 2

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

Or the more general case:

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

Answer 3

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 4

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

Answer 5

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 6

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

Answer 7

Insulting the law of Demeter:

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

Answer 8

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 9

Private auto-implemented properties:

private Boolean MenuExtended { get; set; }

Answer 10

• 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 11

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 12

Needless casting (please trust the compiler):

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

Answer 13

Throwing NullReferenceException:

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

Answer 14

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 15

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 16

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 17

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.

Answer 18

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 19

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 20

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 21

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 22

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 23

I see following code a lot:

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

should be:

       return (i==3);

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

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 26

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

Exampls

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

Answer 27

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 28

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 29

using Microsoft.SharePoint;

'nuff said

Answer 30

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 31

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 32

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 33

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 34

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 35

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)