Inner Platform Effect - SQL Utilities Edition

chubertdev

Here's the WTF that I've been having fun with recently. I didn't really go into as much detail as I wanted to, but you'll get the idea.

We have a C# .NET framework/library DLL at my company that we use for all of our projects. It's pretty nice to have all of this code in one place...kinda. We have two different versions for .NET 2.0 and 4.0 (which isn't too bad). However, all of what we do is separate apps. Someone, well before I got here, decided that it was a good idea to use the page-as-an-app methodology, with a few dozen apps. Then, this DLL is built, and placed in each project (not the GAC, something that I'm fixing).

The library itself is full of WTFs, one of which has already made it on the front page (first one):

Remy Porter / May 1, 2014

The Long Way

Sometimes, a developer just needs to take the long way around. Sure, a line of code like DateTime StartTime = DateTime.Now looks simple and readable, but what happens if you want the StartTime variable to be not exactly now? Craig’s co-worker figured out a better solution:

I'm going to concentrate on the class that is used for SQL utilities, as this is the one that exhibits the Inner Platform Effect. I'll refer to the library's namespace as OurLib from here on out, and the class as SQLUtilities.

The class has a wonderful constructor:

        /// <summary>
        /// Constructor
        /// </summary>
        public SQLUtilities()
        {
        }

There are these handy enumerations, which I assume that the author read about one time and thought would be useful:

        public enum ProcExecutionType { DataTable = 1, Scalar, NonQuery }
        public enum SQLParamType { String = 1, UnicodeString, Int, Bit, Date, Image, Decimal, Table, Return }
        public enum OptionalParam { CommandTimeout = 1, ReturnExceptions, ReturnDataSet }

OptionalParam should be a list or class instance or dictionary or anything besides an enumeration, since their usage is so different. Each faux-"property" is used independently. For example, here's CommandTimeout:

        cmd.CommandTimeout = timeout >= 1 ? timeout : 30; //If value passed is negative or empty string, keep timeout as default 30 sec

Logic that could go in the constructor, but.......well, I don't know where to go with this one:

        public static List<SqlParameter> InstantiateParameterList()
        {
            return new List<SqlParameter>();
        }

And instead of having the dev using this libary use the SqlParameter constructor, there are nanny methods that replicate the constructors with less functionality:

        /// <summary>
        /// Adds a string parameter to "cmd"
        /// </summary>
        /// <param name="cmd">SQL Command object</param>
        /// <param name="ParamName">The name of the parameter</param>
        /// <param name="ParamValue">The parameter's value</param>
        /// <param name="nLen">The length of the string</param>
        /// <param name="Unicode">Optional defaults to false.  Use true for unicode strings</param>
        static public void AddStringParam(SqlCommand cmd, string ParamName, string ParamValue, int nLen, bool Unicode = false) //always string...
        {
            if (ParamValue.Length > nLen)
            {
                ParamValue = ParamValue.Substring(0, nLen);
            }

            SqlParameter p = new SqlParameter(ParamName, (Unicode ? SqlDbType.NVarChar : SqlDbType.VarChar), nLen);
            p.Direction = ParameterDirection.Input;
            p.Value = ParamValue;
            cmd.Parameters.Add(p);
        }

And you see the same code, repeated multiple times, with the only main change being the SqlDbType. So you have:

        AddStringParam // (no nLen)
        AddIntParam
        AddBitParam
        AddDateParam
        AddImageParam
        AddDecimalParam
        AddStatusReturnParam
        AddReturnParam
        AddTableParam // (SqlDbType.Structured, can't specify the type)

Not only are these methods all public, but there's another public method that simply perfoms a switch statement on the Type (SQLParamType) value, casts the ParamValue (object), and adds it the Parameters (List<SqlParameter>) object. The best part is, it doesn't have case statements for all SQLParamType values, and since it creates a new SqlParameter and adds it outside of the switch statement, it could add one with a blank value (not sure what the SqlDbType would be).

        public static void BuildParam(List<SqlParameter> Parameters, SQLParamType Type, string ParamName, object ParamValue)

The main offender, the method that I can only call the "God method":

        public static object ExecuteStoredProcedure(string ConnectionString, string ProcName, List<SqlParameter> Parameters, ProcExecutionType ProcType, ref string ReturnParam, Dictionary<OptionalParam, object> Options)

One method to rule them all. I like that it handles the database connection internally with a disconnected recordset. But "ProcType" is the Inner Platform Effect again, replacing ExecuteNonQuery, ExecuteScalar, and using a DataSet. Not only that, but the "ReturnDataSet" value of Options also contributes towards this.

The "ReturnParam" argument is a symptom of not being about to set up the Direction on parameters correctly. It wouldn't even be necessary if this class left the Parameter setup to the developer, but it has to try to do everything for you. I guess that one would never need multiple output parameters.

I'm surprised that the return type of this method is determined by an argument passed in, but simply is an object returned from a switch statement based on ProcType value. So you'll end up seeing code like this:

        if(!(dt is DataTable)) { ... }

If an exception occurs, it's logged, swallowed, and null is returned.

I'm just glad that the current version (after checking with Subversion, previous versions had this issue) doesn't leave handling the database connection up to the developer.

chubertdev

Edited for grammar/readability.

Captain

This is how we'd do it in an embedded system.

DrakeSmith

This looks like it was anonymized from the codebase I am working on... Looks exactly the same.

chubertdev

@DrakeSmith said:

This looks like it was anonymized from the codebase I am working on... Looks exactly the same.

Probably copypasta'd from the same site many years ago.

Nagesh

No need to do that stuff. It is not like you are building the Char Minar or Qut`b Minar.