Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a class like this one:

class EmailClass 
{
    public string MailAdresse { get; set; }
    public string MailAdresseCC { get; set; }
}

Through a JSON deserialization I obtain a List<EmailClass> and two other strings anotherMailAddress and justAnotherMailAddress.

I need to join all those adresses in one big string (the separator is ";"). For this purpose I wrote this code:

// this one is populated in another section but it 
// is put there for the sake of knowing the  variable name
List<EmailClass> splittedList

List<string> listOfAdresses = new List<string>();
string joinedAdresses = String.Empty;

listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresse));
listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresseCC));
listOfAdresses.Add(anotherMailAddress);
listOfAdresses.Add(justAnotherMailAdresses);

joinedAdresses = String.Join(";", listOfAdresses.Distinct().ToArray());

Now, is there a more polished way to obtain this result? How much will Distinct() affect the performance on medium result set (~100.000 adresses)? I've read that the complexity of the function should be O(n), is this true?

share|improve this question

2 Answers 2

up vote 12 down vote accepted

Use a HashSet then it will only contain unique values, duplicates will be discarded when calling Add based upon the hashcode of the string. This should be far more efficient than calling Distinct.

List<EmailClass> splittedList

HashSet<string> listOfAdresses = new HashSet<string>();

// HashSet does not contain an AddRange method.
foreach (var emailClass in splittedList)
{
    listOfAdresses.Add(emailClass.MailAdresse);
    listOfAdresses.Add(emailClass.MailAdresseCC);
}

listOfAdresses.Add(anotherMailAddress);
listOfAdresses.Add(justAnotherMailAdresses);

string joinedAdresses = String.Join(";", listOfAdresses.ToArray());

--

Following the "feedback" from Almaz, here's a basic benchmark to show the performance difference (using unique values for each address):

private static void Main(string[] args)
{
    List<EmailClass> splittedList = Enumerable.Range(1, 100000).Select(i => new EmailClass
    {
        MailAdresse = i.ToString() + "@email.com",
        MailAdresseCC = i.ToString() + "cc@email.com"
    }).ToList();

    OriginalMethod(splittedList);
    HashSetMethod(splittedList);
    LinqMethod(splittedList);

    Console.ReadLine();
}

private static void OriginalMethod(List<EmailClass> splittedList)
{
    var sw = new Stopwatch();
    sw.Start();
    List<string> listOfAdresses = new List<string>();

    listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresse));
    listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresseCC));
    listOfAdresses.Add("someone@email.com");
    listOfAdresses.Add("someone.else@email.com");
    var joinedAdresses = String.Join(";", listOfAdresses.Distinct().ToArray());
    sw.Stop();
    Console.WriteLine("OriginalMethod");
    Console.WriteLine(sw.Elapsed);
}

private static void HashSetMethod(List<EmailClass> splittedList)
{
    var sw = new Stopwatch();
    sw.Start();
    HashSet<string> listOfAdresses = new HashSet<string>();

    foreach (var emailClass in splittedList)
    {
        listOfAdresses.Add(emailClass.MailAdresse);
        listOfAdresses.Add(emailClass.MailAdresseCC);
    }

    listOfAdresses.Add("someone@email.com");
    listOfAdresses.Add("someone.else@email.com");

    string joinedAdresses = String.Join(";", listOfAdresses.ToArray());
    sw.Stop();
    Console.WriteLine("HashSetMethod");
    Console.WriteLine(sw.Elapsed);
}

private static void LinqMethod(List<EmailClass> splittedList)
{
    var sw = new Stopwatch();
    sw.Start();
    var emails = splittedList.SelectMany(emailClass => new[] { emailClass.MailAdresse, emailClass.MailAdresseCC })
        .Concat(new[] { "someone@email.com", "someone.else@email.com" })
        .Distinct();

    var joinedAdresses = String.Join(";", emails);
    sw.Stop();
    Console.WriteLine("LinqMethod");
    Console.WriteLine(sw.Elapsed);
}

Results run in release mode without debugger attached:

OriginalMethod: 00:00:00.0789540
HashSetMethod:  00:00:00.0488568
LinqMethod:     00:00:00.0668056

Ramping up to 1,000,000 items

OriginalMethod: 00:00:00.8189667
HashSetMethod:  00:00:00.6891028
LinqMethod:     00:00:01.0157357

As you can see, the HashSet approach is substantially faster than the OP and significantly faster than the Linq approach.

share|improve this answer
    
Actually Distinct does almost the same, so your solution won't be faster then pure iteration over ".Distinct". See my answer. –  almaz Nov 21 '12 at 17:26
    
Won't the hash set mess up the order of the strings? –  jpfollenius Nov 25 '12 at 11:24

Solution can be optimized even further in terms of readability, with the same level of performance as "manually create a HashSet and populate it with items":

var emails = splittedList.SelectMany(emailClass => new[] { emailClass.MailAdresse, emailClass.MailAdresseCC})
    .Concat(new[] {anotherMailAddress, justAnotherMailAdress})
    .Distinct();
var joinedAdresses = String.Join(";", emails);
share|improve this answer
2  
You can argue readability either way, fewer lines of code does not necessarily mean more readable. Also, this method will result in creating an array for every item in the source list which is completely unnecessary! –  Trevor Pilley Nov 21 '12 at 18:54
    
you can always do .Select(emailClass=>emailList.MailAdresse).Concat(splittedList.Select(emailClass‌​=>emailList.MailAdresseCC)) to avoid creating arrays... –  almaz Nov 21 '12 at 20:13
    
You could, but then you have to enumerate splittedList twice. Once to select the EmailAddresse and a second time to select the EmailAddresseCC which isn't an optimisation either as you're doing twice as much work! Linq isn't the answer to everything, just because you can use it, doesn't mean you should :) –  Trevor Pilley Nov 21 '12 at 20:40
    
I suggested another approach that (on my opinion) is more readable, expressive and compact. It declares what you want to do instead of how you want it to be done. Optimizing microseconds here is just a premature optimization, no-one will ever build a string containing a million of emails. –  almaz Nov 22 '12 at 9:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.