Yet Another ugly if statement question in C#

Question

Hi,

I have the following ugly if statement that is part of a class that is pulled from an IOC container:

protected virtual void ExecuteSourceControlGet(IBuildMetaData buildMetaData, IPackageTree componentTree)
{
    if ((buildMetaData.RepositoryElementList != null) && (buildMetaData.RepositoryElementList.Count > 0))
    {
        componentTree.DeleteWorkingDirectory();

        foreach (var repositoryElement in buildMetaData.RepositoryElementList)
        {
            repositoryElement.PrepareRepository(componentTree, get).Export();
        }

    }

    if((buildMetaData.ExportList != null) && (buildMetaData.ExportList.Count > 0))
    {
        var initialise = true;

        foreach (var sourceControl in buildMetaData.ExportList)
        {
            log.InfoFormat("\nHorn is fetching {0}.\n\n".ToUpper(), sourceControl.Url);

            get.From(sourceControl).ExportTo(componentTree, sourceControl.ExportPath, initialise);

            initialise = false;
        }

    }

    log.InfoFormat("\nHorn is fetching {0}.\n\n".ToUpper(), buildMetaData.SourceControl.Url);

    get.From(buildMetaData.SourceControl).ExportTo(componentTree);
}

My normal approach to eliminating if statements is to create a subclass for each condition.

What is different about this example is:

1. The class that has this method is pulled from the IOC container.
2. I might want the logic in between the 2 if statements to run or not at all.

Any advice greatly welcomed.

Cheers

Paul

Answer 1

I'm not really sure why you'd want to eliminate the if statements - and using inheritance for it seems over the top. You might want to create an extension method for the repeated collection code:

public static bool HasElements<T>(this ICollection<T> collection)
{
    return collection != null && collection.Count != 0;
}

This lets you change the conditions to:

if (buildMetaData.RepositoryElementList.HasElements())

and

if (buildMetaData.ExportList.HasElements())

which is slightly simpler IMO. If there ends up being any more logic you might also want to separate the two blocks into different methods. Other than that, I wouldn't worry.

Oh, and another extension method which won't help if you need to care whether or not you have elements, but will help if you just want to do a null-safe foreach:

public static IEnumerable<T> EmptyIfNull<T>(this IEnumerable<T> source)
{
    return source ?? Enumerable.Empty<T>();
}

(Not that it saves very much over using the null coalescing operator inline, admittedly...)

Answer 2

protected virtual void ExecuteSourceControlGet
    (IBuildMetaData metaData, IPackageTree tree)
{
    if (metaData.RepositoryElementList.HasElements())
    {
        tree.DeleteWorkingDirectory();

        foreach (var element in metaData.RepositoryElementList)
             element.PrepareRepository(tree, get).Export();    
    }

    if(metaData.ExportList.HasElements())
    {
        var init = true;

        foreach (var sourceControl in metaData.ExportList)
        {
            log.InfoFormat
           ("\nHorn is fetching {0}.\n\n".ToUpper(), sourceControl.Url);

            get.From(sourceControl)
            .ExportTo(tree, sourceControl.ExportPath, init);

            init = false;
        }

    }

    log.InfoFormat
       ("\nHorn is fetching {0}.\n\n".ToUpper(), 
           metaData.SourceControl.Url);

    get.From(metaData.SourceControl)
       .ExportTo(tree);
}

//Credits goes to Jon Skeet...
public static class CollectionExtensions
{
    public static bool HasElements<T>(this ICollection<T> collection)
    {
        return collection != null && collection.Count != 0;
    }
}

Answer 3

How about using two extract methods, and invert the if's to be guards:

protected virtual void ExecuteSourceControlGet(IBuildMetaData buildMetaData, IPackageTree componentTree)
{
   ExecuteRepositoryElementList(buildMetaData.RepositoryElementList, componentTree);

   ExecuteExportList(buildMetaData.ExportList, componentTree);

   log.InfoFormat("\nHorn is fetching {0}.\n\n".ToUpper(), buildMetaData.SourceControl.Url);

   get.From(buildMetaData.SourceControl).ExportTo(componentTree);
}

private void ExecuteRepositoryElementList(RepositoryElementList repositoryElements, IPackageTree componentTree)
{
   // Guard: No elements
   if (repositoryElements == null || repositoryElements.Count == 0) return;

   componentTree.DeleteWorkingDirectory();

   foreach (var repositoryElement in repositoryElements)
   {
      repositoryElement.PrepareRepository(componentTree, get).Export();
   }
}

private void ExecuteExportList(ExportList exportList, IPackageTree componentTree)
{
   // Guard: No elements
   if(exportList == null || exportList.Count == 0) return;

   var initialise = true;

   foreach (var sourceControl in exportList)
   {
      log.InfoFormat("\nHorn is fetching {0}.\n\n".ToUpper(), sourceControl.Url);

      get.From(sourceControl).ExportTo(componentTree, sourceControl.ExportPath, initialise);

      initialise = false;
   }
}

BTW: The two methods must be fixed with the correct type of IBuildMetaData.RepositoryElementList and IBuildMetaData.ExportList.

Answer 4

I would either create an extension method as John suggested, or either (if this makes sense in the context of the rest of your class) refactor it and make buildMetaData a member of the class and have two private bool properties with a get that masks your dual clause if statement with something more readable.

Either way accomplishes the same result of making it more readable.