Reducing Inventory Quantity When Order Is Being Processed

Question

I've got a method which removes quantity from inventory when an order is completed. If there is not enough inventory quantity available, the order completion fails and the sql transaction is rolled back. Otherwise, transaction is commited.

I'm using Dapper for MySQL database access

        public async Task<(bool, string)> CompleteInvoice(Invoice invoice, List<InvoiceLineItem> lineItems)
        {
            bool success = true;
            string errorProductNumber = null;

            if (!await UpdateInvoiceLineItemsAsync(lineItems))
                return (false, "Error saving line items. Please try again.");
            //Check to see if there is available quantity
            using (var conn = _InvoiceRepository.DbConnection)
            {
                conn.Open();

                // create the transaction
                using (var trans = conn.BeginTransaction(System.Data.IsolationLevel.RepeatableRead))
                {
                    try
                    {
                        var selectProductSQL = "SELECT InventoryNumber,Quantity FROM Product WHERE Id = @productId";
                        var updateProductSQL = "UPDATE Product SET Quantity = @NewQuantity WHERE Id = @productId";
                        DynamicParameters productParameters;
                        foreach (var lineItem in lineItems)
                        {
                            productParameters = new();
                            productParameters.Add("productId", lineItem.ProductId, System.Data.DbType.String);
                            var cprod = await conn.QueryFirstOrDefaultAsync<Product>(selectProductSQL, productParameters, trans);
                            
                            if (cprod.Quantity >= lineItem.Quantity)
                            {
                                productParameters.Add("NewQuantity", cprod.Quantity - lineItem.Quantity, System.Data.DbType.Int32);
                                if (await conn.ExecuteAsync(updateProductSQL, productParameters, trans) != 1)
                                {
                                    success = false;
                                }
                            }
                            else
                            {
                                errorProductNumber = cprod.InventoryNumber;
                                success = false;
                            }
                            if (!success)
                                break;
                        }
                        if (success)
                        {
                            invoice.InvoiceStatus = "COMPLETE";
                            if (!await UpdateInvoiceAsync(invoice))
                            {
                                success = false;
                                errorProductNumber = "Error saving invoice";
                            }
                        }
                        // if it was successful, commit the transaction
                        if (success)
                            trans.Commit();
                        else
                        {
                            trans.Rollback();
                        }
                    }
                    catch (Exception ex)
                    {
                        // roll the transaction back
                        trans.Rollback();

                        // handle the error however you need to.
                        //throw;
                    }
                }
            }
            return (success, errorProductNumber);
        }

I'm open and any General feedback. Am I correctly using the SQL Transaction mechanism?(Also with Dapper?)

Answer 1

Are you sure that you need two queries for this?

E.g. you could do something like this:

UPDATE Product
SET    Quantity = Quantity - @usedQuantity
WHERE  Id = @productId
  AND  Quantity >= @usedQuantity

Try to execute that and if it doesn't affect 1 row - you don't have enough quantity for the line item.

The drawback for your current logic is that you no longer get the InventoryNumber for when there's an error. I think there are two reasons why this is a better approach anyway:

1. You now execute 1 query per line item on the happy path.
2. I think you have UI logic intermingled here - this code should just deal with ProductId. If you want to display something else, that's on the UI code to do.

---

Other points:

The (bool, string) return type is not idiomatic C#. I don't mind this pattern but am aware I am not in the majority with that opinion. In C#, an exception would probably be expected here. The other option is to create some kind of Result class, there are many examples of this around. E.g. https://github.com/altmann/FluentResults. It's still not recognized by most C# developers though.

_InvoiceRepository.DbConnection - this is not a good place to get configuration from. An IConnectionProvider interface that is used by all the data access may be a better option. Without more code, it's hard to tell.

// if it was successful, commit the transaction - don't comment like this. Anyone reading your code can understand if (success) Commit(). Comments are for why not what.

It looks like you're trying to keep one return statement and that is causing more conditionals with if (success) around the place. I am a big proponent of returning early but I know not everyone agrees. If you only want a single return, you need to at least factor out methods to get rid of the spaghetti.

errorProductNumber = "Error saving invoice"; - That's not a product number. I'm almost sure that you're displaying this string somewhere. Keep the UI concerns out of the infrastructure code.

This seemingly innocuous pattern is a big problem on more established code bases:

invoice.InvoiceStatus = "COMPLETE";
if (!await UpdateInvoiceAsync(invoice))

You have baked in what a complete invoice looks like in an arbitrary place in the code! What if you need to add a CompletedBy or CompletedDate? You have to find all these places and update them. You also have a magic string - it needs to be a constant. Consider instead:

invoice.Complete();
if (!await UpdateInvoiceAsync(invoice))

Now if you add a CompletedBy, you do it in that method and add a parameter. The compiler then ensures you haven't forgotten to update any places. You now have an appropriate place to introduce a constant for the magic string too:

public void Complete()
{
    // You can also add a guard to prevent invalid completion.
    if (!CanCompleteInvoice)
        throw new BusinessConstraintVoilatedException();

    this.InvoiceStatus = COMPLETED_STATUS; // <- name your constants however you like.
}

Overall this method does too much and is too long. You want each method to be scannable, pseudocode:

public async Task<(bool, string)> CompleteInvoice(
    Invoice invoice,
    List<InvoiceLineItem> lineItems)
{
    // Precondition checks omitted

    using (var conn = _InvoiceRepository.DbConnection)
    {
        conn.Open();
        using (var trans = conn.BeginTransaction(System.Data.IsolationLevel.RepeatableRead))
        {
            foreach (var lineItem in lineItems)
            {
                var lineItemResult = UpdateLineItem(lineItem, ...);
                // ... on error, do something
            }
            UpdateInvoiceAsync(invoice);
        }
    }
}