Welcome to MSDN Blogs Sign in | Join | Help

What's wrong with this code?

TechEd is rapidly approaching, and I'm signed up to do a “C# Best Practices” kind of talk.

Rather bore my audience by presenting my views on implementing IDisposable, I'm going to take the “What's wrong with this code?” approach. My goal is to present code examples that show code that's doing something wrong - be it something prohibited by the language, something that's ill-advised, or something with bad performance - and then let the attendees try to figure out what's wrong with the code.

I have a list of 10 or 15 items already, but I'd like to steal leverage your experience in this area. If you have a “poor practice”, please post the code, and then leave some blank space before you explain why it's bad, so that others can try to figure them out on their own. I'm especially interested in code that you (or somebody else) wrote where you didn't understand initially what the problem was. In other words, the subtle ones.

Here's one of my favorites. What's wrong with this code?

public class Processor
{
    DataStore dataStore;

    public Processor(DataStore dataStore)
    {
        dataStore = dataStore;
    }

    public void Process(DiscountStructure discStruct)
    {
        try
        {
            dataStore.ProcessAllEntries(discStruct);
        }
        catch (Exception e)
        {
             if (e.InnerException.GetType() == typeof(ProcessFailedException))
             {
                 throw new InvalidActionException(e.InnerException);
             }
          }
    }
}

Published Tuesday, March 16, 2004 2:32 PM by ericgu
Filed under: , ,

Comments

Tuesday, March 16, 2004 3:03 PM by Jeff Key

# re: What's wrong with this code?

Not sure if this is what you're looking for, but you're doing a GetType on e.InnerException w/o checking to checking for null first.
Tuesday, March 16, 2004 3:04 PM by Sean Malloy

# re: What's wrong with this code?

Umm.. no this.dataStore in the constructor.. instance of dataStore in the class remains null

so you'll get an exception, only its going to be gobbled up by the catch(Exception) code, and never know anything ahbout it. You'll just assume its working.

I can't remember the name of that problem. But there is a name for it, I'm sure of it!
Tuesday, March 16, 2004 3:10 PM by Mattias Sjögren

# re: What's wrong with this code?

I'd say the big problem is the use of "catch(Exception e)", but I see some other issues with the code inside the catch block as well. It could throw a NullReferenceException if e.InnerException is null, and any catched exception that doesn't pass the check in the if statement gets eaten.
Tuesday, March 16, 2004 3:12 PM by Julien Cheyssial

# re: What's wrong with this code?

I think that in the constructor, the private dataStore variable of the class isn't initialized. Since the parameter passed to the constructor has exactly the same name as the private variable, the "dataStore = dataStore" just reassigns the dataStore parameter to itself, and not to the variable of the class.

So when calling Process(), an exception will be raised since dataStore won't be initialized.

I don't know if I made myself clear, since I'm not totally fluent in english.
Tuesday, March 16, 2004 3:13 PM by David Cumps

# re: What's wrong with this code?

I'll go with the "dataStore = dataStore" as well, the compiler has no idea which dataStore is the variable in the constructor scope, and which one is the private variable in the class.
Tuesday, March 16, 2004 3:20 PM by Doug Reilly

# re: What's wrong with this code?

A related favorite of mine is when folks set a property in the property's own setter. Recursion at work...
Tuesday, March 16, 2004 3:20 PM by Michael Bouck

# re: What's wrong with this code?

I agree with Julien which is why I like using a common prefix for class-owned private fields (e.g. _dataStore). If this was the case in the example then you could have done _dataStore = dataStore and get the desired result. As it is, you'll need to change it to this.dataStore = dataStore to do the right thing.

This problem speaks to a similar problem of how not to implement the == operator. It's not hard to implement == the wrong way and end-up infinitely recursing instead of calling Object.Equals()...
Tuesday, March 16, 2004 3:21 PM by Jeff Key

# re: What's wrong with this code?

