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

Comments (37)

  1. Jeff Key says:

    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.

  2. Sean Malloy says:

    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!

  3. 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.

  4. 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.

  5. David Cumps says:

    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.

  6. Doug Reilly says:

    A related favorite of mine is when folks set a property in the property’s own setter. Recursion at work…

  7. Michael Bouck says:

    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()…

  8. Jeff Key says:

    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.

  9. Daren says:

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

    }

    }

  10. Lavos says:

    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.

  11. Juan Felipe Machado says:

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

    }

  12. Juan Felipe Machado says:

    OOPS, Lavos posted while I was writing my post. My code has the exactly same problem as his.

  13. Juan Felipe Machado says:

    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.

  14. 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.

  15. James Higgs says:

    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

    }

  16. Geert says:

    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;

    }

    }

  17. Luc Cluitmans says:

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

  18. Thomas Eyde says:

    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.

  19. Anonymous Coward says:

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

  20. 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.

  21. Another Anonymous Coward says:

    "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

  22. 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!)

  23. 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.

  24. 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.

  25. Juan Felipe Machado says:

    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.

  26. Shaun Wilson says:

    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?

  27. Xicoloko says:

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

    }

    }

    }

  28. Johan De Bock says:

    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

    }

    }

    }

  29. Stephen Johnston says:

    I agree! Without requirements it is impossible to determine if the intention was to eat all exceptions or not.

  30. 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?

  31. Shawn B. says:

    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

  32. Shaun Wilson says:

    "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.

  33. Hello Shaun, I see you understand irony…

  34. Andy Hallock says:

    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.

  35. "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