Clean batch inserts with EntityFramework

Question

Always the endeavour to write cleaner code or in general better code.

I have a scenario where I receive an object OrdersRootObject which contains a list of Orders, BillingInfo, ShippingInfo, LineItems ... etc whilst receiving the object I need to connect to an additional data store ClientConfig, in which yet performs more logic.

I'm very much always looking to refactor and avoid "code smells"

1. Fetch Client Config
2. Get the retailersId
3. If retailer is not null perform next step
4. Loop through root object
5. If order does not exist in "other data store" then insert
6. Retrieve the Id of the newly inserted order Id and use the Id for a related table.

Looking at the below code, what "code smells" do we have? A better approach to batch inserts? A pattern to remove conditionals?

       public void Add(OrdersRootObject ordersRootObject,
            string userKey)
        {
        
        try
        {
            //Select the retailer name and grab the Id - Use our client config(Seperate DB)
            var clientConfig = _clientConfigRepository.FindByClientKey(userKey);

            //Get the retailerId from the uproduce Retailer table
            var retailer = _context.Retailers.FirstOrDefault(x => x.Name == clientConfig.ClientName);

            if (retailer != null)
            {
                var retailerId = retailer.Retailer_ID;

                //We now have the retailerId start inserting the orders
                //Grab the Orders from the ordersRootObject
                foreach (var item in ordersRootObject.Orders)
                {
                    //We need to check if the order already exists in the DB, If it does DO NOT insert
                    //Select order from Uproduce
                    var uproduceOrder = _context.Orders.FirstOrDefault(x => x.Id == item.Id);

                    if (uproduceOrder == null)
                    {
                        //If not guest check out then insert customer and return CustomerId

                        //If it's null, it's safe to say we can insert the one being passed in
                        Order order = new Order();

                        order.Retailer_ID = retailerId;

                        //Billing info
                        order.BillingAddressCity = item.Billing_Address.City;
                        order.BillingAddressFirstName = item.Billing_Address.First_Name;
                        order.BillingAddressLastName = item.Billing_Address.Last_Name;
                        order.BillingAddressLine1 = item.Billing_Address.Address1;
                        order.BillingAddressLine2 = item.Billing_Address.Address2;
                        order.BillingAddressPhone = item.Billing_Address.Phone;
                        order.BillingAddressPostalCode = item.Billing_Address.Zip;

                        //Order Date
                        DateTime? dt = null;
                        if (!string.IsNullOrWhiteSpace(item.Created_At))
                            dt = DateTime.ParseExact(item.Created_At, "yyyy-MM-dd HH:mm:ss,fff",
                                   System.Globalization.CultureInfo.InvariantCulture);

                        order.Date = dt;
                        _context.Orders.Add(order);
                        //SaveChanges();

                        if (order.Order_ID > 0)
                        {
                            //Do related table magic
                        }
                    }
                }
            }
            else {
                string message = String.Format("Error fetching Uproduce 'Retailer': BaseRepository:Add");
                throw new ApplicationException(message);
            }
        }
        catch (Exception)
        {
            string message = String.Format("Error fetching client config: BaseRepository:Add");
            throw new ApplicationException(message);
        }
    }

//EDIT

Firstly my original structure was horrendous, it had too much responsibility! Nevertheless I stripped back the individual "Inserts" and placed them in respective "Repositories" - BillingRepository, AddressRepository, OrderRepository, and LineItemsRepository, after a successful insert I returned a DTO.

My second step was exception handling, creating Custom Exceptions to be a little more descriptive than the former: AddOrderException and so on.

Because I have two data stores, two Contexts I guess my custom Repository Pattern became a little too coupled - A Repository should never instantiate another Repository defeats the purpose of the pattern.

Lastly the logic that batches an insert rather than having it in a single Repository, I created a Naïve attempt at a UnitOfWork The purpose of this is primarily to focus on batching, Insert the Customer, return Id, then insert the ShippingMethod and so on, until we have a complete Order.

The end result was to have a Windows Service that references the DLL that inherits from BatchOrder(UnitOfWork) that exposes UnitOfWork.Order.Create

I still have a few more tweaks ideally to roll back changes if one object does not successfully insert.

Answer 1

1. The number of comments is a smell. Code should be self-documenting. Consider using properly named functions to describe intent instead of comments.

For example, when you create your Order object, you could extract that to a function:

private Order CreateOrder(var item, var retailerId) {
    Order order = new Order();

    order.Retailer_ID = retailerId;
    order.BillingAddressCity = item.Billing_Address.City;
    order.BillingAddressFirstName = item.Billing_Address.First_Name;
    order.BillingAddressLastName = item.Billing_Address.Last_Name;
    order.BillingAddressLine1 = item.Billing_Address.Address1;
    order.BillingAddressLine2 = item.Billing_Address.Address2;
    order.BillingAddressPhone = item.Billing_Address.Phone;
    order.BillingAddressPostalCode = item.Billing_Address.Zip;
    order.Date = DetermineOrderDate(item);

    return order;
}

private DateTime? DetermineOrderDate(var item) {
    DateTime? parsedOrderDate = null;
    if (!string.IsNullOrWhiteSpace(item.Created_At)) {
        parsedOrderDate = DateTime.ParseExact(
              item.Created_At, 
              "yyyy-MM-dd HH:mm:ss,fff",
              System.Globalization.CultureInfo.InvariantCulture
        );
    }
    return parsedOrderDate;
}

---

2. Error messages that give zero context of the problem are not very useful. This tells us something broke, but doesn't give any clues as to who experienced it or why it happened. Assuming this is going into a log file somewhere, try adding relevant info to the message.

string message = String.Format("Error fetching Uproduce 'Retailer': BaseRepository:Add");

...or...

string message = String.Format(
    "Error fetching Uproduce 'Retailer': BaseRepository:Add for client {0}", 
    clientConfig.ClientName
);

---

3. catch (Exception) This means, whatever breaks, hide the actual stack trace. Anywhere in this method. Say the DateTime.ParseExact breaks. You wouldn't be able to tell. I suggest grabbing the Exception object and passing it along to your ApplicationException.

try {
    ...
} catch (Exception ex) {
    string message = String.Format(
        "Error fetching client config: BaseRepository:Add: {0}",
         ex.Message // or ex.StackTrace
    ); 
    throw new ApplicationException(message);
}

---

4. Commented out code gets forgotten easily. I presume you persist changes to the database context elsewhere, so why not remove the line //SaveChanges(); ?

---

5. Normally I won't add any code for future work. If I absolutely have to, I'll add a TODO, to it to get picked up by Visual studio tasks.

if (order.Order_ID > 0)
{
     //TODO: Do related table magic
}