Creating membership list with Parallel.ForEach [closed]

Question

I had a foreach loop which used to loop Membership users and create list of a custom class. Following was the for loop:

foreach (MembershipUser usr in userlist)
{
    userandGroup usrgp = new userandGroup();
    usrgp.id = ((Guid)usr.ProviderUserKey).ToString() ;
    usrgp.Name = usr.UserName;
    ProfileBase profile = ProfileBase.Create(usr.UserName);
    profile.Initialize(usr.UserName, true);
    // Following line takes approx 40ms to execute once.
    usrgp.DisplayName = profile.GetPropertyValue("FirstName").ToString() + " " + profile.GetPropertyValue("LastName").ToString();
    usrgp.type = "user";
    lst.Add(usrgp);
}

As the commented line used to take approx 40ms to execute, the whole loop for 40 users used to take approx 1600ms to execute.

I got suggestion to use Parallel.ForEach loop to reduce the execution time of the loop from this question

SO I updated the loop as:

    Object obj = new Object();
    Parallel.ForEach(userlist, (usr) =>
    {
        userandGroup usrgp = new userandGroup();
        usrgp.id = ((Guid)usr.ProviderUserKey).ToString();
        usrgp.Name = usr.UserName;
        ProfileBase profile = ProfileBase.Create(usr.UserName);
        profile.Initialize(usr.UserName, true);
        usrgp.type = "user";
        usrgp.DisplayName = profile.GetPropertyValue("FirstName").ToString() + " " + profile.GetPropertyValue("LastName").ToString();
        lock (obj)
        {                
            lst.Add(usrgp);
        }
    });

The loop works. Now the performance is way better (Although not best). It now takes only 700ms to execute the loop.

But I am not sure if it is the correct way to write Parallel.ForEach loop as I am new to it. Kindly suggest if the code is right?

Answer 1

Before going parallel I'd first try to understand which part of that code is slow. Maybe there is something you can do which will boost performance even more. That said I have few notes:

• userandGroup does not follow naming conventions, types should be PascalCase.
• There is no benefit to abbreviate names to usrgp and obj. is usrgrp a...group? Call it group (or userGroup if owner isn't clear from context). obj says what it is (an object), not what it does (the object which is used as synchronisation context). You may rename it (at least) to syncRoot (name used also in the framework itself for this purpose).
• If lst is local then you do not need obj and you can directly lock on it.
• You do not need to cast ProviderUserKey to Guid to call ToString(): usr.ProviderUserKey.ToString() or (if ProviderUserKey is object and it might be null) maybe Convert.ToString(usr.ProviderUserKey).
• Object initialisation may be simplified:

Using Object Initialization syntax:

var group =  new userandGroup
{
    id = usr.ProviderUserKey.ToString(),
    Name = usr.UserName
};

Follow naming convention also here, id should be Id. Also move Profile creation before group and you can initialise everything in this way.

• DisplayName initialisation may use string interpolation:

As before:

DisplayName = $"{profile.GetPropertyValue("FirstName")} {profile.GetPropertyValue("LastName")}"

• If FirstName or LastName may be empty then you're adding a spurious space. Handle the case (or at least add a call to Trim().)
• true boolean parameters are a pain, I see the call and I need to go to check the prototype to know what it means. More than that: if I need another option then I need to add more and more parameters.

Use named parameters if you can't change the prototype (name is fictional in this case):

profile.Initialize(usr.UserName, loadUserProfile: true);

Or, better, use an enum with the [Flags] attribute to make clear - at calling point - what parameter means.

• You initialised Profile with the user name as parameter in its ctor, do you need it again here?

• If lst is local then you can simplify your code.

Move the logic to create group into a separate function (basically all the code you have inside the parallel loop) then:

var groups = userlist.AsParallel()
    .Select(CreateGroup)
    .ToList();

Where CreateGroup is something like this (note that I moved the hard-coded user string to a constant defined elsewhere and I moved the possibly complex logic for DisplayName to a separate function):

UserAndGroup CreateGroup(MembershipUser user)
{
    var profile = new Profile(user.UserName);
    profile.Initialize(user.UserName, true);

    return new UserAndGroup
    {
        Id = user.ProviderUserKey.ToString(),
        Name = user.UserName,
        Type = UserGroupType,
        DisplayName = ResolveDisplayName()
    };

    string ResolveDisplayName()
        => $"{profile.GetPropertyValue("FirstName")} {profile.GetPropertyValue("LastName")}";
}