Optymalizowanie algorytmu i/lub struktury w C#

Question

Pracuję nad aplikacją, w której można zapisać się do newslettera i wybrać kategorie, które chcesz subskrybować. Istnieją dwa różne zestawy kategorii: miasta i kategorie.

Podczas wysyłania wiadomości e-mail (co jest zaplanowanym wyborem), przed wysłaniem wiadomości e-mail muszę sprawdzić, które miasta i kategorie subskrybentów zasubskrybowano. Jeśli mam abonament na "Londyn" i "Manchester" jako moje wybrane miasta i wybrałem "Żywność", "Tkanina" i "Elektronika" jako moje kategorie, otrzymam tylko biuletyny, które się do nich odnoszą.

Struktura jest następująca:

Na każdym newsitem w Umbraco CMS jest commaseparated ciąg miast i kategorii (w praktyce są one przechowywane jako węzłów identyfikatory od czasu miastach i kategorie są węzły w Umbraco aswell) Kiedy zasubskrybuj jedno lub więcej miast i jedną lub więcej kategorii, przechowuję miasto i kategorię nodeids w bazie danych w niestandardowych tabelach. Moja relacyjne mapowanie wygląda następująco:

I cała konstrukcja wygląda następująco:

Dla mnie wydaje się dwoma zestawami 1 - 1 .. * relations (jeden subskrybent do jednego lub wielu miast i jeden subskrybent do jednej lub wielu kategorii)

Aby dowiedzieć się, które e-maile mają wysyłać do kogo subskrybenta, mój kod wygląda następująco:

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. 
    } 
} 

Podczas tej pracy jestem nieco zaniepokojony osiągami, gdy pewna liczba subskrybentów zostanie osiągnięta, ponieważ jesteśmy w trzech zagnieżdżonych pętlach foreach. Pamiętaj też, że moi dawni nauczyciele nauczają: "dla każdej pętli for jest lepsza struktura"

Więc, chciałbym twojej opozycji do powyższego rozwiązania, czy jest coś, co można poprawić tutaj z daną strukturą? I czy spowoduje to problemy z wydajnością w czasie?

Każda pomoc/podpowiedź jest bardzo doceniana! :-)

Z góry dziękuję.

Rozwiązanie

Więc po kilku dobrych godzin debugowania i fumblin' Around I wreszcie się z czymś, co działa (początkowo wyglądał jak mój oryginalny kod działa, ale tak się nie stało)

Niestety, nie mogę zmusić go do pracy z jakichkolwiek zapytań LINQ próbowałem, więc poszedłem z powrotem do szkoły „ol«»drodze iteracji ;-) ostateczny algorytm wygląda następująco:

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

Najpierw możesz break uzyskać wewnętrzne foreach, jak tylko shouldBeAdde = true.

też mogli używać LINQ, ale nie jestem pewien, czy to będzie szybciej (ale można użyć .AsParallel aby to wielowątkowy łatwo):

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; 

Kompletny sądzę, by następnie zejść do (w tym równolegle):

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

Nie sądzę, że będziesz napotkasz problemy z wydajnością w najbliższym czasie. Zostawiłbym to tak, jak masz to teraz i tylko próbujesz zoptymalizować po napotkaniu rzeczywistego problemu wydajności i użyłeś profilera, aby sprawdzić, czy te pętle są problemem. Obecnie wygląda na to, że robisz przedwczesną optymalizację.

Mimo, że dodaje się może być ewentualna optymalizacja:

Mogłeś zapisać relację z miasta do abonent w słowniku z miastem jako klucz i abonentów dla tego miasta jako wartości słownika przechowywane jako HashSet<T>. I możesz zrobić to samo dla kategorii dla subskrybenta.

Teraz, kiedy wysyłasz swój biuletyn, możesz przeglądać wiadomości, które możesz wyszukać dla abonentów miast korzystających ze słownika, a także pobrać subskrybentów dla kategorii przy użyciu słownika. Teraz musisz przeciąć HashSet z subskrybentami kategorii HashSet, co spowoduje, że będziesz mieć wszystkich pasujących subskrybentów do wiadomości.