Practical Example of YAGNI: Good Intentions Gone Horribly Wrong

YAGNI, "you aren't gonna need it" (should maybe have a sub-principal of "will prolly come back around to bite you in the ass later"), is a principal that gets tossed around a lot to keep gold plating developers like me in check. I just love traveling down bunny trails adding "wouldn't it be cool if..." features to the code I am working on. I recognize this this fault in myself and routinely stop to ask myself, "why am I doing this?" or "how does this relate to the task at hand?" maybe even a "what effects could this have in other areas of the system?" on occasions where I am feeling extra saucy.

I wish someone had asked those questions of the person who wrote this code that I discovered deep in the bowls of "the code base that must not be named".

public void CleanData(DataRow row, int upperLimit)
    {
      if ((row == null))
      {
return;
      }

      //Look at each field in this row 
      for (int i = 0; i <= upperLimit; i++)
      {
//If this field is null, change the value of the field to some type of empty value 
if ((row.ItemArray.GetValue(i)) == DBNull.Value)
{
  // Handle all possible types of data in a datatable 
  switch (row.Table.Columns[i].DataType.Name)
  {
    case "Boolean":
      row[i] = false;
      break;

    case "Byte":
      row[i] = (byte) 0;
      break;

    case "Byte[]":
      row[i] = 0;
      break;

    case "DateTime":
      row[i] = DateTime.MinValue;
      break;

    case "Decimal":
      row[i] = (decimal) 0;
      break;

    case "Double":
      row[i] = (double) 0;
      break;

    case "Int16":
    case "Int32":
    case "Int64":
      row[i] = Conversions.ToInteger(0);
      break;

    case "Long":
      row[i] = Conversions.ToLong(0);
      break;

    case "Short":
      row[i] = (short) 0;
      break;

    case "Single":
      row[i] = (float) 0;
      break;

    case "String":
      row[i] = "";
      break;

      //If a type shows up that is not handled, throw a meaningful exception 
    default:
      throw new ArgumentException(string.Format(CultureInfo.InvariantCulture,
                                                Resources.InvalidConversionDataType,
                                                row.Table.Columns[i].DataType.Name));
  }
}

Now at first glance, this doesn't seem that bad. A very nice example of gold plating. I can hear the developer now saying to him or herself, "wouldn't it be cool if we never had to check for DBNull? Seriously that would be jalapeno hot!"

I wonder if the developer was thinking about this possible unintended consequence that I am currently working to resolve.

So, now I have nonsense dates being rendered out to my page. Trying to address this issue at the source, I commented out the line that sets dates to a min value in the clean data method to see what happens.

Well that is interesting. To make it perfectly obvious _StaffMember is a hashtable. And the value of it's Staff_Lockout_Datetime key is and empty string. And this error is being caused by me switching a low level data clean up method to not change a null value to a invalid date.

So how is this hashtable created? Once again, deep in the bowls of the code base is a method called ToHashtable that takes a datarow and returns a hashtable. The interesting block of code is below.

For i As Integer = 0 To tableRow.Table.Columns.Count - 1

      If tableRow.IsNull(i) Then

        'Convert nulls to 0 or " before putting in hashtable
        Select Case tableRow.Table.Columns(i).DataType.Name

          Case "Int64", "Int32", "Int16", "Decimal", "Double", "Single"

            'Numeric data types
            If convertDBNullIntToZero Then
              ht.Add(tableRow.Table.Columns(i).ColumnName, 0)
            Else
              ht.Add(tableRow.Table.Columns(i).ColumnName, ")
            End If

          Case Else

            'String, DateTime data types
            ht.Add(tableRow.Table.Columns(i).ColumnName, ")

        End Select

      Else
        'blah blah blah
    End If
Next

So once again we have some gold plating here attempting to prevent other developers from having to check for null values. The interesting thing is why are we checking for nulls on a row that has already been though the clean process when the dataset was retrieved from the database? It seems to me this block of code has never been hit before today. Maybe our gold plate-ers are not comparing notes on all the filigree they are adorning our objects with.

The icing to this cake is the line of code where it all blows up. Look at it. What are they really trying to do?

If CDate(_StaffMember("Staff_Lockout_Datetime")) <> CDate(Nothing) Then
All they really want to know is if the hash table value is null. Exactly what our gold plate-ers have so desperately been trying to hide from us.

So I am left with two choices:

1. Attempt to locate all the areas in the entire application where this gold plating has caused assumptions and correct them to let null dates make their way out the UI and hope like hell I don't introduce more bizarre side effects.
2. Come up with some goofy databinding or prerender hack to work around the invalid dates.
   Both options leave a bad taste in my mouth and a strong desire to drown my sorrows with a nice Irish whiskey.