CODEWITHSUNDEEP
c#
Hank Mooody
Hank Mooody

Reputation: 527

Refactoring foreach loop with several of if-else statements

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

Upvotes: 2

Views: 960

Answers (5)

Timon
Timon

Reputation: 1013

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

Upvotes: 1

davidallyoung
davidallyoung

Reputation: 1332

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

Upvotes: 0

displayName
displayName

Reputation: 14379

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

Upvotes: 0

Josh Grosso
Josh Grosso

Reputation: 2114

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

Upvotes: 0

Roman Marusyk
Roman Marusyk

Reputation: 24579

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

Upvotes: 1

Related Questions