3
\$\begingroup\$
public (List<Order>, Dictionary<int, string>, Dictionary<int, string>) GetOrders(int RestaurantID, int StartID)
    {
        List<Order> Orders = null;
        Dictionary<int, string> MenuItemsXml = null;
        Dictionary<int, string> OrderHistoryXml = null;
        string sql = "SELECT ev.ID, ev.RestaurantID, ev.UserID, ev.DateCreated, ev.Amount, ev.AmountDiscounted, ev.TableNumber, (SELECT '|', FoodOrderDetail.ItemID, FoodOrderDetail.ItemName, FoodOrderDetail.Quantity, FoodOrderDetail.[Price], FoodOrderDetail.[Note] FROM FoodOrderDetail INNER JOIN FoodOrders a ON FoodOrderDetail.OrderID = a.ID WHERE a.ID = ev.ID ORDER BY DateCreated DESC FOR XML PATH('')) [OrderItems],(SELECT '|', FoodOrderHistory.[Status], FoodOrderHistory.[StatusDate] FROM FoodOrderHistory INNER JOIN FoodOrders a ON FoodOrderHistory.OrderID = a.ID WHERE a.ID = ev.ID ORDER BY StatusDate ASC FOR XML PATH('')) [OrderHistory] FROM FoodOrders ev WHERE ev.RestaurantID = @param1 AND ev.ID > @param2 AND ev.PreAuthorised = 1 ORDER BY ev.ID DESC;";

        using (var connection = new SqlConnection(ConnectionString))
        {
            using (SqlCommand command = new SqlCommand(sql, connection))
            {
                command.Parameters.Add(new SqlParameter("@param1", RestaurantID));
                command.Parameters.Add(new SqlParameter("@param2", StartID));
                connection.Open();
                using SqlDataReader dataReader = command.ExecuteReader(CommandBehavior.CloseConnection);
                if (dataReader.HasRows)
                {
                    Orders = new List<Order> { };
                    while (dataReader.Read())
                    {
                        Order objOrder = new Order();
                        objOrder.ID = Convert.ToInt32(dataReader["ID"]);
                        objOrder.UserID = Convert.ToInt32(dataReader["UserID"]);
                        objOrder.DateCreated = Convert.ToDateTime(dataReader["DateCreated"]);
                        objOrder.Amount = Convert.ToDecimal(dataReader["Amount"]);

                        if (dataReader["AmountDiscounted"] != DBNull.Value)
                        {
                            objOrder.Amount = Convert.ToDecimal(dataReader["AmountDiscounted"]);
                        }
                        if (dataReader["TableNumber"] != DBNull.Value)
                        {
                            objOrder.TableNumber = Convert.ToString(dataReader["TableNumber"]);
                        }
                        if (dataReader["OrderItems"] != DBNull.Value)
                        {
                            if(MenuItemsXml == null)
                            {
                                MenuItemsXml = new Dictionary<int, string>();
                            }
                            MenuItemsXml.Add(objOrder.ID, Convert.ToString(dataReader["OrderItems"]));
                        }
                        if(dataReader["OrderHistory"] != DBNull.Value)
                        {
                            if(OrderHistoryXml == null)
                            {
                                OrderHistoryXml = new Dictionary<int, string>();
                            }
                            OrderHistoryXml.Add(objOrder.ID, Convert.ToString(dataReader["OrderHistory"]));
                        }
                        Orders.Add(objOrder);
                    }
                }
                dataReader.Close();
            }
            if (connection.State == ConnectionState.Open)
            {
                connection.Close();
            }
        }
        return (Orders, MenuItemsXml, OrderHistoryXml);
    }

The above function is called by this function:

