2
\$\begingroup\$

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?)

\$\endgroup\$
8
  • \$\begingroup\$ throw exceptions instead of using Boolean, and only use rollback inside the catch or final blocks. This way, you will be able to trace down exceptions better. \$\endgroup\$
    – iSR5
    Jan 30, 2021 at 11:05
  • \$\begingroup\$ Not going to do a review, just want to remark that the inconsistent use of curly braces is annoying. IIRC the recommendation is to always use them, and certainly not to mix various styles. \$\endgroup\$
    – BCdotWEB
    Jan 30, 2021 at 14:43
  • \$\begingroup\$ Why is UpdateInvoiceAsync outside of the transaction? \$\endgroup\$
    – BCdotWEB
    Jan 30, 2021 at 14:44
  • \$\begingroup\$ @BCdotWEB yes, I agree! I need to add some braces. Thanks. Also, good question; I think I need to pass the transaction into updateinvoice or is it "automatic" since the transaction has been started? This wasn't clear in the dapper doc or I haven't found it yet. Looking for some guidance. I want it inside the transaction. \$\endgroup\$
    – GisMofx
    Jan 30, 2021 at 14:54
  • 1
    \$\begingroup\$ @aepot Thanks! Point noted. I did a little digging. DbConnection /should/ rollback on dispose, but that behavior is provider specific. If I change db provider, it better have rollback called. docs.microsoft.com/en-us/dotnet/api/… \$\endgroup\$
    – GisMofx
    Jan 31, 2021 at 12:42

1 Answer 1

2
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you. I've read thru this multiple times. I plan on making about all of these changes. I can share a little more of the dbconnection use from the repository. It actually calls a service injected inside the repository that instantiates a new ibconnection. Need to think more about how I want to capture error product number. Maybe. Generic result class? I want to pass the pertinent information for the UI from the business logic. \$\endgroup\$
    – GisMofx
    Feb 1, 2021 at 16:00
  • \$\begingroup\$ @GisMofx - you could do it with a Result class although this is much easier in languages like F# with tagged/discriminated unions when you need different data for different error cases. You could keep it simple and have a class with an enum and an optional product Id - new Fail { Type = Enum.ProductNotFound, Id = lineItem.ProductId } \$\endgroup\$
    – RobH
    Feb 2, 2021 at 7:25

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.