At my current employer, I'm surrounded by old-timers who don't want to change their ways. It's not an exaggeration to say about a third of my company got their start on punch cards.
For a long time, everything here was in VB6 before eventually being rewritten in C# (they're just finishing up the conversion this year!). But a lot of code is still very.... VBish. But mostly just bad.
I don't deal with most of that, thouigh. I do the web development, and get to use pretty much whatever tools I want (though I keep the backend in C#, because honestly I love the language). For a while it was myself and a long term contractor, but he left a few months ago. As a Big Project had just completed, they decided not to replace him, but instead I get to cross-train someone from the "Operational Package" team. I got my hands on the one young guy over there (this is his first programming job), as I view him as... salvageable. He's picked up some bad habits from them, but he seems smart enough. We're doing code reviews (something the operational package team doesn't do), in part to try and correct some issues.
So, I get my first pull request form him today, and look through the code to find this gem. He was calling this "DataCleanup()" mehod constantly. He copy/pasted this from the Operational Package code, where it's called on every sql parameter before it goes in, and it's called on every field that comes out before it's hydrated in an object. For instance:
command.Parameters.AddWithValue("Origin", DataAccessHelper.DataCleanup<string>(rate.Origin));
or:
result.Origin = DataAccessHelper.DataCleanup<string>(_reader["Origin"]);
result.TransitDays = DataAccessHelper.DataCleanup<int>(_reader["TransitDays"]);
result.IsHazardous = DataAccessHelper.DataCleanup<bool>(_reader["IsHazardous"]);
So, what does this magic DataCleanup function do? Well, let's take a stroll through the DataAccessHelper class! All the comments here are mine.
//Note that all the methods are static, but the class itself isn't.
public class DataAccessHelper
{
private static readonly string DateFormatShort = "MM/dd/yyyy";
private static readonly string DateFormatLong = "MM/dd/yyyy hh:mm:ss.fff tt";
private static readonly DateTime MinimumDate = new DateTime(1900, 1, 1);
private static readonly CultureInfo UserCultureInfo = new CultureInfo("en-US");
//This method is used for turning decimals into strings before we
//use them as sql parameters because reasons.
public static string ParseDecimalToDatabaseFormatParameter(decimal value, string defaultValue = "0")
{
string tempValue = string.Empty;
decimal resultingValue;
//We are literally calling toString() on a decimal and then parsing it back into a decimal again.
bool isValid = decimal.TryParse(value.ToString(), out resultingValue);
if (isValid)
{
//After all that, if it's a 0, we return the "defaltValue", which unless
//an optional parameter supplied will just be "0". They could literally have
//just called myDecimal.ToString() in most cases.
tempValue = (resultingValue == 0) ? defaultValue : resultingValue.ToString();
}
//This is, obviously, impossible. isValid will always be true.
else
{
tempValue = defaultValue;
}
return tempValue;
}
//Sniping the methods 'ParseInt16ToDatabaseFormatParameter', 'ParseInt32ToDatabaseFormatParameter',
//and ParseDateTimeEndToDatabaseFormatParameter... I'm sure you can guess what they look like
//(hint, they all call ToString() on the supplied value and parse it back).
public static string ParseDateTimeEndToDatabaseFormatParameter(DateTime? value)
{
string tempValue = string.Empty;
if (IsDate(value))
{
//The next 3 lines of comments aren't mine.
// This did not work. The value was set to SQL as '2012-01-29 23:59:59.900 P1'
//tempValue = value.Value.ToString("yyyy-MM-dd 23:59:59.900 PM");
//DateTime lDateTime = new DateTime(value.Value.Year, value.Value.Month, value.Value.Day, 23, 59, 59, 990);
DateTime lDateTime = new DateTime(value.Value.Year, value.Value.Month, value.Value.Day, 0, 0, 0, 0);
lDateTime = lDateTime.AddDays(1);
tempValue = lDateTime.ToString(DateFormatLong);
}
//Yup, this is still impossible.
else
{
tempValue = null;
}
return tempValue;
}
//What the fuck is this? Why is 1/1/1900 acceptable and 1/2/1900 isn't???
//Why is ToString() being called on the DateTiems for comparison???
public static string ParseDateTimeToDatabaseFormatParameter2(DateTime? value)
{
string tempValue = (value != null) ? value.ToString() : "01/02/1900";
DateTime mydatetime;
DateTime.TryParse(tempValue, out mydatetime);
if (mydatetime.ToString("MM/dd/yyyy") == "01/02/1900" || mydatetime.ToString("MM/dd/yyyy") == "01/01/0001")
{
tempValue = null;
}
else
{
tempValue = mydatetime.ToString(DateFormatLong);
}
return tempValue;
}
//String is trimed twice (once in that horrible double ternary at the top and again at the bottom).
//Also, if removeCharacter is false, why do the foreach loop at all?
public static string StringCleanup(string value, bool removeCharacters = false, bool trim = false)
{
//Seriuosly, what is it with my job and the weird Hungarian Notation?
string lStringValue = (value != null) ? (trim) ? value.Trim() : value : string.Empty;
StringBuilder newString = new StringBuilder();
foreach (char lChar in lStringValue.ToCharArray())
{
switch (lChar)
{
case '\t':
case '\r':
case '\n':
if (!removeCharacters)
{
newString.Append(" ");
}
break;
default:
newString.Append(lChar);
break;
}
}
return (trim) ? newString.ToString().Trim() : newString.ToString();
}
//This method determines if an object is either a DateTime or a string that
//represents one. Because, you know, this is the best way to determine that.
public static bool IsDate(object obj)
{
DateTime lDateTime;
if (obj == null)
{
return false;
}
else if (obj.GetType() == typeof(DateTime))
{
//Gotcha! Despite the name of the method, in additon to checking if it's a DateTime,
//it also does validation to make sure it's greater than the TSQL default and less
//than the .NET maximum.
lDateTime = (DateTime)obj;
if (lDateTime > MinimumDate && lDateTime < DateTime.MaxValue)
{
return true;
}
return false;
}
else if (obj.GetType() == typeof(DateTime?))
{
DateTime? nullableDate = obj as DateTime?;
//Of course, we already checked if the object was null, but can't hurt to check again!
if (nullableDate != null && nullableDate > MinimumDate && nullableDate < DateTime.MaxValue)
{
return true;
}
return false;
}
string strDate = obj.ToString();
//I don't even want to know what kind of hell on earch requires this kind of parsing logic.
//I'll just leave this here.
try
{
string[] stringSplit = strDate.Split('/');
if (stringSplit.Length == 3 && stringSplit[2].Length >= 4)
{
string[] stringSplitYearAndTimePart = stringSplit[2].Split(new char[] { ' ', ':', '.' });
if (stringSplitYearAndTimePart.Length > 0)
{
int month, day, year;
if (int.TryParse(stringSplit[0], out month) == true && int.TryParse(stringSplit[1], out day) == true && int.TryParse(stringSplitYearAndTimePart[0], out year) == true)
{
if (month > 0 && month < 13 && year >= MinimumDate.Year && year <= DateTime.MaxValue.Year)
{
if (DateTime.TryParseExact(strDate, DateFormatLong, Thread.CurrentThread.CurrentCulture, DateTimeStyles.AllowWhiteSpaces, out lDateTime) && lDateTime > MinimumDate && lDateTime < DateTime.MaxValue)
{
return true;
}
else if (DateTime.TryParseExact(strDate, DateFormatShort, Thread.CurrentThread.CurrentCulture, DateTimeStyles.AllowWhiteSpaces, out lDateTime) && lDateTime > MinimumDate && lDateTime < DateTime.MaxValue)
{
return true;
}
else if (DateTime.TryParse(strDate, out lDateTime) && lDateTime > MinimumDate && lDateTime < DateTime.MaxValue)
{
return true;
}
}
}
}
}
return false;
}
finally
{
//Gotta love the try/finally with an empty finally.
}
}
//Now for the Piece de Resistance. This is the method I mentioned at the top that gets
//called on everything going in or out of SQL in the Operational Package
//This will check of the object is "valid" and if not return the defaultValue.
public static T DataCleanup<T>(object value, T defaultValue = default(T))
{
Type objectType = typeof(T);
bool isNullableType = Nullable.GetUnderlyingType(objectType) != null;
objectType = Nullable.GetUnderlyingType(objectType) ?? objectType;
T returnValue = defaultValue;
try
{
if (isNullableType && (value == null || DBNull.Value.Equals(value)))
{
returnValue = defaultValue;
}
else if (objectType == typeof(String))
{
//Note that even if you passed null in as the defaultValue, this method's opinion is
//you can suck it and take an empty string. Apparently they almost never put nulls
//into the database. It's always empty strings.
string lString = (value != null) ? value as string : (defaultValue != null) ? defaultValue as string : string.Empty;
returnValue = (T)System.Convert.ChangeType(StringCleanup(lString, true, true), typeof(string), UserCultureInfo);
}
else if (objectType == typeof(DateTime))
{
DateTime myDateTime;
//Somehow, this line is actually necessary. But only
//becuase the rest of this mehtod is so fucked up.
DateTime lDefaultDateTime = (!defaultValue.Equals(default(T))) ? (DateTime)System.Convert.ChangeType(defaultValue, typeof(DateTime), UserCultureInfo) : MinimumDate;
if (value == null)
{
//Here, if you suplied "foo" for the defaultValue, but if the value object
//was a DateTime, a method meant to 'clean up' the Data will explode.
returnValue = (T)System.Convert.ChangeType(lDefaultDateTime, typeof(DateTime), UserCultureInfo);
}
else if (IsDate(value) == true)
{
if (value.GetType() == typeof(DateTime))
{
//On this line, we (A) call ToString() on a DateTime, (B)parse that string
//back into a DateTime, then we(C) run that DateTime through Convert.ChangeType
//to change a DateTime into....a DateTime???? Am I missing something here????????
returnValue = (DateTime.TryParse(value.ToString(), out myDateTime) && myDateTime > lDefaultDateTime) ? (T)System.Convert.ChangeType((DateTime)value, typeof(DateTime), UserCultureInfo) : (T)System.Convert.ChangeType(lDefaultDateTime, typeof(DateTime), UserCultureInfo);
}
else
{
//Have they ever heard of Convert.ToDateTime()????
returnValue = (DateTime.TryParse(value.ToString(), out myDateTime) && myDateTime > lDefaultDateTime) ? (T)System.Convert.ChangeType(myDateTime, typeof(DateTime), UserCultureInfo) : (T)System.Convert.ChangeType(lDefaultDateTime, typeof(DateTime), UserCultureInfo);
}
}
else
{
//This ternary should always evaluate false. The IsDate() method above
//already determined it couldn't be parsed into a DateTime, right?
//Although, admitedly, IsDate() is so fucked up it could well have failed, so hey.
//Truly, this is madness.
returnValue = (DateTime.TryParse(value.ToString(), out myDateTime) && myDateTime > lDefaultDateTime) ? (T)System.Convert.ChangeType((DateTime)value, typeof(DateTime), UserCultureInfo) : (T)System.Convert.ChangeType(lDefaultDateTime, typeof(DateTime), UserCultureInfo);
}
}
else if (objectType.IsEnum && Enum.IsDefined(objectType, value))
{
//Enums can't be null! What the fuck is wrong with these people?
//Plus, if it was null, it'd have been caught earlier in the method.
return (value != null) ? (T)value : defaultValue;
}
else
{
returnValue = (value != null && !DBNull.Value.Equals(value) && value.ToString().Length > 0) ? (T)System.Convert.ChangeType(value, objectType, UserCultureInfo) : (!defaultValue.Equals(default(T))) ? defaultValue : default(T);
}
}
catch (Exception)
{
//Aaaaand the try/catch ends with just re-throwing.
throw;
}
//Overall, why hy have one long method for multiple types. Why not a "StringCleanup()"
//and a separate "DateTimeClenup()"? It's like someone discovered generics and was all
//like "I know just what to do with these!!! Wheeeee!!!!"
return returnValue;
}
}