Interacting with a database

Question

I've written my first class to interact with a database. I was hoping I could get some feedback on the design of my class and areas that I need to improve on. Is there anything I'm doing below that is considered a bad habit that I should break now?

• public IEnumerable<string> ReturnSingleSetting(int settingCode) interacts with a normalized table to populate combo boxes based on the setting value passed to it (for example, a user code of 20 is a user, sending that to this method would return all users (to fill the combobox).
• public void InsertHealthIndicator(string workflowType, string workflowEvent, int times, string workflowSummary) interacts with a stored procedure to write a workflow error type into another normalized table.
• public DataView DisplayHealthIndicator(DateTime startDate, DateTime endDate) uses another stored procedure to return the workflow error types between specific dates.

Note: Although this seems like I likely shouldn't use stored procedures in some areas here, I've done so so I can base some SSRS reports off the same stored procedures (so a bug fixed in one area is a bug fixed in both).

using System;
using System.Collections.Generic;
using System.Data;
using System.Globalization;
using System.Data.SqlClient;
using System.Windows;

namespace QIC.RE.SupportBox
{
    internal class DatabaseHandle
    {
        /// <summary>
        /// Class used when interacting with the database
        /// </summary>

        public string GetConnectionString()
        {
            // todo: Integrate into settings.xml
            return "Data Source=FINALLYWINDOWS7\\TESTING;Initial Catalog=Testing;Integrated Security=true";
        }

        public IEnumerable<string> ReturnSingleSetting(int settingCode)
        {
            var returnList = new List<string>();

            string queryString =      " select  setting_main"
                                    + " from    [marlin].[support_config]"
                                    + " where   config_code = " + settingCode.ToString(CultureInfo.InvariantCulture)
                                    + "         and setting_active = 1"
                                    + " order by setting_main";

            using (var connection = new SqlConnection(GetConnectionString()))
            {
                var command = new SqlCommand(queryString, connection);

                try
                {
                    connection.Open();

                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        while (reader.Read())
                        {
                            returnList.Add(reader[0].ToString());
                        }

                        reader.Close();
                    }
                }
                catch (Exception ex)
                {
                    MessageBox.Show(ex.ToString());
                    throw;
                }
                connection.Close();
            }
            return returnList;
        }

        public void InsertHealthIndicator(string workflowType, string workflowEvent, int times, string workflowSummary)
        {
            string queryString =
                "EXEC   [marlin].[support_add_workflow_indicator]"
                        + "@workflow_type = @workflowType,"
                        + "@workflow_event = @workflowEvent,"
                        + "@event_count = @eventCount,"
                        + "@event_summary = @eventSummary";

            using (var connection = new SqlConnection(GetConnectionString()))
            {
                try
                {
                    connection.Open();

                    using(var cmd = new SqlCommand(queryString, connection))
                    {
                        cmd.Parameters.AddWithValue("@workflowType", workflowType);
                        cmd.Parameters.AddWithValue("@workflowEvent", workflowEvent);
                        cmd.Parameters.AddWithValue("@eventCount", times);
                        cmd.Parameters.AddWithValue("@eventSummary", workflowSummary);

                        cmd.CommandType = CommandType.Text;

                        cmd.ExecuteNonQuery();
                    }
                    connection.Close();
                }
                catch(SqlException ex)
                {
                    string msg = "Insert Error: ";
                    msg += ex.Message;

                    throw new Exception(msg);
                }
            }
        }

        public DataView DisplayHealthIndicator(DateTime startDate, DateTime endDate)
        {
            string queryString = "[marlin].[support_retrieve_workflow_history]";

            using (SqlConnection connection = new SqlConnection(GetConnectionString()))
            {
                using (var cmd = new SqlCommand(queryString, connection))
                {
                    connection.Open();

                    cmd.CommandType = CommandType.StoredProcedure;
                    cmd.Parameters.AddWithValue("date_from", startDate.Date);
                    cmd.Parameters.AddWithValue("date_to", endDate.Date);

                    var reader = cmd.ExecuteReader();

                    var dt = new DataTable();
                    dt.Load(reader);

                    connection.Close();
                    return dt.DefaultView;

                }
            }

        }
    }
}

Answer 1

I believe it's much better to use some ORM together with LINQ, rather than writing raw SQL. It means more errors are checked at compile time, it will help you avoid some common mistakes and it will make your code much shorter.

I would also always use parametrized SQL queries and never concatenate them by hand. You do use them most of the time, and in the one case where you don't, there is no danger of SQL injection, because the parameter is an integer, but I still think it's better to use parameters everywhere. (I think it may also make your query faster thanks to caching, but I'm not completely sure about that.)

Also, you shouldn't throw Exception, you should create a custom class that inherits from Exception. And, if possible, include the original exception as inner exception, to make debugging the original source of the error easier.

Answer 2

There are so many things you can get wrong when you use raw SQL, for instance:

1. vulnerable to SQL injection
2. you need to take care of messy details, such as concatenating a query string
3. you aren't calling Dispose on the SQLCommand (should be in a using statement)
4. the using statement will close the connection for you, no need for the close call
5. everything is much longer and more complicated than it needs to be

As svick suggested, use LINQ to SQL. The entity classes and stored procedures (SupportConfig, db.SupportRetrieveWorkflowHistory) can be generated automatically in the ORM designer.

Here is how your code could look like using LINQ to SQL (adjustments may be required):

public IEnumerable<string> ReturnSingleSetting(int settingCode)
{
    using (var db = new TestingDataContext(GetConnectionString()))
    {
        var result = from row in db.SupportConfig
                        where row.ConfigCode == settingCode
                        && row.SettingActive
                        orderby row.SettingMain
                        select row.SettingMain;
        return result;
    }
}

public IEnumerable<WorkflowItem> DisplayHealthIndicator(DateTime startDate, DateTime endDate)
{
    using (var db = new TestingDataContext(GetConnectionString()))
    {
        return db.SupportRetrieveWorkflowHistory(startDate.Date, endDate.Date);
    }
}