Clean batch inserts with EntityFramework

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"

• Fetch Client Config

• Get the retailersId

• If retailer is not null perform next step

• Loop through root object

• If order does not exist in "other data store" then insert

• 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

• 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;
}

1. 2.
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
);

1. 3.
2. 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);
}

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

1. 5.
2. 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
}

StackExchange Code Review Q#147723, answer score: 2