Refactoring foreach loop with several of if-else statements

Question

I've got this method where I'm inspecting several times if some field of SPListItem is null and if it is then write default value for that property. Is there any way that I can reduce this code? Thank you

public List<Models.EmployeeInfo> GetEmployeeInfo(SPListItemCollection splic)
{           
    List<Models.EmployeeInfo> listEmployeeInfo = new List<Models.EmployeeInfo>();

    foreach (SPListItem item in splic)
    {               
        var employeeInfo = new Models.EmployeeInfo();

        if (item["EmployeeName"] == null)
        {
            employeeInfo.EmployeeName = "";
        }
        else
        {
            employeeInfo.EmployeeName = item["EmployeeName"].ToString();
        }

        if (item["Position"] == null)
        {
            employeeInfo.Position = "";
        }
        else
        {
            employeeInfo.Position = item["Position"].ToString();
        }

        if (item["Office"] == null)
        {
            employeeInfo.Office = "";
        }
        else
        {
            employeeInfo.Office = item["Office"].ToString();
        }

        if (item["IsPublic"] == null)
        {
            employeeInfo.IsPublic = true;
        }
        else
        {
            employeeInfo.IsPublic = Convert.ToBoolean("IsPublic"); 
        }

        listEmployeeInfo.Add(employeeInfo);
    }

    return listEmployeeInfo;                                           
}

