Rolling up Invoice Items Matching Certain Criteria

I have a listing of invoice line items that I need to apply some convoluted business logic to. We have another listing of items that I compare the invoice items to and if I find matches, I then need to ensure that that all of the values from yet another listing of items are present on the invoice in the correct quantities, based on the number of the original invoice line item was ordered.

Reading that back doesn't make much sense to me, and I wrote it. I'll try to help by providing a couple examples.

We have an item, let's call it PIZZA KIT, which is just a placeholder item for all the other items inside that kit. On our invoice, there will be the PIZZA KIT item, and then several other items sequentially following it on the invoice within that kit. All of these values (name of kit, name of item, quantity required to qualify for kit) are stored separately and provided to this method via the DataTable kitData for comparison.

DataTable invoiceData contains all of the necessary raw data from applicable invoices to this process. The item PIZZA KIT does not have any value on the invoice itself, the items below it summed together hold all of the value for the kit.

The customer which receives the result of this process doesn't want to see any of the non kit items, they just want to see PIZZA KIT and the sum total of the value of all the items as the PIZZA KIT value. I need to validate that if a kit item is on a particular invoice, all the associated sub-items are also on this invoice, in the correct quantities.

I've written the very lengthy function below, which works as desired from my testing to date. As I said, it's quite lengthy and it seems to me that there should be a more elegant way to write it. I'm open to any/all suggestions on ways I can improve the code length, look, or overall algorithm.

If there are any questions, please let me know.

As requested: schema for both tables:

```
kitData: kit_description string,
item_number s

I noticed that you have a section of code where a boolean variable floats out of a foreach loop and into the next if statement, but you can remove the if statement, and the boolean variable.

Here is the code that I am looking at

else
{
    bool allItemsQuantitiesMatched = true;
    foreach (var checkedItem in invoiceItemsToCheck)
    {
        var kitItem = matchedKitItems.Single(r => 
            String.Equals(r["Item_Number"].ToString().Trim(), checkedItem["Invoice Description"].ToString().Trim()));

        var comparisonValues = new
        {
            decimalCountOfKitsOnInvoice = Convert.ToDecimal(numberOfKitsInvoiced),
            kitRequiredQuantity = Convert.ToDecimal(kitItem["Quantity"]),
            invoicedQuantity = Convert.ToDecimal(checkedItem["Invoice Quantity *"])
        };

        allItemsQuantitiesMatched = (comparisonValues.invoicedQuantity == 
            (comparisonValues.kitRequiredQuantity * comparisonValues.decimalCountOfKitsOnInvoice));

        if (!allItemsQuantitiesMatched)
        {
            break;
        }
    }

    if (allItemsQuantitiesMatched)//merge line item values into kit sku values cells and skip inserting them to the staging table
    {
        foreach (var verifiedItem in invoiceItemsToCheck)
        {
            kitLineItem["Line Amount *"] = Convert.ToDecimal(kitLineItem["Line Amount *"]) +  Convert.ToDecimal(verifiedItem["Line Amount *"]);
        }

        kitLineItem["Unit Price *"] = 
            Math.Round(Convert.ToDecimal(kitLineItem["Line Amount *"]) / Convert.ToDecimal(numberOfKitsInvoiced), 2, MidpointRounding.AwayFromZero);

        allInvoiceLines.Rows.Add(kitLineItem.ItemArray);
        itemIndex = endingPOLineNum - 1;//this is skipping a row otherwise as the po line index is 1 based
    }
    else//some item quantities didn't match kit
    {
        //probably need to assign tax amount and freight amount to the next line item and drop the kit sku
        throw new Exception(String.Format("Quantity mismatch on invoice {0} with kit {1}.", invoiceNumber, invoiceItemNum));
    }
}

I think that you can throw your exception earlier, inside the foreach loop, then you don't have to create the boolean variable allItemsQuantitiesMatched, it was a bad name anyway because it only checked on item's quantity at a time. you can reduce some of the indentation as well, because the if/then that follows can be eliminated because we are going to throw the exception where we assigned a value to the variable inside that previous foreach loop.

we can also get rid of the comment

//probably need to assign tax amount and freight amount to the next line item and drop the kit sku

This information shouldn't be a comment, it should be a requirement or something like that, but not a comment.

this is what happened when I made those changes:

else
{
    bool allItemsQuantitiesMatched = true;
    foreach (var checkedItem in invoiceItemsToCheck)
    {
        var kitItem = matchedKitItems.Single(r => 
            String.Equals(r["Item_Number"].ToString().Trim(), checkedItem["Invoice Description"].ToString().Trim()));

        var comparisonValues = new
        {
            decimalCountOfKitsOnInvoice = Convert.ToDecimal(numberOfKitsInvoiced),
            kitRequiredQuantity = Convert.ToDecimal(kitItem["Quantity"]),
            invoicedQuantity = Convert.ToDecimal(checkedItem["Invoice Quantity *"])
        };

        if (comparisonValues.invoicedQuantity == 
            (comparisonValues.kitRequiredQuantity * comparisonValues.decimalCountOfKitsOnInvoice))
        {
            throw new Exception(String.Format("Quantity mismatch on invoice {0} with kit {1}.", invoiceNumber, invoiceItemNum));
        }
    }

    foreach (var verifiedItem in invoiceItemsToCheck)
    {
        kitLineItem["Line Amount *"] = Convert.ToDecimal(kitLineItem["Line Amount *"]) + Convert.ToDecimal(verifiedItem["Line Amount *"]);
    }

    kitLineItem["Unit Price *"] = 
        Math.Round(Convert.ToDecimal(kitLineItem["Line Amount *"]) / Convert.ToDecimal(numberOfKitsInvoiced), 2, MidpointRounding.AwayFromZero);

    allInvoiceLines.Rows.Add(kitLineItem.ItemArray);
    itemIndex = endingPOLineNum - 1;//this is skipping a row otherwise as the po line index is 1 based
}

You have a lot of comments in this code. I am hoping that is for our benefit and not actually in the code, there is just so many comments, if you need that many comments it means that your code should be reworked so that you can tell what is going on through the code just by reading the code.

Code is for our benefit, not the machine's benefit, so make it readable.

Code should tell a story.

StackExchange Code Review Q#125821, answer score: 2