Some (More) AntiPatterns At My Job



  • 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;
        }
    }
    
    


  • Hmmm. Why isn't the code highlighting working?



  • Triple-tick also goes at the bottom of the code block, and it's cs at the top, not c#. You can also create a code block by a 4-or-more-space indent, but those aren't highlighted.

    Welcome to DiscoBBMarkHTCodeDownMLWTF.



  • @TwelveBaud said:

    Triple-tick also goes at the bottom of the code block, and it's cs at the top, not c#. You can also create a code block by a 4-or-more-space indent, but those aren't highlighted.

    Welcome to DiscoBBMarkHTCodeDownMLWTF.

    So your first example should look like:

    ```cs
    command.Parameters.AddWithValue("Origin", DataAccessHelper.DataCleanup<string>(rate.Origin));
    ```
    

    To get:

    command.Parameters.AddWithValue("Origin", DataAccessHelper.DataCleanup<string>(rate.Origin));
    


  • Thanks.



  • That's impressive. Was all that bullshit necessary for vb6? I'm fortunate enough to not have much exposure to it.



  • Was doing things like passing all sql parameters in as strings necessary in VB6? Maybe. I'm not sure either. The DataCleanup method isn't VB-like at all, though. It's just.... bad.


  • Notification Spam Recipient

    @Pharylon said:

    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.

    Sounds familiar... did they come up with a brilliant idea to put
    using Microsoft.VisualBasic;
    everywhere, to 'feel more at home, develop faster and make the code more readable'?


  • Discourse touched me in a no-no place

    Looks to me like the guy who wrote it didn't fully understand it, so they invoked all those magic commands because that's what it takes to make magicThing happen.

    I've seen placement students churn out much better code, hell I've seen high school kids do better.

    I really enjoyed your commentary within the code. Usually people just paste in massive code blocks for you to wade through. The comments made it much more entertaining.

    I'd consider committing your comments into the operations source control ;-)


    Filed under: ...because Voodoo


  • No, that would be a silly syntax error! Here, let me fix that...

    Imports Microsoft.VisualBasic
    

    There, much better!



  • @Pharylon said:

    ```
    //Aaaaand the try/catch ends with just re-throwing.

    
    I can forgive this, I've done it to set a breakpoint there.  At least he didn't trash the stack trace.
    
    As for the rest of the code:
    
    @Pharylon <a href="/t/via-quote/54528/1">said</a>:<blockquote>```
    //Truly, this is madness.
    ```</blockquote>
    
    Indeed.


  • @Pharylon said:

    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.


    You in the audience might laugh about C# being written as if it is VB6 with weird syntax. Come to think of it, it sounds like @Pharylon is poking fun.

    I once worked in a place that had an unholy mix of Fortran and C, with a sprinkling of C++ and JavaScript (with GTK bindings, no less, for UI management), and one mad corner that used Java. They were trying valiantly to rid themselves of the Fortran code, but this process was hindered mightily by the tendency of the old-guard programmers to imitate one of the classic jokes about Real Programmers(TM) : a Real Programmer can write Fortran in any language.

    It was deeply scary, some of the code you found when you went digging. It really was exactly as if they thought that the C compiler was just a Fortran compiler that used a strange input syntax, so they wrote string manipulation routines that were 100% C language but ignored decades of how-to guides for strings in C, and treated them exactly as if they were Fortran strings in situations where there was no need to do so. A Fortran string variable is just an array of characters, but there is no magic character that marks the end of a Fortran string. So they were manipulating C strings without NUL terminators.


  • FoxDev

    @NedFodder said:

    At least he didn't trash the stack trace.

    try {
        [...]
    } catch (Exception up) {
        [...]
        throw up; // NO! NONONONONONONO! YOU ARE DESTROYING THE STACK TRACE! EITHER WRAP THE EXCEPTION IF YOU WANT TO MAKE YOUR PURILE JOKE OR JUST USE THE UNADORNED `throw`!!!!! BECAUSE I WILL CUT YOU IF YOU DO THIS TO ME AND I HAVE TO DEBUG WHAT YOU HAVE PAINSTAKINGLY GIVEN TO ME WITHOUT A PROPER STACK TRACE! GAH!
    }
    

    💢

    one of my biggest pet peeves that....


  • Discourse touched me in a no-no place

    @Steve_The_Cynic said:

    So they were manipulating C strings without NUL terminators.jdk^CjfdLk jk3s d#f67howe%^U r89nvy~~owmc63^Dz x.xvcu


    Filed under: FTFY


  • You could have just fixed it instead of adding the all-caps comment.

    Call me crazy I guess.


  • FoxDev

    @blakeyrat said:

    You could have just fixed it instead of adding the all-caps comment.

    yes, but if i did then my comment wouldn't have made much sense now would it?

    OF COURSE I MADE THE DEVELOPER WHO WROTE IT FIX IT!

    :ANGER:



  • @DoctorJones said:

    @Steve_The_Cynic said:
    So they were manipulating C strings without NUL terminators.jdk^CjfdLk jk3s d#f67howe%^U r89nvy~~owmc63^Dz x.xvcu


    Filed under: FTFY

    Indeed, although because they were doing it in a Fortran way (Fortran using weird syntax, remember), they didn't have that particular problem. Nevertheless, it gave me the willies every time I saw any of it.

    But that was far from the worst thing I saw there. There was, for example, a 1500-line Fortran function of which approximately 90% (measured by LOC count) was conditional on who called it, but in many, many different small if statements. And there was the notorious "evildead.c", all of whose variables and internal functions were named after characters or lines in the film suggested by the filename. And a C file somewhere which attracted complaints about it being hard to edit because of the exclusive checkout policy, where the complainers didn't take into account that the file was in excess of 1.6 MB, and drove a frequently-changed screen, so there were always four or five developers trying to modify it.

    In case you were wondering, evildead.c contained production code.



  • I don't feel we can blame this all on VB6 exposure. I'm sure some people wrote good VB6 code... somewhere.

    Also, the generics there at the end. Seriously... not a feature of VB6. It's just someone learned about generics and decided they'd found a super awesome use-case. 😱



  • By the way, I just checked. The DataCleanup() method is called 18,098 times in our codebase. It's not some cherry-picked monstrosity form the bowels of the codebase no one ever uses. It's a core function that's used a lot.



  • @Pharylon said:

    I'm sure some people wrote good VB6 code... somewhere

    It's hard. You can easily write good VB.Net, but classic VB is missing a lot of what modern languages take for granted. The good VB6 programmers immediately recognized the value of most of the new stuff and started using it immediately. The bad VB6 programmers tried to write VB6 in VB.Net and complained about everything that changed.

    My favorite example is when I had to add some features to a VB6 app that was scheduled to be upgraded to VB.Net in a few months. I structured the methods the way they would be in VB.Net - and it took some extra boilerplate code in VB6 to get it done. The plan was for the people doing the upgrade (it was all done by hand, not a tool) to replace the slightly cumbersome methods with the obvious VB.Net equivalent. Instead, they added more boilerplate to the existing boilerplate.



  • @Jaime said:

    The bad VB6 programmers tried to write VB6 in VB.Net and complained about how everything has changed.

    Where are my line numbers, goddammit!



  • @Pharylon said:

    ```cs
    else if (obj.GetType() == typeof(DateTime?))

    Wow, someone should remind them that *nullables are not boxed as nullables, but as their underlying type*.
    
    @Pharylon <a href="/t/via-quote/54528/1">said</a>:<blockquote>```cs
        //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.
    ```</blockquote>
    Wait, what? If you supplied `"foo"` for the `defaultValue`, you won't be in this code path at all since the test on `objectType` is based on `defaultValue`'s type...
    
    @Pharylon <a href="/t/via-quote/54528/1">said</a>:<blockquote>```cs
        //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.
    ```</blockquote>
    Given how fucked-up the start of the method is (what with trying to detect `Nullable` from a boxed value), I'm not as certain as you are.


  • @Pharylon said:

    The DataCleanup() method is called 18,098 times in our codebase

    In terms of functionality, it certainly doesn't appear to do very much of any use, beyond heating the server room just that little bit more. Overall it's probably not much of an overhead, especially as it comes before / after database access, which should be several orders of magnitude more significant.

    That said, it definitely instills a desire to pull out the cluebat and apply it to those reponsible, finger by finger, until they can't physically use a keyboard any more. The generics stuff is particularly splendid.


Log in to reply