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 service for that — Roman Marusyk
– Roman Marusyk, Commented Dec 9, 2015 at 17:01
Is that last one supposed to be Convert.ToBoolean(item["IsPublic"])? It doesnt match all the others — Jamiec
– Jamiec, Commented Dec 9, 2015 at 17:03
what about put those checking and setting default value into your model setter. It's cleaner in my opinion — TLJ
– TLJ, Commented Dec 9, 2015 at 17:09
@HankMooody: If you are indeed going with the Reflection approach (indicated by your accepted answer), this link about Reflection in C# would be useful. — displayName
– displayName, Commented Dec 14, 2015 at 21:18

Timon · Accepted Answer · 2015-12-14 13:22:41Z

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, 2015 at 13:22

answered Dec 9, 2015 at 17:27

Timon

1,0131 gold badge10 silver badges40 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Hank Mooody Over a year ago

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.

Timon Over a year ago

Forgot to add the GetType(). You shouldn't get this error anymore now, i've changed my answer.

Hank Mooody Over a year ago

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 Over a year ago

Found the bug.Thank you :D

Roman Marusyk · Accepted Answer · 2015-12-09 17:04:52Z

1

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;
}

edited Dec 9, 2015 at 17:04

answered Dec 9, 2015 at 17:03

Roman Marusyk

24.8k27 gold badges83 silver badges127 bronze badges

1 Comment

Jamiec Over a year ago

I think the point was to avoid the repetition, rather than just shorten the code with ternary operators

Community · Accepted Answer · 2017-05-23 12:23:13Z

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).

displayName · Accepted Answer · 2015-12-09 17:21:21Z

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;
}

davidallyoung · Accepted Answer · 2015-12-09 19:25:12Z

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;
}

Collectives™ on Stack Overflow

Refactoring foreach loop with several of if-else statements

5 Answers 5

4 Comments

1 Comment

Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

4 Comments

1 Comment

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related