Ottimizzazione di un algoritmo e/o struttura in C#

Question

Sto lavorando a un'applicazione in cui è possibile iscriversi a una newsletter e scegliere le categorie a cui si desidera iscriversi. Esistono due diversi gruppi di categorie: città e categorie.

Al momento dell'invio di e-mail (che è un assaggio programmato), devo guardare in quali città e in quali categorie è iscritto un sottoscrittore, prima di inviare l'e-mail. Ad esempio, se mi sono abbonato a "Londra" e "Manchester" come città di mia scelta e ho scelto "Cibo", "Tessuto" e "Elettronica" come categorie, riceverò solo le newsletter relative a queste.

La struttura è la seguente:

Su ogni NewsItem in Umbraco CMS non è una stringa commaseparated di città e categorie (in modo efficace, questi sono memorizzati come ID dei nodi da città e le categorie sono i nodi in Umbraco aswell) Quando ho iscriversi a una o più città e una o più categorie, memorizzo i nodi della città e della categoria nel database in tabelle personalizzate. Il mio relational mapping simile a questa:

e tutta la struttura è simile al seguente:

Per me, questo sembra come due serie di 1 - 1 .. * relazioni (un abbonato a una o più città e un abbonato a una o più categorie)

Per trovare quali e-mail per inviare chi l'abbonato, il mio codice assomiglia a questo:

private bool shouldBeAdded = false; 

// Dictionary containing the subscribers e-mail address and a list of news nodes which should be sent 
Dictionary<string, List<Node>> result = new Dictionary<string, List<Node>>(); 

foreach(var subscriber in subscribers) 
{ 
    // List of Umbraco CMS nodes to store which nodes the html should come from 
    List<Node> nodesToSend = new List<Node> nodesToSend(); 

    // Loop through the news 
    foreach(var newsItem in news) 
    { 
     // The news item has a comma-separated string of related cities 
     foreach (string cityNodeId in newsItem.GetProperty("cities").Value.Split(',')) 
     { 
      // Check if the subscriber has subscribed to the city 
      if(subscriber.CityNodeIds.Contains(Convert.ToInt32(cityNodeId))) 
      { 
       shouldBeAdded = true; 
      } 
     } 

     // The news item has a comma-separated string of related categories 
     foreach (string categoryNodeId in newsItem.GetProperty("categories").Value.Split(',')) 
     { 
      // Check if the subscriber has subscribed to the category 
      if(subscriber.CategoryNodeIds.Contains(Convert.ToInt32(categoryNodeId))) 
      { 
       shouldBeAdded = true; 
      } 
     } 
    } 

    // Store in list 
    if (shouldBeAdded) 
    { 
     nodesToSend.Add(newsItem); 
    } 

    // Add it to the dictionary 
    if (nodesToSend.Count > 0) 
    { 
     result.Add(subscriber.Email, nodesToSend); 
    } 
} 

// Ensure that we process the request only if there are any subscribers to send mails to 
if (result.Count > 0) 
{ 
    foreach (var res in result) 
    { 
     // Finally, create/merge the markup for the newsletter and send it as an email. 
    } 
} 

Mentre questo funziona, sono un po 'preoccupato per le prestazioni quando viene raggiunta una certa quantità di abbonati poiché siamo in tre cicli foreach annidati. Inoltre, ricordando i miei vecchi insegnanti predica: "per ogni ciclo for c'è una struttura migliore"

Quindi, vorrei il vostro parere sulla soluzione di cui sopra, c'è qualcosa che può essere migliorato qui con la struttura data? E causerà problemi di prestazioni nel tempo?

Qualsiasi aiuto/suggerimento è molto apprezzato! :-)

Grazie in anticipo.

Soluzione

Così, dopo un paio di buone ore di debug e fumblin' in giro ho finalmente si avvicinò con qualcosa che funziona (inizialmente, sembrava che il mio codice originale ha funzionato, ma non ha)

Purtroppo, non ho potuto farlo funzionare con qualsiasi query LINQ ho provato, così sono tornato al modo in cui 'scuola' "ol di iterazione ;-) l'algoritmo finale assomiglia a questo:

private bool shouldBeAdded = false; 

// Dictionary containing the subscribers e-mail address and a list of news nodes which should be sent 
Dictionary<string, List<Node>> result = new Dictionary<string, List<Node>>(); 

