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.

This is my constructor to transform an Outlook contact into a Dynamics CRM contact:

public Contact(ContactItem contactItem)
{
    LogicalName = EntityLogicalName;
    FirstName = contactItem.FirstName;
    LastName = contactItem.LastName;
    //if no last name, use the first name
    if (String.IsNullOrEmpty(LastName))
    {
        LastName = contactItem.FirstName;
        //if no First name, use the Full Name.
        if (String.IsNullOrEmpty(LastName))
        {
            LastName = contactItem.FullName;
            //if no last name or first name, use the company name
            if (String.IsNullOrEmpty(LastName))
            {
                LastName = contactItem.CompanyName;
                //If no last name, no first name and no company name, use the email address.
                if (String.IsNullOrEmpty(LastName))
                {
                    LastName = contactItem.Email1Address;
                    //if no Last name, no first name, no company name, no email address, default to naam niet bekend.
                    if (String.IsNullOrEmpty(LastName))
                    {
                        LastName = @"Naam niet bekend";
                    }
                }
            }
        }
        else
        {
            FirstName = "";
        }
    }

    Address1_City = contactItem.BusinessAddressCity;
    Address1_Country = contactItem.BusinessAddressCountry;
    Address1_Line1 = contactItem.BusinessAddressStreet;
    Address1_PostalCode = contactItem.BusinessAddressPostalCode;
    Telephone1 = contactItem.BusinessTelephoneNumber;
    Telephone2 = contactItem.HomeTelephoneNumber;
    Fax = contactItem.BusinessFaxNumber;
    Address2_Line1 = contactItem.HomeAddressStreet;
    Address2_City = contactItem.HomeAddressCity;
    Address2_PostalCode = contactItem.HomeAddressPostalCode;
    Address2_Country = contactItem.HomeAddressCountry;
    EMailAddress1 = contactItem.Email1Address;
    EMailAddress2 = contactItem.Email2Address;
    EMailAddress3 = contactItem.Email3Address;
}

As you can see, I got 5 nested ifs to determine what I should use for LastName. I don't like how it looks, but the only other option I can see is putting them in sequence instead of nested. However, that would mean that instead of usually having only 1-3 ifs, it would be forced into doing 5 ifs every single time. during tests with 1800 records, it already felt noticeably slower if it got to the literal string.

Is there some really easy way (or some really obscure way) of doing this which I missed?

share|improve this question
3  
You should avoid pre-mature optimization at the expense of readability until it's absolutely required. Then you should measure to see what your really slow parts are and address those first. (If a simple string null check is your biggest problem, then I applaud you for your pure awesome). –  Mitchell Lee Apr 10 at 11:50
    
IFs are lightning fast, null check and zero length checks are lightning fast, how could it be possible to perceive slowness over this??. In any case, regardless the implementation you should create a DisplayName property with only get and put the logic there. –  Leopoldo Salvo Apr 10 at 12:43
    
When i'm referring to "noticeably slower", I meant that during the creation of 1800 contacts in-memory, the " X contacts created" console statements scrolled by at an approximate rate of 100-150 per second instead of 200 per second. it still was done quite fast (inserting them into CRM took quite a lot more time using a simple foreach(){context.addobject()} followed by a savechanges()), but it felt like it went somewhat slower. –  Nate Kerkhofs Apr 10 at 13:19

4 Answers 4

up vote 10 down vote accepted
  1. You could create a helper function with a loop:

    public string getFirstNonNullOrEmpty(params string[] names) {
        foreach (string name in names) {
            if (String.IsNullOrEmpty(name)) {
                continue;
            }
            return name;
        }
        return @"Naam niet bekend";
    }
    

    Usage:

    LastName = getFirstNonNullOrEmpty(contactItem.LastName, contactItem.FirstName, 
        contactItem.FullName, contactItem.CompanyName, contactItem.Email1Address);
    

    You might also need this because of the else branch:

    if (String.IsNullOrEmpty(contactItem.LastName)) 
        && !String.IsNullOrEmpty(contactItem.FirstName)) {
        FirstName = "";
    }
    
  2. You might want to use IsNullOrWhiteSpace instead of IsNullOrEmpty.

