1
\$\begingroup\$

I want to optimize this code of generate pdf file with many nested loops, what makes it slow that i have for loop with 20 items EmissionManchCoupEnvoi and then another nested foreach with method GetPropositionsQuest that do a query on a view in the database with 400k elements which is PropostionForPrint, now i have 12 secondes time to generate the file.

Here is my code :

for (int i = 1; i <= EmissionManchCoupEnvoi.Count; i++) {
  foreach(var item in GetPropositionsQuest(datab, EmissionManchCoupEnvoi[i - 1].QId)) 
  {
    foreach(var subitem in item) {
      if (subitem.reponse) {
        tableCE.AddCell(new PdfPCell(new Phrase(subitem.position.ToString() + " - " + subitem.libelle, fontArialBold_Reponse)) {
          Border = 0,
          Colspan = 1
        });
      }
      else {
        tableCE.AddCell(new PdfPCell(new Phrase(subitem.position.ToString() + " - " + subitem.libelle, fontArial_Reponse)) {
          Border = 0,
          Colspan = 1
        });
      }
    }
  }
}


    private static IEnumerable<dynamic> GetPropositionsQuest(Bdd12CdmContext datab, long id)
    {
      yield return datab.PropostionForPrint.Where(p => p.Id_question == id);
    }
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Feb 2 at 9:26

1 Answer 1

9
\$\begingroup\$

Since you've specifically asked about speeding your code up, I'll answer that first. The main issue you have here is that you're doing a server call for every value in EmissionManchCoupEnvoi, which you would really rather do once. With that in mind, we can get all of your items in one hit like so:

var IDs = EmissionManchCoupEnvoi
    .Select(emce => emce.QId)
    .ToList();
var items = datab.PropostionForPrint
    .Where(p => IDs.contains(p.Id_question))
    .ToLookup(p => p.Id_question);

ToLookup will create an in-memory object with all of your items, grouped by ID so you can look them without doing a seperate database call each time. This is going to be a big increase in speed. Before I show you how to use this let me also point out something you've done in your for loop: you are starting at i = 1 and going while i <= EmissionManchCoupEnvoi.Count, which is very different to the usual method of starting at i = 0 and going while i < EmissionManchCoupEnvoi.Count. Doing this the usual way would have prevented you needing to subtract 1 to look up the QId inside your loop. What's more, you're just using this to look up a value in an indexable and presumably therefore IEnumerable object, probably a List or Array. You should just use another foreach loop here, iterating over the IDs, which we have already extracted:

foreach (var ID in IDs)
{
    var item = items[ID];
}

Once you've done this something else becomes clear, you don't actually need to iterate over the IDs at all, just the collection of items you would get from each ID. With that in mind we can change our ILookup to an IGrouping and iterate over it directly:

var items = datab.PropostionForPrint
    .Where(p => IDs.contains(p.Id_question))
    .GroupBy(p => p.Id_question)
    // in order to maintain ordering, use an OrderBy here
    .OrderBy(p => p.Key);

foreach (var item in items)
{
    foreach (var subitem in item)
    {
        ...
    }
}

You may not even need the GroupBy unless you're doing something else with the item here, if instead you just wanted to preserve ordering you can just use an OrderBy and then just have the outer loop:

var items = datab.PropostionForPrint
    .Where(p => IDs.contains(p.Id_question))
    .OrderBy(p => p.Id_question);

foreach (var subitem in items)
{
    ...
}

Whether or not you can do this depends on if ordering is important and whether or not the IDs are ordered in EmissionManchCoupEnvoi, which I don't know. With that in mind I'll stick with the lookup, which is guaranteed to keep the same ordering as your current code. Now that you're here, note that what you're doing inside your if/else statement is almost the same in both branches. Here you can just select your font and otherwise call AddCell once. You can also simplify your string creation using the string interpolation syntax with $:

var font = subitem.response ? fontArialBold_Reponse : fontArial_Reponse;
tableCE.AddCell(new PdfPCell(new Phrase($"{subitem.position} - {subitem.libelle}", font)) 
    {
      Border = 0,
      Colspan = 1
    });

For ease of reading I would also extract the creation of the Phrase and PdfCell. Putting this all together, the final code looks like this:

var IDs = EmissionManchCoupEnvoi
    .Select(emce => emce.QId)
    .ToList();
var items = datab.PropostionForPrint
    .Where(p => IDs.contains(p.Id_question))
    .ToLookup(p => p.Id_question);

foreach (var ID in IDs)
{
    var item = items[ID];
    foreach (var subitem in item)
    {
        var font = subitem.response ? fontArialBold_Reponse : fontArial_Reponse;
        var phrase = new Phrase($"{subitem.position} - {subitem.libelle}", font);
        var cell = new PdfPCell(phrase) 
            {
                Border = 0,
                Colspan = 1
            };
        tableCE.AddCell(cell);
    }
}

EXTRA NOTES

This isn't a review of your code so much as some advice: make sure you have an index on you Id_question column. I cannot overstate this enough, if you do not have that index then it is going to slow things down a lot.

Additionally, entity framework has the ability to select only specific columns from a table. This can be useful if you only need a few columns from a table which has many, and can sometimes bring a noticeable speed increase. In your case it may be worth doing this if you are only querying these items and not modifying them. You can change your items query like so:

var items = datab.PropostionForPrint
    .Where(p => IDs.contains(p.Id_question))
    .Select(p => new
    {
        p.Id_question,
        p.response,
        p.position,
        p.libelle
    })
    .ToLookup(p => p.Id_question);
\$\endgroup\$
0

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.