public List<Order> GetOrders(int RestaurantID, int StartID)
    {
        List<Order> Orders = null;
        Dictionary<int, string> OrderItemsXml = null;
        Dictionary<int, string> OrderHistoryXml = null;
        OrderData objData = new OrderData();

        try
        {
            (Orders, OrderItemsXml, OrderHistoryXml) = objData.GetOrders(RestaurantID, StartID);
        }
        catch (Exception ex)
        {
            logger.Error(ex);
        }

        if(OrderItemsXml != null)
        {
            foreach (var order in Orders)
            {
                try
                {
                    // stripe makes us store everything in major currency unit e.g. £1 is 100, so divide by 100 for when we later need to display values
                    if(order.Amount > 0)
                    {
                        order.Amount /= 100;
                    }
                    if (OrderItemsXml.ContainsKey(order.ID))
                    {
                        order.OrderItems = ConvertXmlToItemsList(OrderItemsXml[order.ID].TrimStart("|".ToCharArray()));
                    }
                    else
                    {
                        logger.Warn(string.Format("Dictionary OrderItemsXml does not contain OrderId Key of {0} in OrderManager", order.ID.ToString()));
                    }
                    if (OrderHistoryXml.ContainsKey(order.ID))
                    {
                        order.OrderHistory = ConvertXmlOrderHistory(OrderHistoryXml[order.ID].TrimStart("|".ToCharArray()));
                    }
                    else
                    {
                        logger.Warn(string.Format("Dictionary OrderHistoryXml does not contain OrderId Key of {0} in OrderManager", order.ID.ToString()));
                    }
                }
                catch (Exception ex)
                {
                    logger.Error(ex);
                }
            }
        }
        else
        {
            logger.Warn(string.Format("No order items found for RestaurantID {0} and StartID {1} in method GetOrders() in OrderManager", RestaurantID, StartID));
        }
        return Orders;
    }

Is function GetOrders() always closing the database connection? Sometimes I get Timeout errors and the most likely reason is the connection pool running out of connections. When this happens the CPU usage goes to 100%. This happens around once per day.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I don't know what your database looks like but I suspect that the timeouts are caused by the table structure and a SQL statement that is possibly overcomplex. You have joins, one subselect + XML PATH, but what about indexes and how much data do you store currently ? Have you run an execution plan against your query ? CommandBehavior.CloseConnection is not useful if you are wrapping the connection within a Using statement. \$\endgroup\$
    – Anonymous
    May 2 at 19:24
4
\$\begingroup\$

Quick remarks:

  • Please follow the Microsoft naming guidelines. Parameters should be camelcase, same with local variables.

  • Don't write your own ORM or use ADO.NET. Instead, use Dapper.

  • In the middle of retrieving your data from the DB, you start filling MenuItemsXml. Don't do that. Simply retrieve all the values, close the connection, and then start converting the retrieved data into other structures. (Ditto OrderHistoryXml.) In your case you might need a RawOrder which can store OrderItems and OrderHistory, and then afterwards you can (easily) convert this collection of RawOrder into a collection of Order.

  • I'm not a fan of having (long) SQL queries in the midst of C# code. I usually move these to .sql files which I then embed in the DLL and read by using a class like this.

  • Don't use the ContainsKey/get value pattern when using a Dictionary. Instead use the far more efficient TryGetValue.

  • OrderItemsXml and OrderHistoryXml have confusing names. These do not appear to have anything to do with XML. Give them proper names which express their contents (e.g. OrdeHistoryByOrderId).

  • Use meaningful names. @param1 and @param2 mean nothing.

\$\endgroup\$
2
\$\begingroup\$

Simple answer is yes. when you open a connection in a Using statement, once the scope leaves that statement, it automatically disposes of the connection, there really isn't a reason to explicitly close the connection when implementing a Using statement.

Reference: Microsoft Reference to the IDisposable Interface that is implemented by the DbConnection Class --> https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-5.0#using-an-object-that-implements-idisposable

Instead of writing out your SQL Statement in a String, you should create a Stored Procedure and call it from the code, and here is why:

  1. Parameterization of the input, when you hand the parameters/variables to the Stored Procedure, C#/SQL Server will automatically convert the input to a string so that SQL Injection can't happen

  2. Let SQL Server do what SQL Server does best, plan the query. When you have C# sending the Query in Text form, SQL Server has to create the query plan on the fly, every time. But, if you create a Stored Procedure, then SQL Server can create a plan (once) and use that plan to make it more efficient.

