4
\$\begingroup\$

I have a method that works perfectly well and does what it's supposed to, albeit a little slow (30 - 40 seconds). I would like to be able to speed it up or at least make it more efficient, any ideas please?

The purpose is to generate a list of which customers are buying the top requested (topN) lines from a supplier. The first part gets a list of products from the requested supplier, the second part works out which products are the best selling by distribution and then sales and then adds those products to an array and they form the column names for the datatable. The last part loops all of the customer accounts and puts yes or no in the column if they have bought it in the last 3 months. I hope that makes sense.

private void CustomersBuyingRange(string supplier, int topN)
{
    // Get a list of products from a supplier.
    string[] products = productDetails
                        .Where(x => x.SupplierID == supplier.ToUpper())
                        .Select(x => x.ProductCode).ToArray();

    // Rank the products by distribution and then by sales,
    // then add the required amount to an array.
    string[] topSellers = detailedOrderLines
                            .Where(x => products.Contains(x.ProductCode))
                            .GroupBy(x => x.ProductCode)
                            .Select(x => new
                            {
                                x.FirstOrDefault().ProductCode,
                                DeliveredQty = x.Sum(p => p.DeliveredQty),
                                Distribution = x.Select(c => c.CustomerID).Distinct().Count()
                            })
                            .OrderByDescending(x => x.Distribution)
                            .ThenByDescending(x => x.DeliveredQty)
                            .Take(topN)
                            .Select(a => a.ProductCode).ToArray();

    DataTable table = new DataTable("Customers");
    table.Columns.Add("CustomerID", typeof(string));
    table.Columns.Add("Customer", typeof(string));
    table.Columns.Add("AccountManager", typeof(string));
    // Add the columns with the products.
    foreach (string product in topSellers)
    {
        table.Columns.Add(product, typeof(string));
    }

    // We only want customers that have had an order in the last 3 months.
    var customers = customerDetails
        .Where(x => x.LastInvoiceDate > DateTime.Now.AddMonths(-3).Date)
        .OrderBy(x => x.CustomerName)
        .ToList();

    foreach (var customer in customers)
    {
        DataRow row = table.NewRow();
        row["CustomerID"] = customer.CustomerID;
        row["Customer"] = customer.CustomerName;
        row["AccountManager"] = customer.AccountManager;
        for (int i = 0; i < topN; i++)
        {
            row[topSellers[i]] = detailedOrderLines
                .Any(x => x.CustomerID == customer.CustomerID &&
                     x.ProductCode == topSellers[i] &&
                     x.InvoiceDate > DateTime.Now.AddMonths(-3).Date
                     ) ? "Yes" : "No";
        }

        table.Rows.Add(row);
    }
    Export.ExportToExcel(table, true);
}

There are roughly 3000 customers.
Any advise would be greatly appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ The "easy" fix here is to not do this in your client, but in the database... Unfortunately that doesn't make for a great answer, so I'll just leave it as a comment :) \$\endgroup\$ – Vogel612 Mar 18 at 13:15
  • 4
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Mar 18 at 13:25
  • \$\begingroup\$ What data types are productDetails, detailedOrderLines and customerDetails and what type of objects do they contain and how do you instantiate them? \$\endgroup\$ – Henrik Hansen Mar 19 at 11:52
3
\$\begingroup\$

As said in a comment this may be done better and faster with an SQL script depending on how the involved data sets are created, but below are some optimization suggestions.


You should maybe consider some input check:

private void CustomersBuyingRange(string supplier, int topN)
{
  if (string.IsNullOrWhiteSpace(supplier)) return;
  if (topN <= 0) throw new ArgumentOutOfRangeException(nameof(topN));

Optimizations:

1)

  // Get a list of products from a supplier.
  string[] products = productDetails
                      .Where(x => x.SupplierID == supplier.ToUpper())
                      .Select(x => x.ProductCode).ToArray();

supplier.ToUpper() is called repeatedly for every element in the vector. Consider to do it once:

  supplier = supplier.ToUpper();

  // Get a list of products from a supplier.
  var products = productDetails
                      .Where(x => x.SupplierID == supplier)
                      .Select(x => x.ProductCode);

As shown, there is also no need to populate an array with the result here.


2)

  var customers = customerDetails
      .Where(x => x.LastInvoiceDate > DateTime.Now.AddMonths(-3).Date)
      .OrderBy(x => x.CustomerName)
      .ToList();

Again you create a new DateTime object for each customer you evaluate in the Where() call. Consider to create that date once:

    DateTime minDate = DateTime.Now.AddMonths(-3).Date;
    // We only want customers that have had an order in the last 3 months.
    var customers = customerDetails
        .Where(x => x.LastInvoiceDate > minDate)
        .OrderBy(x => x.CustomerName);

You can then reuse it in the last loop too.


3)

    for (int i = 0; i < topN; i++)
    {
      row[topSellers[i]] = detailedOrderLines
          .Any(x => x.CustomerID == customer.CustomerID &&
               x.ProductCode == topSellers[i] &&
               x.InvoiceDate > DateTime.Now.AddMonths(-3).Date
               ) ? "Yes" : "No";
    }

Here you should be able to run a foreach loop instead:

      foreach (var topSeller in topSellers)
      {
        row[topSeller] = detailedOrderLines
            .Any(x => x.CustomerID == customer.CustomerID &&
                 x.ProductCode == topSeller &&
                 x.InvoiceDate > minDate) ? "Yes" : "No";
      }

