Problem
Is there any way to refactor this?
public IEnumerable<Option> Options
{
get
{
{
List<Option> ListOption = new List<Option>();
if (!String.IsNullOrEmpty(Option1))
{
ListOption.Add(new Option() {Name=Option1 });
}
if (!String.IsNullOrEmpty(Option2))
{
ListOption.Add(new Option() { Name = Option2 });
}
if (!String.IsNullOrEmpty(Option3))
{
ListOption.Add(new Option() { Name = Option3 });
}
if (!String.IsNullOrEmpty(Option4))
{
ListOption.Add(new Option() { Name = Option4 });
}
return ListOption;
}
}
}
public class Option
{
public string Name { get; set; }
}
Solution
This is a bit neater:
List<Option> ListOption = new List<Option> { };
Action<string> add = x => { if (!String.IsNullOrEmpty(x)) ListOption.Add(new Option { Name = x }); }
add(Option1);
add(Option2);
add(Option3);
add(Option4);
return ListOption;
This is perhaps even better:
return (new string[] {Option1, Option2, Option3, Option4})
.Where(x => !String.IsNullOrEmpty(x))
.Select(x => new Option { Name = x })
.ToList();
This is a good opportunity to use the yield
operator:
public IEnumerable<Option> Options
{
get
{
if (!string.IsNullOrEmpty(Option1)) yield return new Option{Name=Option1};
if (!string.IsNullOrEmpty(Option2)) yield return new Option{Name=Option2};
if (!string.IsNullOrEmpty(Option3)) yield return new Option{Name=Option3};
if (!string.IsNullOrEmpty(Option4)) yield return new Option{Name=Option4};
}
}
Just to be kind of outrageous, here’s a version using a standard looping mechanism:
static List<Option> GetNonNullOptions(params string[] options)
{
var results = new List<Option>();
if (options != null)
{
foreach (var option in options)
{
if (option != null)
{
results.Add(new Option { Name = option });
}
}
}
return results;
}
Leaving you to do something like this:
var nonNullOptions = GetNonNullOptions(Option1, Option2, Option3, Option4);
Notice that I only check for null value items, as an empty string and a null value are two different things.