foreach(var subscriber in subscribers) 
{ 
    // List of Umbraco CMS nodes to store which nodes the html should come from 
    List<Node> nodesToSend = new List<Node> nodesToSend(); 

    // Loop through the news 
    foreach(var newsItem in news) 
    { 
     foreach (string cityNodeId in newsItem.GetProperty("cities").Value.Split(',')) 
     { 
      // Check if the subscriber has subscribed to the city 
      if (subscriber.CityNodeIds.Contains(Convert.ToInt32(cityNodeId))) 
      { 
       // If a city matches, we have a base case 
       nodesToSend.Add(newsItem); 
      } 
     } 

     foreach (string categoryNodeId in newsItem.GetProperty("categories").Value.Split(',')) 
     { 
      // Check if the subscriber has subscribed to the category 
      if (subscriber.CategoryNodeIds.Contains(Convert.ToInt32(categoryNodeId))) 
      { 
       shouldBeAdded = true; 

       // News item matched and will be sent. Stop the loop. 
       break; 
      } 
      else 
      { 
       shouldBeAdded = false; 
      } 
     } 

     if (!shouldBeAdded) 
     { 
      // The news item did not match both a city and a category and should not be sent 
      nodesToSend.Remove(newsItem); 
     } 
    } 

    if (nodesToSend.Count > 0) 
    { 
     result.Add(subscriber.Email, nodesToSend); 
    } 
} 

// Ensure that we process the request only if there are any subscribers to send mails to 
if (result.Count > 0) 
{ 
    foreach (var res in result) 
    { 
     // StringBuilder to build markup for newsletter 
     StringBuilder sb = new StringBuilder(); 

     // Build markup 
     foreach (var newsItem in res.Value) 
     { 
      // build the markup here 
     } 

     // Email logic here 
    } 
} 

Answer 1

In primo luogo è possibile break interno foreach non appena shouldBeAdde = true.

Si potrebbe anche usare LINQ, ma non sono sicuro se sarà più veloce (ma si potrebbe usare .AsParallel per renderlo multithreaded facilmente):

var nodesToSend = from n in news 
       where n.GetProperties("cities").Value.Split(',') 
        .Any(c => subscriber.CityNodeIds.Contains(Convert.ToInt32(c)) && 
       n.GetProperties("categories").Value.Split(',') 
        .Any(c => subscriber.CategoryNodeIds.Contains(Convert.ToInt32(c)) 
       select n; 

Il think completo sarebbe poi scendere a (incluso parallelo):

Dictionary<string, IEnumerable<Node>> result = new Dictionary<string, IEnumerable<Node>>(); 
foreach(var subscriber in subscribers) 
{ 
    var nodesToSend = from n in news.AsParallel() 
     where n.GetProperties("cities").Value.Split(',') 
       .Any(c => subscriber.CityNodeIds.Contains(Convert.ToInt32(c)) && 
      n.GetProperties("categories").Value.Split(',') 
       .Any(c => subscriber.CategoryNodeIds.Contains(Convert.ToInt32(c)) 
     select n; 

    if (nodesToSend.Count > 0) 
     result.Add(subscriber.Email, nodesToSend); 
} 

if (result.Count > 0) 
{ 
    foreach (var res in result) 
    { 
     // Finally, create/merge the markup for the newsletter and send it as an email. 
    } 
} 

Answer 2

Non penso che lo farai imbattersi in problemi di prestazioni in qualsiasi momento presto. Lo lascerei come lo hai tu ora e cercherò di ottimizzare solo dopo esserti imbattuto in un problema di prestazioni reali e di aver usato un profiler per verificare che questi loop costituiscano il problema. Attualmente, sembra che tu stia facendo un'ottimizzazione prematura.

Detto questo, il seguente potrebbe essere una possibile ottimizzazione:

è possibile memorizzare il rapporto di città in abbonato in un dizionario con la città come la chiave e gli iscritti per questa città come il valore del dizionario memorizzato come HashSet<T>. E puoi fare lo stesso per la categoria per l'abbonato.

Ora quando si invia la newsletter è possibile scorrere le voci di notizie è possibile recuperare gli abbonati per le città utilizzando il dizionario e è possibile recuperare gli abbonati per le categorie utilizzando il dizionario. Ora devi intersecare gli abbonati di città HashSet con gli abbonati di categoria HashSet e di conseguenza avrai tutti gli abbonati corrispondenti per la notizia.