Methoden auslagern um die Verschachtelung zu verringern

Anhand einem echten, wenn auch altem Beispiel von Code aus dem Majesty ERP der majesty GmbH, möchte ich hier im folgenden einen kleine Schulung zum Refactoring erstellen. Hier der C# Code, mit dem wir starten:

public static ILagerbuchung GetUrsprungsLagerbuchung(IQSPruefung xQSPruefung, bool xKompletterUrsprung = true)
{
    var idHerkunft = xQSPruefung.IDWareneingangposition;
    if (idHerkunft == Guid.Empty)
    {
        idHerkunft = xQSPruefung.IDWareneingangKundePosition;
        if (idHerkunft == Guid.Empty)
        {
            idHerkunft = xQSPruefung.IDBetriebsauftragFertigmeldung;
            if (idHerkunft == Guid.Empty)
            {
                idHerkunft = xQSPruefung.IDManuelleLagerbuchung;
                if (idHerkunft == Guid.Empty)
                {
                    idHerkunft = xQSPruefung.IDLagerbuchung;
                    if (idHerkunft != Guid.Empty)
                    {
                        if (xKompletterUrsprung)
                        {
                            var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
                            while (lagbuch != null && lagbuch.HerkunftObjectType == typeof(BO_Lagerbuchung).FullName)
                            {
                                idHerkunft = lagbuch.IDHerkunft;
                                lagbuch = BO_Lagerbuchung.Load(idHerkunft);
                            }
 
                            if (lagbuch != null)
                                idHerkunft = lagbuch.IDHerkunft;
                        }
                    }
                }
            }
        }
    }
 
    if (idHerkunft == Guid.Empty)
    {
        return null;
    }
 
    var query = BO_Lagerbuchung.GetQLagerBuchungenByHerkunft(idHerkunft);
    return BO_Lagerbuchung.LoadList(query.SQL).FirstOrDefault();
}

Hier sieht man schnell, dass ein Großteil der Methode dazu dient, die idHerkunft zu ermitteln. Um die Methode lesbarer zu machen, sollten wir daher eine Methode für diese Ermittlung auslagern:

public static ILagerbuchung GetUrsprungsLagerbuchung(IQSPruefung xQSPruefung, bool xKompletterUrsprung = true)
{
    var idHerkunft = GetIdHerkunft(xQSPruefung, xKompletterUrsprung);
    if(idHerkunft == Guid.Empty)
        return null;
 
    var query = BO_Lagerbuchung.GetQLagerBuchungenByHerkunft(idHerkunft);
    return BO_Lagerbuchung.LoadList(query.SQL).FirstOrDefault();
}
 
private static Guid GetIdHerkunft(IQSPruefung xQSPruefung, bool xKompletterUrsprung)
{
    var idHerkunft = xQSPruefung.IDWareneingangposition;
    if(idHerkunft == Guid.Empty)
    {
        idHerkunft = xQSPruefung.IDWareneingangKundePosition;
        if(idHerkunft == Guid.Empty)
        {
            idHerkunft = xQSPruefung.IDBetriebsauftragFertigmeldung;
            if(idHerkunft == Guid.Empty)
            {
                idHerkunft = xQSPruefung.IDManuelleLagerbuchung;
                if(idHerkunft == Guid.Empty)
                {
                    idHerkunft = xQSPruefung.IDLagerbuchung;
                    if(idHerkunft != Guid.Empty)
                    {
                        if(xKompletterUrsprung)
                        {
                            var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
                            while(lagbuch != null && lagbuch.HerkunftObjectType == typeof(BO_Lagerbuchung).FullName)
                            {
                                idHerkunft = lagbuch.IDHerkunft;
                                lagbuch = BO_Lagerbuchung.Load(idHerkunft);
                            }
 
                            if(lagbuch != null)
                                idHerkunft = lagbuch.IDHerkunft;
                        }
                    }
                }
            }
        }
    }
 
    return idHerkunft;
}

Weiter fällt auf, dass die Bedingungen sehr tief verschachtelt sind, obwohl hier eigentlich jeweils nur ein Zweig relevant ist. Durch die Auslagerung der Methode haben wir hier nun sehr einfach die Möglichkeit, die Bedingungen zu invertieren und jeweils mit einem return zu arbeiten:

private static Guid GetIdHerkunft(IQSPruefung xQSPruefung, bool xKompletterUrsprung)
{
    var idHerkunft = xQSPruefung.IDWareneingangposition;
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
    
    idHerkunft = xQSPruefung.IDWareneingangKundePosition;
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
    
    idHerkunft = xQSPruefung.IDBetriebsauftragFertigmeldung;
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
    
    idHerkunft = xQSPruefung.IDManuelleLagerbuchung;
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
    
    idHerkunft = xQSPruefung.IDLagerbuchung;
    if(idHerkunft == Guid.Empty)
        return idHerkunft;
    
    if(!xKompletterUrsprung)
        return idHerkunft;
    
    var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    while(lagbuch != null && lagbuch.HerkunftObjectType == typeof(BO_Lagerbuchung).FullName)
    {
        idHerkunft = lagbuch.IDHerkunft;
        lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    }
 
    if(lagbuch == null)
        return idHerkunft;
 
    return lagbuch.IDHerkunft;
}

