Separate SQL strings from code

Question

I work on a ancient large WinForms project that contains a lot of win form libraries.

Each form uses hard-coded sql commands in methods. Sometimes the same SQL strings are duplicated, and modifying one I should research "similar" strings in the form's code.

I understand that the architecture is not super-pretty, but at moment we can't perform large architecture modifications. I think to do "little steps" to ameliorate the separation o the UI and BD logic....

A first 'step', by eg. I thought "separate" in a way sql strings from code. How could this be achieved, using Resources, a special class, a XML file?

Variant I - using named Resources

' ancient variant '
_SqlPeriod = String.Format("SELECT * FROM DBO.GP_PERIOD WHERE PERIOD = {0} ORDER BY LABEL", Me._Period)

' a better way (?) ... '
_SqlPeriod = String.Format(My.Resources.ResourceManager.GetString("SelectAPeriod"), Me._Period)

Answer 1

This is what your .NET code could look if you were to rewrite the whole thing into stored procedures:

 public List<User> listUsers(Guid pPersonTypeId, string pFirstName, string pLastName)
        {
            List<User> list = new List<User>();
            List<SqlParameter> pa = new List<SqlParameter>();
                       pa.Add(SqlHelper.createSqlParameter("@ID_PERSONTYPE", SqlDbType.UniqueIdentifier, pPersonTypeId != Guid.Empty ? (Guid?)pPersonTypeId : null));
            pa.Add(SqlHelper.createSqlParameter("@FNAME", SqlDbType.NVarChar, String.IsNullOrEmpty(pFirstName) ? null : pFirstName));
            pa.Add(SqlHelper.createSqlParameter("@LNAME", SqlDbType.NVarChar, String.IsNullOrEmpty(pLastName) ? null : pLastName));

            try
            {
                DataSet ds = SqlHelper.ExecuteDataset("proc_USER_list", pa.ToArray());
                if (ds.Tables.Count == 0)
                    return list;
                foreach (DataRow r in ds.Tables[0].Rows)
                {
                    User u = new User();
                    //populate your User object with data from the DataRow
                    ...
                    list.Add(u);
                }
                return list;
            }
            catch (Exception ex)
            {
                throw (new BaseException(ex));
            }
        }

You would of course need to implement the SQLHelper and SQLParameter classes, which is fairly simple.

if this is not a way you want to go right now, I suggest something like creating an XML document (one per data access class), storing your queries in it, and writing a very simple wrapper class to fetch your queries based on a query code or ID. The XML could look like this for example:

<query code="listUsers"> select * from USER where NAME = {0} </query>

To go even further, I guess you could use the method name as a query code and even have the wrapper use reflection to see the method it's being called from and use the name of the method to look for your query in the XML.

Using resource classes to store your queries is even simpler, but I'm not sure there are any benefits at all to be had there, with the XML at least you get SOME separation.

Answer 2

Generally, your biggest concern is using String.Format to form your SQL queries, because this is prone to Sql Injection Attacks, if you don't explicitly validate your parameters. So separating these strings is not an improvement (and moving them into an xml file would be a security disaster, if they are resolved during runtime).

To prevent this, you need to either create SqlCommand instances and set parameters programatically (check MSDN - How To: Protect From SQL Injection in ASP.NET), or use a completely different approach and switch to an ORM framework.

Since moving to ORM might take considerable amount of time if you haven't used it before, I believe you will opt for the first option. In that case, simply switching to Sql parameters as described above will be an improvement (even with sql strings left hard-coded).

To prevent duplication, you should consider moving all database access calls into a separate project (a Data Access Layer, DAL). If you want to start in smaller steps, you might consider (for start) delegating the creation of the SQL string to a different layer, e.g.:

_SqlPeriod = QueryStrings.GetPeriodsById(Me._Period) ' <- returns an sql string

This would allow you to move parameter validation into a common project. Later, you will want to avoid passing sql strings to your UI layer completely, and simply get the data:

_ActualData = Dal.GetPeriodsById(Me._Period) ' <- gets the actual list of periods

Answer 3

I would go for a class. At first you just replace literal sql statements with getter calls to centralize the SQL access. Then you can use the class' init code to fine tune the statements where appropriate (local pathes, dates, users, other environment factors). Later you can refactor the getters to return prepared statements on a second call or do other fancy things.

A resource would give you just a list of strings; an .xml file would need a lot of structural planning and supporting code before you can exploit XML features.

Answer 4

I would use stored procedures instead of having SQL in code, XML or resource.

You can have a look at this for some pros and cons of using SP. What are the pros and cons to keeping SQL in Stored Procs versus Code

Actually, in this thread the guys against SP has the most votes but don't let that put you off :).

One of the biggest advantages that I see is that you can change and test SP without changes the client code as long as you work with the same parameters and your datasets contain the same columns. Another is that your queries are checked against dependencies when you create them and you can fairly easy find out if a table or column is used by some SP before you change the table structure. Not that easy when the query is stored away in a resource or embedded in code