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 string,
          quantity decimal,
          price decimal

 invoiceData: file identifier      string,
              supplier             string,
              supplier site        string,
              supplier e-mail      string,
              invoice number       string,
              invoice description  string,
              invoice date         string,--date formatted as MMDDYYYY for export
              po number            string,
              po line              int,
              po shipment          string,
              invoice quantity     decimal,
              unit price           decimal,
              line amount          decimal,
              tax amount           decimal,
              freight amount       decimal,
              misc. amt.           decimal,
              misc desc.           string

private DataTable rollupKitValues(DataTable invoiceData, DataTable kitData)
{
    DataTable finalizedData = invoiceData.Clone();

var enumeratedInvoiceLines = invoiceData.AsEnumerable(); var enumeratedKits = kitData.AsEnumerable(); var distinctInvoices = enumeratedInvoiceLines.Select(r => Convert.ToInt32(r["Invoice Number *"])).Distinct(); var distinctKits = enumeratedKits.Select(r => r["Kit_Description"].ToString().Trim()).Distinct(); foreach (var invoiceNumber in distinctInvoices)//scan per invoice item/kits are invoice specific { var allInvoiceLines = invoiceData.Clone();//staging table to store lines while being rolled up var invoiceItems = enumeratedInvoiceLines.Where(r => Convert.ToInt32(r["Invoice Number *"]) == invoiceNumber) .OrderBy(r => Convert.ToInt32(r["PO Line *"])) .ToArray(); for (int itemIndex = 0; itemIndex < invoiceItems.Length; itemIndex++) { string invoiceItemNum = invoiceItems[itemIndex]["Invoice Description"].ToString().Trim(); if (!distinctKits.Contains(invoiceItemNum))//not a kit sku, add to staging table and continue to next item { allInvoiceLines.Rows.Add(invoiceItems[itemIndex].ItemArray); } else//line item is a kit sku, find out which kit it is and then scan the items following on the invoice to ensure all items are present { var kitLineItem = invoiceItems[itemIndex]; var matchedKitItems = enumeratedKits.Where(r => String.Equals(r["Kit_Description"].ToString().Trim(), kitLineItem["Invoice Description"].ToString().Trim())); int numberOfKitsInvoiced = Convert.ToInt32(kitLineItem["Invoice Quantity *"]); int requiredKitItemCount = matchedKitItems.Count(); int startingPOLineNum = Convert.ToInt32(kitLineItem["PO Line *"]) + 1;//incrementing 1 to start with the next line int endingPOLineNum = startingPOLineNum + requiredKitItemCount - 1;//decrementing 1 to not skip ahead beyond the kit item count var invoiceItemsToCheck = invoiceItems.Where(r => Convert.ToInt32(r["PO Line *"]) >= startingPOLineNum && Convert.ToInt32(r["PO Line *"]) <= endingPOLineNum); if (invoiceItemsToCheck.Count() != requiredKitItemCount)//missing item(s) in 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("Items on invoice {0} do not match kit {1}.", invoiceNumber, invoiceItemNum)); } 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)); } } } } for (int rowIndex = 0; rowIndex < allInvoiceLines.Rows.Count; rowIndex++) { //renumber PO Lines to match current order and count of rows to account for rows that were rolled up allInvoiceLines.Rows[rowIndex]["PO Line *"] = rowIndex + 1; //add renumbered row to master table for return and export finalizedData.Rows.Add(allInvoiceLines.Rows[rowIndex].ItemArray); } } return finalizedData; }
up vote 2 down vote accepted

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.

  • 1
    Good points. Some of the comments I added are for display here but you're right in that they shouldn't be necessary to understand the code. The requirement comment you mentioned I put in as the requirement was TBD at the time. Thank you for your review, I'll leave the question open a while to see if anyone else has any input. – Michael McGriff May 10 '16 at 17:55

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.