Wie verschachtelte For-Schleifen in Methoden schreiben (sauberer Code)

Question

Clean-Code bedeutet für mich: nur eine Aufgabe für jede Methode und keine verschachtelte Schleifen.

Als ich den folgenden Code bekam, fragte ich mich, wie kann ich verschachtelte for-Schleifen vermeiden und sie in Methoden einkapseln.

private String getUser(){ 
    for (FieldConfigScheme context : getConfigurationSchemes()) { 
     for (Option option : getOptions(context)) { 
      for (Group group : getGroups()) { 
       if (option.getValue().equalsIgnoreCase(group.getName())) { 
        return group.getUser(); 
       } 
      } 
     } 
    } 
    return "default"; 
} 

Meine erste Lösung war die folgende. Das Problem hier ist, die for-Schleifen laufen bis zum Ende und brechen nicht (zurück), wenn der Wert gefunden und gesetzt ist.

private String user = "default"; 

private String getUser(){ 
    for (FieldConfigScheme context : getConfigurationSchemes()) { 
     processOptions(context); 
    } 
    return this.user; 
} 

private void processOptions(FieldConfigScheme context){ 
    for (Option option : getOptions(context)) { 
     processGroups(option); 
    } 
} 

private void processGroups(Option option){ 
    for (Group group : getGroups()) { 
     setUser(option, group); 
    } 
} 

private void setUser(Option option, Group group){ 
    if (option.getValue().equalsIgnoreCase(group.getName())) { 
     this.user = group.getUser(); 
    } 
} 

so schrieb ich diesen Code, die gleich wie die erste sein sollte:

private String user = "default"; 
private boolean isUserSet = false; 

private String getUser(){ 
    for (FieldConfigScheme context : getConfigurationSchemes()) { 
     if(!isUserSet) processOptions(context); 
     else return this.user; 
    } 
    return this.user; 
} 

private void processOptions(FieldConfigScheme context){ 
    for (Option option : getOptions(context)) { 
     if(!isUserSet) processGroups(option); 
     else return; 
    } 
} 

private void processGroups(Option option){ 
    for (Group group : getGroups()) { 
     if(!isUserSet) setUser(option, group); 
     else return; 
    } 
} 

private void setUser(Option option, Group group){ 
    if (option.getValue().equalsIgnoreCase(group.getName())) { 
     this.user = group.getUser(); 
     isUserSet = true; 
    } 
} 

fragte ich mich, aber dann ist das wirklich besser Code? Ist das ein sauberer Code? Ja, jede Methode macht nur eine Sache. Und ja, der Code ist meiner Meinung nach besser zu lesen. Aber aus ursprünglich 12 Zeilen kompakter Code habe ich nun 30 Codezeilen und eine Membervariable mehr im Code. Ist der erste Code also besser, weil er sogar mit verschachtelten For-Loops kompakter ist?

Was denkst du? Welches ist besser? Oder wie kann ich den Code besser schreiben?

Vielen Dank im Voraus für Ihre Antworten!

Answer 1

Anstatt void zurückzugeben, warum nicht boolean?

private String getUser(){ 
    for (FieldConfigScheme context : getConfigurationSchemes()) { 
     if (processOptions(context)) { 
      break; 
     } 
    } 
    return this.user; 
} 

private boolean processOptions(FieldConfigScheme context){ 
    for (Option option : getOptions(context)) { 
     if (processGroups(option)) { 
      return true; 
     } 
    } 
    return false; 
} 

private boolean processGroups(Option option){ 
    for (Group group : getGroups()) { 
     if (option.getValue().equalsIgnoreCase(group.getName())) { 
      this.user = group.getUser(); 
      return true; 
     } 
    } 
    return false; 
} 

T.B.H. Ich bevorzuge die Nested-Loop-Methode. Es sieht sauber aus, es gibt nichts mehr in der Schleife als einfach etwas in einer Hierarchie zu finden, und das ist völlig in Ordnung.

Die Verwendung der zusätzlichen Funktion ist in diesem Fall einfach schlecht. Stellen Sie sich vor, Sie müssten diesen Code jetzt debuggen, anstatt sich auf eine Methode zu konzentrieren, die dies tut, müssen Sie sich alle zusätzlichen ansehen, die Sie gemacht haben.

Auch diese Methode scheint keine Parameter zu haben, was darauf hindeutet, dass sie diese Prüfung nur einmal durchführen muss und den Rest der Zeit nur den gleichen Wert zurückgeben soll. Das ist nur eine Vermutung, aber wenn das der Fall wäre, dann macht es Ihre Bemühungen, es sauberer zu machen, umso unnötiger.