I think many developers don't have a good enough understanding of some of the nastiness associated with boxing. Here's a very simple snippet. What's the output?

public class MyClass
{
public static void Main()
{
int i = 3;
object o = i;

/* 1 */ Console.WriteLine(i == (int)o);
/* 2 */ Console.WriteLine((object)i == o);
/* 3 */ Console.WriteLine(i.Equals(o));
/* 4 */ Console.WriteLine(o.Equals(i));
/* 5 */ Console.WriteLine(AreEqual(i, i));
/* 6 */ Console.WriteLine(AreEqual(o, o));
}

static bool AreEqual(object a, object b)
{
return a == b;
}
}















1: True. o is unboxed and a value comparison is made.
2: False. i is cast to object and a reference comparison is made.
3: True. Same as 1.
4: True. o is a boxed int, so it's virtual Equals is called.
5: False. Are equal does a reference comparison of the boxed ints.
6: True. Reference comparison of the same object.
Tuesday, March 16, 2004 3:44 PM by Daren

# re: What's wrong with this code?

One of my all time favorites is:

public class MyClass
{
public class MyInnerClass
{
private int myValue;
public int MyValue
{
get { return MyValue; }
}

public MyInnerClass(int initial)
{
myValue = initial;
}
}

private MyInnerClass myInnerClass;

public MyClass(int something)
{
myInnerClass = new MyInnerClass(something);
Console.WriteLine("My property = {0}", myInnerClass.MyValue);
}
}
Tuesday, March 16, 2004 3:59 PM by Lavos

# re: What's wrong with this code?

