5
\$\begingroup\$

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;

                }
            }

        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I've been playing around with LINQ but what do you mean by 'ORM' (sorry, new) \$\endgroup\$ – Michael A Aug 11 '12 at 14:12
  • \$\begingroup\$ and sorry, I know I'm asking a lot - but can you provide an example of an exception class that inherits from exception? \$\endgroup\$ – Michael A Aug 11 '12 at 14:15
  • 2
    \$\begingroup\$ ORM means Object-relational mapping. Examples in .Net are LINQ to SQL, Entity Framework or NHibernate. And for an example of creating custom exception, see How to: Create User-Defined Exceptions. \$\endgroup\$ – svick Aug 11 '12 at 18:33
  • \$\begingroup\$ In the code above, exception catching/throwing is fine. You only need a custom exception if you want to indicate such a thing. catch(Exception ex) is just a catch all & throw will re-throw the original exception. \$\endgroup\$ – Simon Halsey Aug 12 '12 at 16:19
5
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I've been told by one of the senior developers at work that I should avoid linq to sql as it's now depreciated - is there much truth to this statement? \$\endgroup\$ – Michael A Aug 11 '12 at 15:02
  • \$\begingroup\$ @Michael I haven't seen any mention of it being decpreciated officially however it appears most recommendations are for use of EF. There are benefits of both depending on the requirements of the solution and architecture \$\endgroup\$ – dreza Aug 11 '12 at 20:26
  • 2
    \$\begingroup\$ @Michael LINQ to SQL is neither dead nor deprecated and will not go away in the foreseeable future. StackExchange uses LINQ to SQL (yes, it still does). It is possible that LINQ to SQL and Linq to Entities (EF) may merge in the future — until then, it's good to use whichever fits best. \$\endgroup\$ – Adam Aug 11 '12 at 21:16
  • \$\begingroup\$ I'd also note that Linq-To-SQL continues to support stored procedures better than EF does. \$\endgroup\$ – RubberDuck Oct 28 '15 at 23:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.