(I'm not too familiar with C# and don't have a C# compiler right now so the syntax might be invalid.)

share|improve this answer
4  
You could avoid that continue and simplify the code a bit by negating the condition: if (!String.IsNullOrEmpty(name)) { return name; }. –  svick Apr 10 at 12:40
    
@svick: Thanks for the edit! –  palacsint Apr 10 at 13:07
    
@svick: I know that but I like guard clauses. –  palacsint Apr 10 at 13:07
3  
using Linq body of getFirstNonNullOrEmpty can be changed to return names.SkipWhile(String.IsNullOrEmpty).FirstOrDefault() ?? @"Naam niet bekend"; –  abuzittin gillifirca Apr 10 at 13:44
  1. Contact and ContactItem should in some way be child and parent, or at-least share a common ancestor so that you don't have to manually copy over all that identical contact information.

  2. I'm not certain, but I think you might have wanted all this to be done with the LogicalName and not the LastName of this contact.

  3. You should be using string.IsNullOrWhiteSpace in-case these strings haven't been trimmed or something.

  4. As for simplifying your nest of ifs there are two solutions that fit the program as is.

    1. Use a less ugly ternary statement to do exactly what you were doing before

      if(string.IsNullOrWhiteSpace(LastName))
      {
          LastName = 
              !String.IsNullOrEmpty(contactItem.FirstName) ? contactItem.FirstName : 
              !String.IsNullOrEmpty(contactItem.FullName) ? contactItem.FullName : 
              !String.IsNullOrEmpty(contactItem.CompanyName) ? contactItem.CompanyName : 
              !String.IsNullOrEmpty(contactItem.Email1Address) ? contactItem.Email1Address : 
              @"Naam niet bekend";
          if(LastName == FirstName)
              FirstName = string.Empty;
      }
      
    2. Using Linq: I created a list of all the possible values that you considered valid, then calling the First() Extension method on our string array, would by default return just the first item in our array. By passing in a lambda expression I override that behavior to return the first value which matches my condition, which is !string.IsNullOrWhiteSpace()

      if (string.IsNullOrWhiteSpace(LastName))
      {
          LastName = new string[] { contactItem.FirstName, contactItem.FullName, contactItem.CompanyName, contactItem.Email1Address, @"Naam niet bekend" }
              .First(str => !string.IsNullOrWhiteSpace(str));
          if (LastName == FirstName)
              FirstName = string.Empty;
      }
      
  5. If you were certain that all your empty strings were actually null instead of empty strings you could have just used the null-coalescing operator ??, which returns the first side of the argument which proves to not be null while moving from left to right. In this case, your code would have looked like this.

    Note: To make a string null use string str = default(string);

    if (string.IsNullOrWhiteSpace(LastName))
    {
        LastName = contactItem.FirstName ?? contactItem.FullName ?? contactItem.CompanyName ?? contactItem.Email1Address ?? @"Naam niet bekend";
        if (LastName == FirstName)
            FirstName = string.Empty;
    }
    
  6. The code I posted in 4.1 is the most efficient because it only does as many comparisons as is needed, and cuts out when it finds the correct value. One advantage 4.1 has over the code you posted is that it doesn't do a string assignment before it does the comparison. Your code could have been re-written to avoid that as-well.

    The code in 4.2 is sleek, however it does require you to create an array, which is not expensive at all.

    I am only mentioning this, because it is something to consider when choosing the right code for you. While the differences here may not even be noticeable by the computer its self in terms of run-time speed, efficiency is something to keep in mind at all times when coding.

share|improve this answer
    
I can already answer to the first item that that's just not relevant because they're from entirely different software: ContactItem is from Microsoft Outlook 2013 and Contact is from Microsoft Dynamics CRM 2013. I had to write a custom import tool because I couldn't use the CRM Outlook connector. And there's a 1 to 1 relationship between the 2, but 1 is basically a COM-object while the other is derived from a CRM-specific Object called Entity. LogicalName is unrelated to LastName, instead indicating what implementation of the Entity object we're talking about (in this case Contact). –  Nate Kerkhofs Apr 10 at 14:09
    
@NateKerkhofs I completely understand... Working with foreign APIs always does this to me too. –  BenVlodgi Apr 10 at 14:11

Take advantage of inline assignment and the short-circuiting behavior of the && operator. This code has the same behavior as your original, but it collapses the innermost three if-statements together into one:

if (String.IsNullOrEmpty(LastName))
{
    if (String.IsNullOrEmpty(LastName = contactItem.FirstName))
    {
        if (String.IsNullOrEmpty(LastName = contactItem.FullName)
            && String.IsNullOrEmpty(LastName = contactItem.CompanyName)
            && String.IsNullOrEmpty(LastName = contactItem.Email1Address))
        {
            LastName = @"Naam niet bekend";
        }
    }
    else
    {
        FirstName = "";
    }
}
share|improve this answer

You could reverse the condition, and build an if - else - if ladder. Only the worst case scenario would have 5 evaluations.

...
    if (String.IsNullOrEmpty(LastName))
    {
         if (!string.IsNullOrEmpty(contactItem.FirstName))
         {
                LastName = ...
         }
         else if (!string.IsNullOrEmpty(contactItem.FullName))
         {
                LastName = ...
         }
         else if (!string.IsNullOrEmpty(contactItem.CompanyName))
         {
                LastName = ...
         }
         else if (!string.IsNullOrEmpty(contactItem.Email1Address))
         {
                LastName = ...
         }
         else
         {
                LastName = ...
         }
    }
...
share|improve this answer
    
That's really similar to what I currently have, because I also currently have 5 evaluations in worst-case. –  Nate Kerkhofs Apr 10 at 11:47
    
true, but the nesting stays at one level, and indentation doesn't move further right. I find it personally more readable. It approaches "switch clarity". –  Willem van Rumpt Apr 10 at 11:49
    
You could of course build a list with all the potential "LastName" values in order of priority, and use a FirstOrDefault(s => !string.IsNullOrEmpty(s)), but I think it goes a bit too far. But maybe it's something? –  Willem van Rumpt Apr 10 at 11:51

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.