\$\endgroup\$
2
\$\begingroup\$

General Observations

After reviewing this code it is clear that the 2 functions come from 2 different classes, one class called OrderData which contains the first function shown (with the actual database query) and a second class currently unknown that contains the second function shown. To really help you optimize the code we would need to see the full definitions of both classes (the full contents of both .cs files).

It isn't clear in the code if it is all synchronous or if the database calls are asynchronous.

It isn't clear in the question if the database has been optimized using indexes within the tables.

As indicated in the answer by @BCdotWEB, you want to minimize the time the tables in the database are locked by just retrieving the data from the database, and then closing the connection to the database so that other queries that are accessing the same table can proceed. The OrderData.GetOrders() function should simply return a data table containing the raw data from the sql query there should be an intermediate function that processes that raw data and creates the list of Orders and the 2 dictionaries. Good object oriented programming follows the SOLID principles:

SOLID is 5 object oriented design principles. SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable. This will help you design your objects and classes better.

  1. The Single Responsibility Principle - A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.
  2. The Open–closed Principle - states software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
  3. The Liskov Substitution Principle - Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.
  4. The Interface segregation principle - states that no client should be forced to depend on methods it does not use.
  5. The Dependency Inversion Principle - is a specific form of decoupling software modules. When following this principle, the conventional dependency relationships established from high-level, policy-setting modules to low-level, dependency modules are reversed, thus rendering high-level modules independent of the low-level module implementation details.

The Single Responsibility Principle applies to methods and functions as well as to classes.

The code in this question violates the Single Responsibility Principle when it is applied to methods or functions.

Readability

The SQL query in OrderData.GetOrders() is almost unreadable. Don't create lines in the code that are wider than the IDE screen. Try to keep the lines less than 80 characters, and definitely less than 120 characters. You can break up string constant construction using the + (plus) operator. Compare the current code to

        string sql = "SELECT ev.ID, ev.RestaurantID, ev.UserID, ev.DateCreated, ev.Amount, ev.AmountDiscounted, ev.TableNumber," +
            " (SELECT '|', FoodOrderDetail.ItemID, FoodOrderDetail.ItemName, FoodOrderDetail.Quantity, FoodOrderDetail.[Price], FoodOrderDetail.[Note]" +
                "FROM FoodOrderDetail INNER JOIN FoodOrders a ON FoodOrderDetail.OrderID = a.ID WHERE a.ID = ev.ID " +
                "ORDER BY DateCreated DESC FOR XML PATH('')) [OrderItems], " +
                "(SELECT '|', FoodOrderHistory.[Status], FoodOrderHistory.[StatusDate] FROM FoodOrderHistory " + 
                "INNER JOIN FoodOrders a ON FoodOrderHistory.OrderID = a.ID WHERE a.ID = ev.ID " +
                "ORDER BY StatusDate ASC FOR XML PATH('')) [OrderHistory] FROM FoodOrders ev " + 
                "WHERE ev.RestaurantID = @param1 AND ev.ID > @param2 AND ev.PreAuthorised = 1 ORDER BY ev.ID DESC;";

Which is more readable?

For a query this complex it might be better to write it as a stored procedure and move the processing to the server rather than the local host. Then just call the stored procedure to get the raw data table.

Variable Name

Rather than use sql as the name for the query string, the variable name queryString might be more meaningful.

TSQL Exception Handling

The TSQL code in OrderData.GetOrders() should be embedded in in a try {} catch {} block within the function rather than having the try {} catch {} block in the calling function. Log the error and then return normally.

You will never need to explicitly close the database connection when using using (var connection = new SqlConnection(ConnectionString)) { ... } statement. The connection is opened and closed automatically by the using statement. Here is the Microsoft documentation.

\$\endgroup\$
0

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