4)

foreach (var customer in customers)
  {
    DataRow row = table.NewRow();
    row["CustomerID"] = customer.CustomerID;
    row["Customer"] = customer.CustomerName;
    row["AccountManager"] = customer.AccountManager;
    for (int i = 0; i < topN; i++)
    {
      row[topSellers[i]] = detailedOrderLines
          .Any(x => x.CustomerID == customer.CustomerID &&
               x.ProductCode == topSellers[i] &&
               x.InvoiceDate > DateTime.Now.AddMonths(-3).Date
               ) ? "Yes" : "No";
    }
    table.Rows.Add(row);
  }

Here you keep require the entire detailedOrderLines although you previously when creating the topSellers actually grouped those by ProductCode. So if you instead of selecting topSellers as strings select them as anonymous objects like:

  // Rank the products by distribution and then by sales,
  // then add the required amount to an array.
  var topProducts = detailedOrderLines
                          .Where(x => products.Contains(x.ProductCode))
                          .GroupBy(x => x.ProductCode)
                          .Select(x => new
                          {
                            ProductCode = x.FirstOrDefault().ProductCode,
                            DeliveredQty = x.Sum(p => p.DeliveredQty),
                            Distribution = x.Select(c => c.CustomerID).Distinct().Count(),
                            OrderLines = x.ToList()
                          })
                          .OrderByDescending(x => x.Distribution)
                          .ThenByDescending(x => x.DeliveredQty)
                          .Take(topN).ToList();

you only have to query the subset of orderLines that belongs to the current ProductCode:

    foreach (var customer in customers)
    {
      DataRow row = table.NewRow();
      row["CustomerID"] = customer.CustomerID;
      row["Customer"] = customer.CustomerName;
      row["AccountManager"] = customer.AccountManager;

      foreach (var topProduct in topProducts)
      {
        row[topProduct.ProductCode] = topProduct.OrderLines.Any(x => x.CustomerID == customer.CustomerID && x.InvoiceDate < minDate) ? "Yes" : "No";
      }

      table.Rows.Add(row);
    }

All in all the method could then be something like:

private void CustomersBuyingRange(string supplier, int topN)
{
  if (string.IsNullOrWhiteSpace(supplier)) return;
  if (topN <= 0) throw new ArgumentOutOfRangeException(nameof(topN));

  supplier = supplier.ToUpper();

  // Get a list of products from a supplier.
  var products = productDetails
                      .Where(x => x.SupplierID == supplier)
                      .Select(x => x.ProductCode);

  // Rank the products by distribution and then by sales,
  // then add the required amount to an array.
  var topProducts = detailedOrderLines
                          .Where(x => products.Contains(x.ProductCode))
                          .GroupBy(x => x.ProductCode)
                          .Select(x => new
                          {
                            ProductCode = x.FirstOrDefault().ProductCode,
                            DeliveredQty = x.Sum(p => p.DeliveredQty),
                            Distribution = x.Select(c => c.CustomerID).Distinct().Count(),
                            OrderLines = x.ToList()
                          })
                          .OrderByDescending(x => x.Distribution)
                          .ThenByDescending(x => x.DeliveredQty)
                          .Take(topN).ToList();

  using (DataTable table = new DataTable("Customers"))
  {
    table.Columns.Add("CustomerID", typeof(string));
    table.Columns.Add("Customer", typeof(string));
    table.Columns.Add("AccountManager", typeof(string));
    // Add the columns with the products.
    foreach (var topProduct in topProducts)
    {
      table.Columns.Add(topProduct.ProductCode, typeof(string));
    }

    DateTime minDate = DateTime.Now.AddMonths(-3).Date;
    // We only want customers that have had an order in the last 3 months.
    var customers = customerDetails
        .Where(x => x.LastInvoiceDate > minDate)
        .OrderBy(x => x.CustomerName);


    foreach (var customer in customers)
    {
      DataRow row = table.NewRow();
      row["CustomerID"] = customer.CustomerID;
      row["Customer"] = customer.CustomerName;
      row["AccountManager"] = customer.AccountManager;

      foreach (var topProduct in topProducts)
      {
        row[topProduct.ProductCode] = topProduct.OrderLines.Any(x => x.CustomerID == customer.CustomerID && x.InvoiceDate < minDate) ? "Yes" : "No";
      }

      table.Rows.Add(row);
    }

    Export.ExportToExcel(table, true);
  }
}

Disclaimer: I haven't had the opportunity to test the above, so don't hang me if it doesn't improve anything or if I have misunderstood something.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi Henrik, thanks for all of the tips it's greatly appreciated. I will test it all later and let you know. \$\endgroup\$ – Lee Morgan Mar 19 at 14:52
2
\$\begingroup\$
  • My eye started twitching uncontrollably when I saw .Where(x => x.SupplierID == supplier.ToUpper()). To me, this indicates a major design flaw: something that's called an ID should not be compared to a string that needs to be uppercased.

  • Names are important. string[] products = [...] .Select(x => x.ProductCode).ToArray(); suggests to me that this should be named productCodes, not products. See also DataTable table: "table" is about as undescriptive as you can get.

  • The best solution would be to investigate if you couldn't replace all this code with a single query.
\$\endgroup\$

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.