13
\$\begingroup\$

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; }
}
\$\endgroup\$
33
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ I personally find this hard to read and pretty long winded. I think a simple loop as per @Grant Thomas answer below might be more appropriate here. \$\endgroup\$ – MemeDeveloper May 11 at 17:35
11
\$\begingroup\$

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};
    }
}
\$\endgroup\$
8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ YES these days actually writing a foreach in C# is rightfully frowned upon and should clearly NEVER be done (it's not like looping is a core language construct for a reason). We should all use linq for absolutely everything, all the time ;) haha +1 \$\endgroup\$ – MemeDeveloper May 11 at 17:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.