0
\$\begingroup\$

I have this method that returns an IEnumerable list , it takes 5 seconds to execute , so I want to know how can I optimize it side performance time of response.

private IEnumerable<dynamic> GetData(IEnumerable<dynamic> listD)
{
    List<dynamic> data = new List<dynamic>();

    foreach (var item in listD)
    {
        var allPro = item?.Pro?.Select(x => new RepHelper
        {
            Res = x.Rep,
            Title = x.Title
        });

        string allRep = "";

        if (allPro.Any())
        {
            allRep = JsonConvert.SerializeObject(allPro);
        }

        data.Add(new
        {
            Id = item.Id,
            Auteur = item?.AspNetUsers?.Init ?? "",
            NiveauCode = item?.Niveau?.Code ?? "",
            NiveauTextColeur = item?.Niveau?.TextColeur ?? "",
            Rep = item?.Pro?.FirstOrDefault(p => p.Rep == true)?.Title ?? "",
            IdParent = item?.Id_Parent ?? -1,
            allReponse = allRep,
        });
    }
    return data;
}
\$\endgroup\$
3
  • 6
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$
    – Mast
    Oct 27, 2021 at 16:24
  • \$\begingroup\$ I have rolled back Rev 2 → 1. Please see What to do when someone answers. \$\endgroup\$ Nov 9, 2021 at 16:06
  • 1
    \$\begingroup\$ I would start by not ever using dynamic in performance-sensitive code. \$\endgroup\$ Nov 10, 2021 at 3:45

2 Answers 2

2
\$\begingroup\$

Linq Select and FirstOrDefault(Func) both have internal iterators, as well as JsonConvert.SerializeObject. So, if you count them, then you have at least 4 iterators going inside your method. You can at reduce that by combining Select and FirstOrDefault(Func) with a single iterator something like :

private IEnumerable<dynamic> GetData(IEnumerable<dynamic> listD)
{
    List<dynamic> data = new List<dynamic>();
    
    foreach (var item in listD)
    {
        if(item == null) continue;

        var responseList = new List<RepHelper>();

        string repTitle; 
        
        if(item?.Pro != null)
        {
            foreach(var pro in item.Pro)
            {
                if(pro == null) continue;
                
                var rep = new RepHelper { Res = pro.Rep, Title = pro.Title };
                
                if(repTitle == null && rep.Res == true)
                {                   
                    repTitle = rep.Title;                   
                }                               
            
                responseList.Add(rep);
            }
        }   
        
        data.Add(new
        {
            Id = item.Id,
            Auteur = item?.AspNetUsers?.Init ?? string.Empty,
            NiveauCode = item?.Niveau?.Code ?? string.Empty,
            NiveauTextColeur = item?.Niveau?.TextColeur ?? string.Empty,
            Rep = repTitle ?? string.Empty,
            IdParent = item?.Id_Parent ?? -1,
            allReponse = responseList.Count > 0 ? JsonConvert.SerializeObject(responseList) : string.Empty,
        });
    }
    
    return data;
}

Now, since you're returning IEnumerable, why not use yield return instead of initializing a List<dynamic> ? so your work would be like :

private IEnumerable<dynamic> GetData(IEnumerable<dynamic> listD)
{
    foreach (var item in listD)
    {
        // same code here ....
        
        
        yield return new
        {
            Id = item.Id,
            Auteur = item?.AspNetUsers?.Init ?? string.Empty,
            NiveauCode = item?.Niveau?.Code ?? string.Empty,
            NiveauTextColeur = item?.Niveau?.TextColeur ?? string.Empty,
            Rep = repTitle ?? string.Empty,
            IdParent = item?.Id_Parent ?? -1,
            allReponse = responseList.Count > 0 ? JsonConvert.SerializeObject(responseList) : string.Empty,
        };
    }
}

You can also go IEnumerable all the way, with something like this :

private string _repTitle; 
private bool _hasRepHelperCollection;