There are always the standbys. Catching exceptions instead of checking error conditions first. (I actually have code from a collab partner that catches all exceptions, then checks to see if a parameter is null to decide if it should rethrow or swallow. It swallows on 99.9% of all cases :(

However, this one tricked me:

public struct MyStruct
{
public int a;
public void ModA()
{
Console.Writeline(a++);
}
}

public delegate void SomeOp();

public class EntryPoint
{
void Main()
{
MyStruct myStruct;
myStruct.a = 20;

SomeOp del = new SomeOp(myStruct.ModA);

myStruct.a = 40;

del();

Console.WriteLine(myStruct.a);
}
}

What's the output?















I first encountered this when trying to use a structure's method as a callback.

The answer is:
20
40

The delegate AFAIK actually has a reference to a boxed copy of the structure, not a reference to the structure. A very important distinction and another good reason not to treat structs as classes.
Tuesday, March 16, 2004 4:06 PM by Juan Felipe Machado

# re: What's wrong with this code?

To Eric:
The lack of this.dataStore and the fact that the block catch(Exception e) doesn't rethrow the exception is violating a design guideline.

To Jeff:
I think this is a pretty good one, it really make me think... my guess : 1.true (the numbers are compared), 2. false (the object references are compared), 3.true (o is casted to int so the numbers are compared), 4.false (i is casted to object so the object references are compared), 5. fase (the ints are casted to object independently so the references are diferent), 6. true (the references are the same).

BTW I have a favorite one too:

public struct S
{
public int X;
public void ChangeX(int y)
{
X = y;
}
}

public S[] arr = new S[5];
for (int i = 0; i < 5; ++i)
{
arr[i].ChangeX(i);
}
for (int i = 0; i < 5; ++i)
{
Console.WriteLine(S[i].X.ToString());
}
Tuesday, March 16, 2004 4:09 PM by Juan Felipe Machado

# re: What's wrong with this code?

OOPS, Lavos posted while I was writing my post. My code has the exactly same problem as his.
Tuesday, March 16, 2004 4:35 PM by Juan Felipe Machado

# re: What's wrong with this code?

To Daren:
I'm not completely sure, but I think is the same problem when you call a virtual method in the constructor, the inner class isn't fully created and you are calling a method (get_MyValue()) on it, so probably (I'm not sure, I repeat) an exception will be thrown.
Tuesday, March 16, 2004 11:11 PM by Omer van Kloeten

# re: What's wrong with this code?

I'll go with all of what's been said here and one more thing: The checking for the type is not neccesarily the right way to go at all times.
I'd prefer using:
e.InnerException is ProcessFailedException
instead of:
e.InnerException.GetType() == typeof(ProcessFailedException)
Since it supports an exception inheriting from ProcessFailedException to be thrown as well.
Wednesday, March 17, 2004 2:53 AM by Code/Tea/Etc...

# Eric Gunnerson just keep pumping out the cool posts...

Wednesday, March 17, 2004 12:33 AM by James Higgs

# re: What's wrong with this code?

Surely a more readable way to write the same code, and without the side-effect of swallowing exceptions would be this:

try {
dataStore.ProcessAllEntries(discStruct);
} catch (ProcessFailedException pfe) {
throw new InvalidActionException(pfe.InnerException);
}

or, if you really do want to swallow the exceptions, then this:

try {
dataStore.ProcessAllEntries(discStruct);
} catch (ProcessFailedException pfe) {
throw new InvalidActionException(pfe.InnerException);
} catch {
//ignore unanticipated exceptions
}
Wednesday, March 17, 2004 12:40 AM by Geert

# re: What's wrong with this code?

To Juan Felipe:

Actually, your code doesn't compile. I had to change the line
Console.WriteLine(S[i].X.ToString());
to
Console.WriteLine(arr[i].X.ToString());
But I'll assume that was what you meant.

After changing that, I found that your code works as expected, it prints out 0 1 2 3 4.

But perhaps I misunderstood your intention.

This is the code I used:
class Class1
{
[STAThread]
static void Main(string[] args)
{
S[] arr = new S[5];
for (int i = 0; i < 5; ++i)
{
arr[i].ChangeX(i);
}

for (int i = 0; i < 5; ++i)
{
Console.WriteLine(arr[i].X.ToString());
}
}
}

public struct S
{
public int X;
public void ChangeX(int y)
{
X = y;
}
}
Wednesday, March 17, 2004 2:41 AM by Luc Cluitmans

# re: What's wrong with this code?

Whats wrong with the code has been mentioned a few times now, I guess.

Though... Did anyone mention already that the exception testing code won't work when there are multiple exception nesting levels? If you want to test against the root cause of your exception, test e.GetBaseException(), not e.InnerException. And the code will act funny if there is no InnerException, of course.

What I wonder is if the compiler should generate a warning on the 'dataStore = dataStore;' statement in the constructor, something like 'Warning: this statement has no effect'.

And of course, this is a perfect example of why you should use some naming convention that prevents arguments, fields and property names from ever clashing. Of course *which* naming convention to use is a religious war... (I stick to the old delphi convention: all field names start with a capital F followed by another capital letter; had I written the example code, there would have been a field FDataStore ...)
Wednesday, March 17, 2004 2:56 AM by Thomas Eyde

# But what does it do?

All of the names reveals very little about the intention of this code.

What does Process() do with the DiscountStructure?

What should it do when it fails?

And I think something is wrong in the design when all Process() does, is just to wrap a try..catch around a delegation.
Wednesday, March 17, 2004 6:15 AM by Anonymous Coward

# re: What's wrong with this code?

There are no requirements. Without requirements or a maybe some test cases, I can't say anything is wrong with this code.
Wednesday, March 17, 2004 7:07 AM by Larry Osterman

# re: What's wrong with this code?

It doesn't matter whether you have requirements or not :) As several people above have pointed out, the problem is:

catch (Exception e)

If you EVER see this construct in production code, watch out, it's almost always a bug waiting to be happened. Especially if, as in this example, the exception isn't rethrown.
Wednesday, March 17, 2004 9:23 AM by Another Anonymous Coward

# re: What's wrong with this code?

"There are no requirements. Without requirements or a maybe some test cases, I can't say anything is wrong with this code. "

Funny! (and a good point...) +1
Wednesday, March 17, 2004 9:25 AM by Ernst Kuschke

# re: What's wrong with this code?

1. dataStore in the Processor method would reference the passed parameter, and thus only set it to itself.

2. There is no need for the Process method, since it just calls another method, passing its parameter as is. This function (ProcessAllEntries) exists on the dataStore passed, and could thus be called as

dataStore.ProcessAllEntries(discStruct);

instead of

Processor p = new Processor(dataStore);
p.Process(discStruct);

and the error handling be implemented in the dataStore class as well. This would mean that this Processor class is redundant :)

(I might be having a late night, and be mistaken!)
Wednesday, March 17, 2004 10:01 AM by Christian Romney

# re: What's wrong with this code?

I think there's a mix up between a flat out bug (most likely unexpected behavior) and poor design or not adhering to best practices. dataStore = dataStore falls in the first category, catch (Exception e) falls into the second. The former statement's intent is clear - to initialize the private variable. The latter statement's motivation cannot be known without annotation. If we want to pick on poor best-practice adherence, we could gripe about the lack of a private access modifier on the DataStore. Relying on the premise that everyone *knows* that it will be private by default because it's in the language spec is a bad idea in the face of the newbie coder hired to maintain this junk.
Wednesday, March 17, 2004 12:10 PM by Phillip Trelford

# re: What's wrong with this code?

On the subject poor C# Practice examples I'd suggest a good starting point as general anti-patterns, here is an example:
http://mindprod.com/unmain.html

I've uncovered some appalling C# code over the last year, here is one of my favourites:)

public class Customer : Address
{
protected DataTable custTbl = null;
protected DataTable tempTbl = null;
protected XmlDocument xmlDocument = null;
protected XmlNode xmlNode1 = null;
protected XmlNode xmlNode2 = null;
protected XmlNode xmlNode3 = null;

void SetAddress1(string XML)
{
xmlDocument = new XmlDocument();
// ...
}

void SetAddress2(string XML)
{
xmlDocument.LoadXml( XML );

}


The *rationale* given behind this code was "I might want to use a variable again so if I define them all at class scope, I won't need to define them again, and that'll mean its faster too... and when I subclass I can reuse all those variables aswell..."
So Customer is an Address? Or Customer has an Address or Addresses?
Side effects anyone? SetAddress2 better hope that SetAddress1 has been called first.
Wednesday, March 17, 2004 1:22 PM by Juan Felipe Machado

# re: What's wrong with this code?

All right, I'm sorry, I didn't test the code I posted before writing my post. This is the real tested code thah HAS a problem in it.

public static void Main()
{
C[] arr = new C[5];
for (int i = 0; i < 5; ++i)
{
arr[i] = new C();
arr[i].Prop.ChangeX(i);
}
for (int i = 0; i < 5; ++i)
Console.WriteLine(arr[i].Prop.X.ToString());
}
public class C
{
private S _s = new S();
public S Prop
{
get {return _s;}
}
}

public struct S
{
public int X;
public void ChangeX(int y)
{
X = y;
}
}











The problem here is regarding mutability in structs. The C# structs are mutable, so they allow you to call functions that change the internal state of the struct. If instead of
arr[i].Prop.ChangeX(i);
I'd called
arr[i].Prop.X = i;
the compiler will tell me there's an error, but since I'm changing the internal state of the struct with a function, the compiler didn't warn me, and the bug apears. BTW, the above code print a list of 5 0's.
The same problem happens with something like this as Peter Golde points out in his blog (http://peter.golde.org/2003/11/04.html#a20):

private void Form1_MouseDown(object sender, System.Windows.Forms.MouseEventArgs e)
{
DesktopBounds.Inflate(30, 30);
}

in this case you are actually inflating just a temp variable and not the size of your form.

Again, I'm really sorry.
Wednesday, March 17, 2004 5:03 PM by Shaun Wilson

# re: What's wrong with this code?

public class Processor
{
DataStore dataStore = null; // be clear

public Processor(DataStore dataStore)
{
if (dataStore == null) // ensure it's not null
throw new ArgumentInvalidException("dataStore cannot be null");
dataStore = dataStore;
}

public void Process(DiscountStructure discStruct)
{
try
{
dataStore.ProcessAllEntries(discStruct);
}
catch (Exception e)
{
if ((e.InnerException != null) && e.InnerException.GetType() == typeof(ProcessFailedException)))
{
throw new InvalidActionException(e); // do not throw away exception details, bad practice where i work.
}
throw e; // don't forget to rethrow if unhandled
}
}
}

what did i miss?
Thursday, March 18, 2004 4:04 AM by Xicoloko

# re: What's wrong with this code?

To Juan: as you said the problem is related to the fact that the struct is not a reference type but a data type. This modified sample shows the differences:

public class Foo
{
private S _s = new S();
public S PropS
{
get {return _s;}
}

private C c = new C();
public C PropC
{
get { return this.c; }
}
}

public struct S
{
public int X;
public void ChangeX(int y)
{
X = y;
}
}

public class C
{
public int X;
public void ChangeX(int y)
{
X = y;
}
}


public class MyClass
{
public static void Main()
{
Foo[] arr = new Foo[5];
for (int i = 0; i < 5; ++i)
{
arr[i] = new Foo();
arr[i].PropC.ChangeX(i);
arr[i].PropS.ChangeX(i);
Console.WriteLine("Is the struct the same? " + object.ReferenceEquals(arr[i].PropS, arr[i].PropS));
Console.WriteLine("Is the class the same? " + object.ReferenceEquals(arr[i].PropC, arr[i].PropC));
}

for (int i = 0; i < 5; ++i)
{
Console.WriteLine(arr[i].PropC.X.ToString() + "\t" + arr[i].PropS.X.ToString());
}
}
}

Thursday, March 18, 2004 4:22 AM by Johan De Bock

# re: What's wrong with this code?

Nice response of Shaun Wilson ...
I only disagree on one point:

use throw; in stead of throw e;

Johan

public class Processor
{
DataStore dataStore = null; // be clear

public Processor(DataStore dataStore)
{
if (dataStore == null) // ensure it's not null
throw new ArgumentInvalidException("dataStore cannot be null");
dataStore = dataStore;
}

public void Process(DiscountStructure discStruct)
{
try
{
dataStore.ProcessAllEntries(discStruct);
}
catch (Exception e)
{
if ((e.InnerException != null) && e.InnerException.GetType() == typeof(ProcessFailedException)))
{
throw new InvalidActionException(e); // do not throw away exception details, bad practice where i work.
}
throw; // Rethrow so that you don't loose context information : do not hide the correct stack trace.
//////throw e; // don't forget to rethrow if unhandled
}
}
}
Thursday, March 18, 2004 10:38 AM by Stephen Johnston

# re: What's wrong with this code?

I agree! Without requirements it is impossible to determine if the intention was to eat all exceptions or not.
Thursday, March 18, 2004 5:44 PM by Phillip Trelford

# re: What's wrong with this code?

MSDN: .NET Framework Class Library -> DataTable.PrimaryKey Property example ( yes, look it up):

private void SetPrimaryKeys(){
// Create a new DataTable and set two DataColumn objects as primary keys.
DataTable myTable = new DataTable();
DataColumn[] keys = new DataColumn[2];
DataColumn myColumn;
// Create column 1.
myColumn = new DataColumn();
myColumn.DataType = System.Type.GetType("System.String");
myColumn.ColumnName= "FirstName";
// Add the column to the DataTable.Columns collection.
myTable.Columns.Add(myColumn);
// Add the column to the array.
keys[0] = myColumn;

// Create column 2 and add it to the array.
myColumn = new DataColumn();
myColumn.DataType = System.Type.GetType("System.String");
myColumn.ColumnName = "LastName";
myTable.Columns.Add(myColumn);
// Add the column to the array.
keys[1] = myColumn;
// Set the PrimaryKeys property to the array.
myTable.PrimaryKey = keys;
}

Most of the replies here seem to mostly complain about context in Eric's example, but lets dig a little.
myTable? myColum?
Why System.Type.GetType("System.String")? Why not typeof(System.String)?
Here is another example...

DataTable personTable = new DataTable(); // please why can't I just say DataTable person; as I would in C++
DataColumn firstName = new DataColumn(); // same again
firstName.ColumnName = "FirstName"; // why not use the constructor overloads?
firstName.DataType = typeof(System.String); // why would I call a function and make it look up a string?
DataColumn secondName = new DataColumn("SecondName", typeof(string) );
// damn could have used constructor overload from the start
personTable.PrimaryKeys = new DataColumn[] { firstName, secondName }; // Hello, arrays

So looks like the MSDN examples are really setting people up well...
Even better why not use first and second names for primary keys, identities are so last year?





Friday, March 19, 2004 11:23 AM by Shawn B.

# re: What's wrong with this code?

I've been down this road before. The problem is in the constructor. you have to specify "this.datastore = datastore" or else it'll not know what "datastore" to set. It'll keep setting the parameter instead.


Thanks,
Shawn
Friday, March 19, 2004 3:34 PM by Shaun Wilson

# re: What's wrong with this code?

"Even better why not use first and second names for primary keys, identities are so last year?"

Because there may be more than one "Wilson, Shaun" on this planet.


Friday, March 19, 2004 6:10 PM by Phillip Trelford

# re: What's wrong with this code?

Hello Shaun, I see you understand irony...

Monday, March 22, 2004 2:45 PM by Andy Hallock

# re: What's wrong with this code?

This is a little off topic, but the Processor example brought up a few discussions and exception handling, and I'd like to know when you should swallow an exception and when you should rethrow an exception. I'm trying to institute better practices here at work.
Wednesday, March 24, 2004 5:00 PM by Phillip Trelford

# re: What's wrong with this code?

"when you should swallow an exception and when you should rethrow"

Exceptions should probably always be swalled before they hit the user of the application,
so that instead of giving them "SQL Server error 123456" you should give a user friendly message.
Elsewhere unexpected exceptions generally should not be swallowed, as you are just hiding an issue, but if you must then you should consider logging it, so atleast you have a chance of finding out what might be going wrong.

Here are some exception handling tips and links:
i) Do not rely on exceptions in your code. Since exceptions cause performance to suffer significantly, you should never use them as a way to control normal program flow. If it is possible to detect in code a condition that would cause an exception, do so.
ii) Your own application exceptions should derive from System.ApplicationException.
iii) Exceptions should be caught and logged at the application boundary, and a human readable error message should be presented to the final user.
iv) Read the documentation for the types you are calling and catch and handle their specific exceptions, and document the exceptions you throw from your own code.
v) Use exception nesting when rethrowing, if you have extra useful information to add.

See:
.Net Framework Developer's Guide:
Exception Handling Fundamentals: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpguide/html/cpconexceptionhandlingfundamentals.asp
Best Practices for Handling Exceptions: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpguide/html/cpconbestpracticesforhandlingexceptions.asp

Writing Exceptional Code: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dncscol/html/csharp07192001.asp">http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dncscol/html/csharp07192001.asp
The Well-Tempered Exception: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dncscol/html/csharp07192001.asp">http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dncscol/html/csharp07192001.asp
Exception Management Architecture Guide: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnbda/html/exceptdotnet.asp

Tuesday, June 09, 2009 3:14 PM by Eric Gunnerson s C Compendium What s wrong with this code | Green Tea Fat Burner

# Eric Gunnerson s C Compendium What s wrong with this code | Green Tea Fat Burner

PingBack from http://greenteafatburner.info/story.php?id=973

New Comments to this post are disabled
 
Page view tracker