Weiter geht’s mit den ersten 4 ifs. Bei näherem Hinsehen fällt auf, dass hier der erste Wert verwendet wird, der nicht leer ist. Dies lässt sich mittels einer Schleife vereinfachen. Vorsicht dabei: Der 5. if sieht zwar ähnlich aus, ist aber invertiert und darf daher nicht umgestellt werden.

private static Guid GetIdHerkunft(IQSPruefung xQSPruefung, bool xKompletterUrsprung)
{
    var ids = new[] {
        xQSPruefung.IDWareneingangposition,
        xQSPruefung.IDWareneingangKundePosition,
        xQSPruefung.IDBetriebsauftragFertigmeldung,
        xQSPruefung.IDManuelleLagerbuchung,
    };
 
    var idHerkunft = ids.FirstOrDefault(x => x != Guid.Empty);
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
 
    idHerkunft = xQSPruefung.IDLagerbuchung;
    if(idHerkunft == Guid.Empty)
        return idHerkunft;
    
    if(!xKompletterUrsprung)
        return idHerkunft;
    
    var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    while(lagbuch != null && lagbuch.HerkunftObjectType == typeof(BO_Lagerbuchung).FullName)
    {
        idHerkunft = lagbuch.IDHerkunft;
        lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    }
 
    if(lagbuch == null)
        return idHerkunft;
 
    return lagbuch.IDHerkunft;
}

Die beiden aufeinanderfolgenden ifs mit identischer Bedingung können zudem zusammengefasst werden:

private static Guid GetIdHerkunft(IQSPruefung xQSPruefung, bool xKompletterUrsprung)
{
    var ids = new[] {
        xQSPruefung.IDWareneingangposition,
        xQSPruefung.IDWareneingangKundePosition,
        xQSPruefung.IDBetriebsauftragFertigmeldung,
        xQSPruefung.IDManuelleLagerbuchung,
    };
 
    var idHerkunft = ids.FirstOrDefault(x => x != Guid.Empty);
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
 
    idHerkunft = xQSPruefung.IDLagerbuchung;
    if(idHerkunft == Guid.Empty || !xKompletterUrsprung)
        return idHerkunft;
    
    var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    while(lagbuch != null && lagbuch.HerkunftObjectType == typeof(BO_Lagerbuchung).FullName)
    {
        idHerkunft = lagbuch.IDHerkunft;
        lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    }
 
    if(lagbuch == null)
        return idHerkunft;
 
    return lagbuch.IDHerkunft;
}

Schließlich kann die while-Schleife mit der sperrigen Bedingung in eine Endlosschleife mit verschiedenen Abbruchbedingungen umgewandelt werden.

private static Guid GetIdHerkunft(IQSPruefung xQSPruefung, bool xKompletterUrsprung)
{
    var ids = new[] {
        xQSPruefung.IDWareneingangposition,
        xQSPruefung.IDWareneingangKundePosition,
        xQSPruefung.IDBetriebsauftragFertigmeldung,
        xQSPruefung.IDManuelleLagerbuchung,
    };
 
    var idHerkunft = ids.FirstOrDefault(x => x != Guid.Empty);
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
 
    idHerkunft = xQSPruefung.IDLagerbuchung;
    if(idHerkunft == Guid.Empty || !xKompletterUrsprung)
        return idHerkunft;
    
    while(true)
    {
        var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
        if(lagbuch == null)
            return idHerkunft;
        
        if(lagbuch.HerkunftObjectType != typeof(BO_Lagerbuchung).FullName)
            return lagbuch.IDHerkunft;
 
        idHerkunft = lagbuch.IDHerkunft;
    }
}

Zu guter Letzt kann die while-Schleife in eine eigene Methode ausgelagert werden und diese kann rekursiv aufgerufen werden, was die Lesbarkeit erneut verbessert:

public static ILagerbuchung GetUrsprungsLagerbuchung(IQSPruefung xQSPruefung, bool xKompletterUrsprung = true)
{
    var idHerkunft = GetIdHerkunft(xQSPruefung, xKompletterUrsprung);
    if(idHerkunft == Guid.Empty)
        return null;
 
    var query = BO_Lagerbuchung.GetQLagerBuchungenByHerkunft(idHerkunft);
    return BO_Lagerbuchung.LoadList(query.SQL).FirstOrDefault();
}
 
private static Guid GetIdHerkunft(IQSPruefung xQSPruefung, bool xKompletterUrsprung)
{
    var ids = new[] {
        xQSPruefung.IDWareneingangposition,
        xQSPruefung.IDWareneingangKundePosition,
        xQSPruefung.IDBetriebsauftragFertigmeldung,
        xQSPruefung.IDManuelleLagerbuchung,
    };
 
    var idHerkunft = ids.FirstOrDefault(x => x != Guid.Empty);
    if(idHerkunft != Guid.Empty)
        return idHerkunft;
 
    idHerkunft = xQSPruefung.IDLagerbuchung;
    if(idHerkunft == Guid.Empty || !xKompletterUrsprung)
        return idHerkunft;
 
	return GetIdHerkunft(idHerkunft);
}
 
private Guid GetIdHerkunft(Guid idHerkunft)
{
    var lagbuch = BO_Lagerbuchung.Load(idHerkunft);
    if(lagbuch == null)
     return idHerkunft;
       
    if(lagbuch.HerkunftObjectType != typeof(BO_Lagerbuchung).FullName)
        return lagbuch.IDHerkunft;
 
    return GetIdHerkunft(lagbuch.IDHerkunft);
}