Wie kann ich diese hässliche if-Anweisung aufräumen?

Question

Ich habe folgende hässlich if-Anweisung, die Teil einer Klasse ist, die von einem IOC-Container gezogen wird:

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

Mein normaler Ansatz zur Beseitigung von if-Anweisungen ist eine Unterklasse für jede Bedingung zu erstellen.

Was anders an diesem Beispiel ist:

1. Die Klasse, die diese Methode hat, wird von der IOC-Container gezogen.
2. Ich möchte vielleicht die Logik zwischen den 2 if-Anweisungen zu laufen oder gar nicht.

Jeder Rat sehr willkommen.

Answer 1

Wie wäre es mit zwei Extrakt Methoden und invertieren die Wachen der, wenn sein:

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: Die beiden Methoden müssen mit der richtigen Art von IBuildMetaData.RepositoryElementList und IBuildMetaData.ExportList fixiert werden.

Answer 2

Ich bin mir nicht wirklich sicher, warum Sie die if-Anweisungen beseitigen möchten - und die Verwendung der Vererbung dafür scheint übertrieben. Vielleicht möchten Sie eine Erweiterungsmethode für die wiederholte Sammlungscode erstellen:

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

Auf diese Weise können Sie die Bedingungen zu ändern:

if (buildMetaData.RepositoryElementList.HasElements()) 

und

if (buildMetaData.ExportList.HasElements()) 

die etwas IMO einfacher ist. Wenn es noch mehr Logik gibt, möchten Sie vielleicht auch die beiden Blöcke in verschiedene Methoden aufteilen. Ansonsten würde ich mir keine Sorgen machen.

Oh, und eine weitere Erweiterung Methode, die, wenn Sie zu kümmern brauchen wird nicht helfen, ob Sie Elemente haben, aber hilft, wenn Sie nur ein Null-safe foreach tun wollen:

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

(nicht, dass es die null-Koaleszenz-Operator inline über Verwendung, zugegebenermaßen sehr viel spart ...)

Answer 3

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 4

Ich würde entweder eine Erweiterungsmethode wie John vorgeschlagen, oder entweder (wenn dies im Zusammenhang mit dem Rest der Klasse sinnvoll ist) es umgestalten und buildMetaData ein Mitglied der Klasse machen und zwei private bool Eigenschaften mit einem haben Bekomme das maskiert deine Dualklausel wenn Aussage mit etwas besser lesbar ist.

In jedem Fall führt das gleiche Ergebnis, um es lesbarer zu machen.