write(this); //not that

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.

From my latest visit to the coding freak show, I bring you this one-liner:

private int socialSecurityNumber;

Really? At very least, this should be a String:

private String socialSecurityNumber;

But even better, perhaps a domain object is in order that will ensure the correct formatting of the SSN. A simple implementation could wrap a String and employ proper formatting…and would probably suffice for most applications.

A smarter implementation may actually have 3 fields (area, group, and serial) with proper validation and fromString()/toString() implementations…but the field level of detail is probably superfluous for many applications.

Regardless of how you choose to represent a SSN, one thing I know for sure…it’s not an integer. Or a float. Or a double. Or a boolean. Or a…

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.

What up, everybody?

You may have noticed a row of stars at the top and bottom of each post.  You can use these stars to rate content submitted by other users.  Before you ask, no, you can’t rate your own posts and no, you can’t vote for the same post twice.  Nice try.

In the next couple of months we’re going to be rolling out a contest based on these ratings.  The highest rated user at the end of each month will have their name posted prominently on the site and will get some cool swag, courtesy of WriteThisNotThat.com.  For right now I think we’ll probably be doing t-shirts but we are in the process of courting some sponsors so we can toss in some books, development tools, etc.

Next time that you find some particularly fugly source code, share it with us!  There could be a little something in it for you.

Remember, in order to post, you’ll need to register first.  Don’t worry, we promise not to share your information with anyone.  If you don’t believe me, take a look at our privacy policy.

As always, if you like this site, please don’t keep it a secret!

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.