private IEnumerable<dynamic> GetData(IEnumerable<dynamic> source)
{
    foreach (var item in source)
    {   
        if(item != null)
        {
            _hasRepHelperCollection = false;
            _repTitle = null;
    
            var allPro = GetRepHelper(item?.Pro);

            yield return new
            {
                Id = item.Id,
                Auteur = item?.AspNetUsers?.Init ?? string.Empty,
                NiveauCode = item?.Niveau?.Code ?? string.Empty,
                NiveauTextColeur = item?.Niveau?.TextColeur ?? string.Empty,
                Rep = _repTitle ?? string.Empty,
                IdParent = item?.Id_Parent ?? -1,
                allReponse = _hasRepHelperCollection ? JsonConvert.SerializeObject(allPro) : string.Empty
            };
            
        }
    }
}

private IEnumerable<RepHelper> GetRepHelper(IEnumerable<dynamic> source)
{
    if(source == null)
        yield break;
    
    foreach(var item in source)
    {
        if(item != null)
        {
            var rep = new RepHelper { Res = response.Rep, Title = response.Title };
            
            if(_repTitle == null && rep.Rep)
            {                   
                _repTitle = rep.Title;
            }                               
            
            if(!hasRepHelperCollection)
                _hasRepHelperCollection = true;
            
            yield return rep;
        }   
    }
}
\$\endgroup\$
2
\$\begingroup\$

Other than the general rule of "don't use dynamic if you don't have to", there's not much to optimize here. The serialization to JSON is probably your hot spot. However, you can pre-allocate your final list size like so:

List<dynamic> data = new List<dynamic>(listD.Count());

and that will buy you fewer reallocations, depending on the number of items you're passing in.

However, back to "Don't use dynamic: your anonymous type is all dynamics due to the nature of the assignment, but it seems that you know most of them are strings while a couple are (probably) ints. So define a type:

public class FinalData
{
    public int Id { get; set; }

    public string Auteur { get; set; }

    public string NiveauCode { get; set; }

    public string NiveauTextColeur { get; set; }

    public string Rep { get; set; }

    public int IdParent { get; set; }

    public string allReponse { get; set; }
}

And it can still be assigned to a dynamic member of the List<dynamic> as you do. So here is the following code (note, I also combined your if into a ternary in the assignment to tighten things up:

    private IEnumerable<dynamic> GetData(IEnumerable<dynamic> listD)
    {
        List<dynamic> data = new List<dynamic>(listD.Count());

        foreach (var item in listD)
        {
            var allPro = ((IEnumerable<dynamic>)item?.Pro)?.Select(x => new RepHelper
            {
                Res = x.Rep,
                Title = x.Title
            });

            data.Add(new FinalData
            {
                Id = item?.Id ?? -1,
                Auteur = item?.AspNetUsers?.Init ?? string.Empty,
                NiveauCode = item?.Niveau?.Code ?? string.Empty,
                NiveauTextColeur = item?.Niveau?.TextColeur ?? string.Empty,
                Rep = ((IEnumerable<dynamic>)item?.Pro)?.FirstOrDefault(p => p.Rep)?.Title ?? string.Empty,
                IdParent = item?.Id_Parent ?? -1,
                allReponse = allPro?.Any() == true ? JsonConvert.SerializeObject(allPro) : string.Empty,
            });
        }

        return data;
    }

Now, important most of all: You need to define what is acceptable performance of the method. Five seconds is too slow. Measure twice, cut once is an old adage which applies quite well to software development.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I have tested the line of initialisation of the list data with pre allocation size listD.Count() this is take 1174 ms but without specifying the size it takes only 1 ms I have tested with PerfTips in visual studio , so here pre allocation list size dosn't speed up the method note that my count list is only 20 element what do you think ? \$\endgroup\$
    – Coder95
    Oct 28, 2021 at 9:08
  • \$\begingroup\$ @mecab95 Make an interface out of the object type and have that be the list type. Returning List<IDataItem> is fine. Also, only reason I'm not upvoting is == true. \$\endgroup\$
    – T145
    Oct 30, 2021 at 7:38
  • \$\begingroup\$ @T145 what is the advantage of using an interface here over a class type for DataItem in thé side of thé performance ? \$\endgroup\$
    – Coder95
    Oct 31, 2021 at 9:58

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.