I think that you should try to use [CodeReview](http://codereview.stackexchange.com/) service for that — Roman Marusyk, Dec 09 '15 at 17:01
Is that last one supposed to be `Convert.ToBoolean(item["IsPublic"])`? It doesnt match all the others — Jamiec, Dec 09 '15 at 17:03
what about put those checking and setting default value into your model setter. It's cleaner in my opinion — TLJ, Dec 09 '15 at 17:09
@HankMooody: If you are indeed going with the *Reflection approach* (indicated by your accepted answer), this [link about Reflection in C#](http://stackoverflow.com/questions/1458256/why-is-the-use-of-reflection-in-net-recommended) would be useful. — displayName, Dec 14 '15 at 21:18
@HankMooody: Also see [How costly is .NET reflection?](http://stackoverflow.com/questions/25458/how-costly-is-net-reflection) — displayName, Dec 14 '15 at 21:53
yeah, I see now that reflection is bad practice when there are some nested loops.Thank you .I'll find a better approach to this problem. — Hank Mooody, Dec 14 '15 at 22:43

Roman Marusyk · Answer 1 · 2015-12-09T17:04:52.800

Try to something like:

public List<Models.EmployeeInfo> GetEmployeeInfo(SPListItemCollection splic)
{

  var listEmployeeInfo = new List<Models.EmployeeInfo>();
  foreach (SPListItem item in splic)
  {               
    var employeeInfo = new Models.EmployeeInfo();

    employeeInfo.EmployeeName = item["EmployeeName"] == null ? "" : item["EmployeeName"].ToString();

    employeeInfo.Position = item["Position"] == null ? "" : item["Position"].ToString();
    employeeInfo.Office = item["Office"] == null ? "" : item["Office"].ToString();

    employeeInfo.IsPublic = item["IsPublic"] == null || Convert.ToBoolean("IsPublic");

    listEmployeeInfo.Add(employeeInfo);
  }

  return listEmployeeInfo;
}

I think the point was to avoid the repetition, rather than just shorten the code with ternary operators — Jamiec, Dec 09 '15 at 17:04

Timon · Accepted Answer · 2015-12-14T13:22:41.413

1

You could use some reflection to set the property. Then you can loop over a list of all the propertynames and set them. ( This way when a property gets added to the model, all you need to do is add it to the list of strings )

public List<Models.EmployeeInfo> GetEmployeeInfo(SPListItemCollection splic)
{
    var listEmployeeInfo = new List<Models.EmployeeInfo>();
    var propertyNames = new List<string>(){"EmployeeName","Position","Office","IsPublic"}

    foreach (SPListItem item in splic)
    {
        var employeeInfo = new Models.EmployeeInfo(); 

        foreach (var propertyName in propertyNames)
        {  
            string newData = "";
            if (item[propertyName] != null)
            {
                newData = item[propertyName];
            }
            employeeInfo.GetType().GetProperty(propertyName).SetValue(employeeInfo, newData, null); 
        }

        listEmployeeInfo.Add(employeeInfo);
    }
    return listEmployeeInfo;
}

edited Dec 14 '15 at 13:22

answered Dec 09 '15 at 17:27

Timon

1,003
1
9
38

This is really interesting approach and I want to use it, but I'm not familiar with Reflections , please explain me what should I do , because I can't use GetProperty extension method , error says that my class doesn't contain a definition for GetProperty extension method. Thank you. – Hank Mooody Dec 14 '15 at 10:46
Forgot to add the GetType(). You shouldn't get this error anymore now, i've changed my answer. – Timon Dec 14 '15 at 13:24
I've figure that out , thank you .Now I'm having other error, at that line, "Object reference not set to an instance of an object" – Hank Mooody Dec 14 '15 at 13:27
Found the bug.Thank you :D – Hank Mooody Dec 14 '15 at 13:51

score 0 · Answer 3 · edited May 23 '17 at 12:23

Try refactoring the common logic into a function.

employeeInfo.EmployeeName = ConditionalToString(item, "EmployeeName");
employeeInfo.Position = ConditionalToString(item, "Position");
employeeInfo.Office = ConditionalToString(item, "Office");
employeeInfo.IsPublic = item[attrName] == null ? false : Convert.ToBoolean("IsPublic");

string ConditionalToString(SPListItem item, string attrName)
{
    return (item[attrName] == null ? "" : item[attrName].ToString());
}

The null coalesce operator won't work since item[attrName] and "" are different types, so something like this wouldn't work: (item[attrName] ?? "").ToString() (would dynamic be helpful in this case? I don't use it often).

TLJ's comment is an alternative solution for where this logic can take place (although you will still have the same repetition there).

score 0 · Answer 4 · answered Dec 09 '15 at 17:21

I agree with the other answer given here which uses ternary operator. Strangely I was studying this same thing yesterday. You can and should use ternary operator here instead of if - else.

Advantages?

For one, it'll make the code shorter for you to see in one glance.
The bigger advantage is this... your current code is statement based. So what you are doing is that you are testing a condition and executing statements as a side-effect of that condition. But when using ternary operator you are using expression to compute results (which is exactly what you are trying to do - you are trying to generate your employeeInfo object to put in a list).
In your current design for each attribute you have 2 places where its value is assigned (the if block and the else block). When using ternary operator, the values are being assigned at one place only.

Further, you can refactor the employeeInfo object creation to another method and keep the current method cleaner (like shown below):

public List<Models.EmployeeInfo> GetEmployeeInfo(SPListItemCollection splic)
{
    var listEmployeeInfo = new List<Models.EmployeeInfo>();
    foreach (SPListItem splicItem in splic)
    {               
      listEmployeeInfo.Add(CreateEmployeeInfoFromItem(splicItem));
    }
    return listEmployeeInfo;
}

private static Models.EmployeeInfo CreateEmployeeInfoFromItem(SPListItem item)
{
    var employeeInfo = new Models.EmployeeInfo();
    employeeInfo.EmployeeName = item["EmployeeName"] == null ? "" : item["EmployeeName"].ToString();
    employeeInfo.Position = item["Position"] == null ? "" : item["Position"].ToString();
    employeeInfo.Office = item["Office"] == null ? "" : item["Office"].ToString();
    employeeInfo.IsPublic = item["IsPublic"] == null || Convert.ToBoolean("IsPublic");
    return employeeInfo;
}

score 0 · Answer 5 · answered Dec 09 '15 at 19:25

I would consider creating a mapping object that has the sole responsibility of creating an EmployeeInfo instance from a given instance of SPListItem. In this mapping object you would have your validation criteria/setting criteria, and then any time you have this need come up you have a nice mapping object ready to do the job.

public class SPListItemToEmployeeInfoMapper
{
    public static Models.EmployeeInfo Map(SpListItem item);
    { //your logic here to create the employeeinfo from SpListItem }
}

Then your caller:

public List<Models.EmployeeInfo> GetEmployeeInfo(SPListItemCollection splic)
{
   var listEmployeeInfo = new List<Models.EmployeeInfo>();
   foreach (SPListItem splicItem in splic)
   {               
      listEmployeeInfo.Add(SPListItemToEmployeeInfoMapper.Map(splicItem));
   }
   return listEmployeeInfo;
}

Refactoring foreach loop with several of if-else statements

5 Answers5