write(this); //not that

One of the first things you learn when you’re starting to develop software is flow control.  By flow control, I mean if-statements, loops and that sort of thing.  Imagine my surprise and subsequent urge to empty the contents of my stomach all over my keyboard when I ran across this today:

private void SetQuantityOnHand(Item forItem)
{

  while (forItem.IsAutoReplenished)
    forItem.QuantityOnHand = 0;

  // Remainder of method removed for brevity.
}

When posting source code to this site I usually make an attempt to justify what the developer was thinking when he wrote the code in question, but this one is pretty hard to justify.  Listen folks, while loops are awesome… when you have something to loop through!  Although I feel that the solution to this one is pretty self-explanatory (or at least I hope that it is), here is how I wound up refactoring the code:

private void SetQuantityOnHand(Item forItem)
{  

  if (forItem.IsAutoReplenished)
    forItem.QuantityOnHand = 0;

  // Remainder of method removed for brevity.
}

That was painless enough.  I usually conclude my posts with a helpful tip but, if you don’t know the difference between an if-statement and a loop, it might be time to hit the books again.

Good code comments are great.  Useless ones are the root of all evil!  Stop with the useless and redundant comments.

private void CreateTheProcessingThread()
{
	// Create the processing thread
	new Thread(ProcessingThread).Start();
}	// End of method CreateTheProcessingThread()

Really?!  I couldn’t really tell what was going on there… all of those ridiculous comments were clouding what was really going on.  Making the comment the actual name of the method and adding spaces is just asinine!

And do you really need an entire method to make one call?  It’s okay if you do because you might eventually change the implementation of that method… but please just do it like this instead:

private void CreateTheProcessingThread()
{
	new Thread(ProcessingThread).Start();
}

You ever run across some source code that you wish you could unsee?  Yeah, me too.  It happened to me yesterday.  Take a look at this:

if (View.ExistingQuantity > 0)
{
   Model.UpdateInventoryAdjustmentItem();
}
else if (View.ExistingQuantity < 0)
{
   // Don't do anything if existing quantity
   // is less than zero.
}
else
{
   Model.AddInventoryAdjustmentItem();
} 

So, if the existing quantity is less than zero… read this comment.  Genius!  Listen folks, business requirements have a time and a place.  This is neither the time or the place.  Interestingly enough, the only way the final else condition will be met is if the existing quantity is zero.  So, although I feel the correct solution here is pretty obvious, let’s go ahead and take a look for good measure:

if (View.ExistingQuantity > 0)
{
   Model.UpdateInventoryAdjustmentItem();
}
else if (View.ExistingQuantity == 0)
{
   Model.AddInventoryAdjustmentItem();
}

Remember, comments aren’t code.  The CLR won’t touch them.  Seriously.  No kidding.  If you don’t need to do anything, then just… don’t do anything.

Seriously, why write one line of code when you can have a dozen?

//...
switch (boolStatus)
{
   case true:
   {
      break;
   }
   default:
   {
      return false;
   }
}

return true;

I get the impression that this was a remnant of a refactoring that was not taken to completion.  Or at least, I’m hoping that’s what it is.

A few months ago, I ran across this abstract class in a project that I was heading up:

public abstract class AuditableEntity
{
  [Column]
  public DateTime DateCreated { get; set; }

  [Column]
  public DateTime DateLastUpdated { get; set; }

  [Column]
  public string CreatedBy { get; set; }

  [Column]
  public string LastUpdatedBy { get; set; }
}

The idea was that by rolling some common audit properties into a base entity class we could keep everything consistent and make auditing at the database level a lot simpler.  For example, if I wanted to make the Person entity auditable, all I would have to is make sure that the Person database table was set up correctly and that my entity class inherits from AuditableEntity:

public class Person : AuditableEntity
{
  [Column]
  public string FirstName { get; set; }

  [Column]
  public string LastName { get; set; }
}

When the Person entity is persisted to the database, a low-level repository class would recognize that the class inherits from AuditableEntity and fill in the appropriate audit information.

I dig the idea of building in some automatic auditing functionality, but, unfortunately, the OR/M tool we were using, LINQ-to-SQL, isn’t too keen on column-mapped properties defined within the base class of an entity.  As a matter of fact, it can’t see them at all.

To get this to work, we would have to pull the auditing stuff back up to the actual Person entity class.  We still wanted to automatically plug in the audit information so I thought that we could just define the properties in an interface that the Person entity could implement; I was thinking something simple like IAuditableEntity.  After a brief conversation with the developer that originally built all of the auditing stuff he agreed to make the changes so we could get back down to business.

The next day, I opened the project back up again to see what he had come up with.  I was surprised to see not only that the base entity class still existed, but it had been updated:

public abstract class AuditableEntity
{
  [Column]
  public virtual DateTime DateCreated { get; set; }

  [Column]
  public virtual DateTime DateLastUpdated { get; set; }

  [Column]
  public virtual string CreatedBy { get; set; }

  [Column]
  public virtual string LastUpdatedBy { get; set; }
}

I was even more surprised to find the updated Person entity class:

public class Person : AuditableEntity
{
  [Column]
  public string FirstName { get; set; }

  [Column]
  public string LastName { get; set; }

  [Column]
  public override DateTime DateCreated { get; set; }

  [Column]
  public override DateTime DateLastUpdated { get; set; }

  [Column]
  public override string CreatedBy { get; set; }

  [Column]
  public override string LastUpdatedBy { get; set; }
}

Alright, folks.  If you’ve ever interviewed for a C# position, you’ve probably been asked the difference between an abstract class and an interface.  In this case, the AuditableClass has been effectively rendered useless.  LINQ-to-SQL won’t be able to see the virtual properties so you have to override them in the Person entity class.  Even if you were going to go this route, which you absolutely should not, you should at least mark the properties abstract so that you’re forced to override them by the compiler.  If you forgot to override any one of the properties, chances are your application would choke when it tried to persist to the database.  Again, you really shouldn’t go this route.

The right answer here is to create the IAuditableEntity interface and have the Person class implement it.  Sure, it’s a little more work since you have to define the audit properties for each entity class, but, until LINQ-to-SQL learns to dig down into a base class it’s your only option.  You could always use stored procedures or default column values at the database level, but we won’t get into that now.

Interfaces are good.  Trust me.  If you’re not defining any actual, concrete base functionality you don’t need a base class.