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

• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  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!
• Anonymous
  March 16, 2004
  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.
  
• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  A related favorite of mine is when folks set a property in the property's own setter. Recursion at work...
  
• Anonymous
  March 16, 2004
  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()...
• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  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);
  }
  }
  
• Anonymous
  March 16, 2004
  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.
  
• Anonymous
  March 16, 2004
  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());
  }
  
• Anonymous
  March 16, 2004
  OOPS, Lavos posted while I was writing my post. My code has the exactly same problem as his.
• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  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
  }
• Anonymous
  March 16, 2004
  The comment has been removed
• Anonymous
  March 16, 2004
  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 ...)
  
• Anonymous
  March 16, 2004
  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.
• Anonymous
  March 16, 2004
  There are no requirements. Without requirements or a maybe some test cases, I can't say anything is wrong with this code.
  
• Anonymous
  March 16, 2004
  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.
  
• Anonymous
  March 17, 2004
  "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
• Anonymous
  March 17, 2004

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

• Anonymous
  March 17, 2004
  The comment has been removed
• Anonymous
  March 17, 2004
  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.
  
• Anonymous
  March 17, 2004
  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.
• Anonymous
  March 17, 2004
  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?
  
• Anonymous
  March 17, 2004
  The comment has been removed
• Anonymous
  March 17, 2004
  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
  }
  }
  }
  
• Anonymous
  March 18, 2004
  I agree! Without requirements it is impossible to determine if the intention was to eat all exceptions or not.
• Anonymous
  March 18, 2004
  The comment has been removed
• Anonymous
  March 19, 2004
  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
• Anonymous
  March 19, 2004
  "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.
  
  
  
• Anonymous
  March 19, 2004
  Hello Shaun, I see you understand irony...
  
  
• Anonymous
  March 22, 2004
  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.
• Anonymous
  March 24, 2004
  "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
  
  
• Anonymous
  June 09, 2009
  PingBack from http://greenteafatburner.info/story.php?id=973