Fetch data from database either with a stored procedure or a query

Question

I have this method, GetDataAsync(), that grabs data from the database using a stored procedure if sqlToRun Dictionary contains the table name. If not, then there is no stored procedure to run and just grab the table from the db directly (no SP) and convert it to a C# object to use.

Here's the code:

private async Task<Response> GetDataAsync(string table, int? start, int? end)
{
    if (_sqlToRun.ContainsKey(table))
    {
        var sql = _sqlToRun[table];
        var data = new List<Dictionary<string, object>>();
        var resp = new Response();

        using (var conn = new SqlConnection(_ConnectionString))
        {
            var cmd = new SqlCommand(sql, conn) { CommandType = CommandType.StoredProcedure };

            var startParam = new SqlParameter
            {
                ParameterName = "@startRowNum",
                Value = start,
                SqlDbType = SqlDbType.Int
            };

            cmd.Parameters.Add(startParam);

            var endParam = new SqlParameter
            {
                ParameterName = "@endRowNum",
                Value = end,
                SqlDbType = SqlDbType.Int
            };

            cmd.Parameters.Add(endParam);
            var outputParam = cmd.Parameters.Add("@recordCount", SqlDbType.Int);
            outputParam.Direction = ParameterDirection.Output;

            await conn.OpenAsync();
            using (var rdr = await cmd.ExecuteReaderAsync())
            {
                while (await rdr.ReadAsync())
                {
                    var dictionary = new Dictionary<string, object>();
                    foreach (var i in Enumerable.Range(0, rdr.FieldCount))
                    {
                        var dbValue = string.IsNullOrWhiteSpace(rdr.GetValue(i).ToString()) == true
                            ? string.Empty
                            : rdr.GetValue(i);

                        dictionary.Add(rdr.GetName(i), dbValue);
                    }

                    data.Add(dictionary);
                }
            }

            Int32.TryParse(cmd.Parameters["@recordCount"].Value.ToString(), out var recordCount);
            resp.Data = data;
            resp.RecordCount = recordCount;
        }

        return resp;
    }
    else
    {
        var data = new List<Dictionary<string, object>>();
        var resp = new Response();

        using (var conn = new SqlConnection(_ConnectionString))
        {
            string sql = "Select * from " + table;
            SqlCommand cmd = new SqlCommand(sql, conn);

            await conn.OpenAsync();
            using (var rdr = await cmd.ExecuteReaderAsync())
            {
                while (await rdr.ReadAsync())
                {
                    var dictionary = new Dictionary<string, object>();
                    foreach (var i in Enumerable.Range(0, rdr.FieldCount))
                    {
                        var dbValue = string.IsNullOrWhiteSpace(rdr.GetValue(i).ToString()) == true
                            ? string.Empty
                            : rdr.GetValue(i);

                        dictionary.Add(rdr.GetName(i), dbValue);
                    }

                    data.Add(dictionary);
                }
            }

            Int32.TryParse(cmd.Parameters["@recordCount"].Value.ToString(), out var recordCount);
            resp.Data = data;
            resp.RecordCount = recordCount;
        }

        return resp;
    }
}

Where Response is below:

public class Response
{
    public int RecordCount { get; set; }
    public List<Dictionary<string, object>> Data { get; set; }
}

How can I improve this code and refactor it?

Answer 1

My C# is quite rusty, so take everything I'm saying with a grain of salt; I stand by the general principles of what I'm writing, but the syntax/style might be a little off.

---

The first problem is that the two branches of your function are identical except for the SqlCommand being used. You should factor out how you create the SqlCommand and then do the rest the same way. For example,

private SqlCommand GetSqlCommandFromTableIfExists(string table, int? start, int? end, SqlConnection conn, string default)
{
    SqlCommand cmd;
    if (_sqlToRun.ContainsKey(table))
    {
        string sql = _sqlToRun[table];
        cmd = new SqlCommand(sql, conn) { CommandType = CommandType.StoredProcedure };

        var startParam = new SqlParameter
        {
            ParameterName = "@startRowNum",
            Value = start,
            SqlDbType = SqlDbType.Int
        };

        cmd.Parameters.Add(startParam);

        var endParam = new SqlParameter
        {
            ParameterName = "@endRowNum",
            Value = end,
            SqlDbType = SqlDbType.Int
        };

        cmd.Parameters.Add(endParam);
        var outputParam = cmd.Parameters.Add("@recordCount", SqlDbType.Int);
        outputParam.Direction = ParameterDirection.Output;
    } 
    else
    {
        // You could easily not make the default a parameter if you prefer
        cmd = new SqlCommand(default, conn);
    }

    return cmd;
}

Then your function becomes

private async Task<Response> GetDataAsync(string table, int? start, int? end)
{

    var data = new List<Dictionary<string, object>>();
    var resp = new Response();

    using (var conn = new SqlConnection(_ConnectionString))
    {
        var cmd = GetSqlCommandFromTableIfExists(table, start, end, conn, "SELECT * FROM " + table);
        await conn.OpenAsync();
        using (var rdr = await cmd.ExecuteReaderAsync())
        {
            while (await rdr.ReadAsync())
            {
                var dictionary = new Dictionary<string, object>();
                foreach (var i in Enumerable.Range(0, rdr.FieldCount))
                {
                    var dbValue = string.IsNullOrWhiteSpace(rdr.GetValue(i).ToString()) == true
                        ? string.Empty
                        : rdr.GetValue(i);

                    dictionary.Add(rdr.GetName(i), dbValue);
                }

                data.Add(dictionary);
            }
        }

        Int32.TryParse(cmd.Parameters["@recordCount"].Value.ToString(), out var recordCount);
        resp.Data = data;
        resp.RecordCount = recordCount;
    }

    return resp;
}

---

Type unsafety is bad

Right now you're using a raw object as the value-type in your dictionary and Response - why? I assume its because this is a general function that is independent of the data you're grabbing, but it means you're losing out on what data you're adding and any kind of type safety. I think making the function and your Response type generic, and tweaking your implementation, you can ensure type-safety.

---

Don't make some random data object; just use resp.Data.

Answer 2

You've basically written a very crude ORM using ADO.NET, including some very odd functionality where the table name that is passed can actually be a stored procedure, and the next person who'll have to maintain your code will curse at you for doing so. Presenting your results as a List<Dictionary<string, object>> is helping nobody, because then this will need to be translated to meaningful objects anyway.

You should abandon this "generic" approach and instead use an ORM like Dapper or Entity Framework. Those are skills enterprises look for. Don't waste your time on re-inventing the wheel. "Unless you’re Ayende or Greg Young, you should not write your own ORM. ... If you’re writing ADO.Net code by hand, you’re stealing from your employer or client.".