Refaktoryzacja metody ze zbyt dużą liczbą bool

Question

Mam tę metodę w języku C#, i chcę ją refactor. Jest po prostu zbyt wiele boolów i linii. Jaki byłby najlepszy refaktoryzacji. Tworzenie nowej klasy wydaje się nieco przesadą, a cięcie tylko na dwie części wydaje się trudne. Dowolny wgląd lub wskaźnik byłby doceniony.

metoda byłaby

private DialogResult CheckForSireRestrictionInSubGroup(bool deletingGroup,string currentId) 
    { 
     DialogResult result = DialogResult.No; 
     if (!searchAllSireList) 
     { 
      DataAccessDialog dlg = BeginWaitMessage(); 
      bool isClose = false; 
      try 
      { 
       ArrayList deletedSire = new ArrayList(); 
       ISireGroupBE sireGroupBE = sireController.FindSireGroupSearch(); 

       if (sireGroupBE != null) 
       { 
        //if the current group is in fact the seach group before saving 
        bool currentGroupIsSeachGroup = sireGroupBE.TheSireGroup.id == currentId; 

        //if we have setting this group as search group 
        bool selectedAsSearchGroup = this.chkBoxSelectedSireGroup.Checked; 

        //if the group we currently are in is not longer the seach group(chk box was unchecked) 
        bool wasSearchGroup = currentGroupIsSeachGroup && !selectedAsSearchGroup; 

        //if the group is becoming the search group 
        bool becomesSearchGroup = !currentGroupIsSeachGroup && selectedAsSearchGroup; 

        //if the group being deleted is in fact the search group 
        bool deletingSearchGroup = deletingGroup && currentGroupIsSeachGroup; 

        //if the user checked the checkbox but he's deleting it, not a so common case, but 
        //we shouldn't even consider to delete sire in this case 
        bool deletingTemporarySearchGroup = deletingGroup && !currentGroupIsSeachGroup;   

        //if we are not deleting a temporary search group and it's either 
        //becoming one (without deleting it) or we already are the search group 
        bool canDeleteSires = !deletingTemporarySearchGroup && 
              (becomesSearchGroup || currentGroupIsSeachGroup); 
        //we only delete sires if we are in search group 
        if (canDeleteSires) 
        { 
         if (deletingSearchGroup || wasSearchGroup) 
         { 
          // If we deleted all sires 
          deletedSire = new ArrayList(); 
          deletedSire.AddRange(sireGroupBE.SireList); 
         } 
         else 
         { 
          //if we delete a few sire from the change of search group 
          deletedSire = GetDeleteSire(sireGroupBE.SireList); 
         } 
        } 

        EndWaitMessage(dlg); 
        isClose = true; 
        result = ShowSubGroupAffected(deletedSire); 
       } 
      } 
      finally 
      { 
       if (!isClose) 
       { 
        EndWaitMessage(dlg); 
       } 
      } 
     } 

     return result; 
    } 

Answer 1

Jedna opcja to byłaby na każdy z podstawowych wartości logicznych (canDeleteSires, deletingSearchGroup || wasSearchGroup) do metod, których nazwy opisują czytelną wersję logiki:

if (WeAreInSearchGroup()) 
{ 
    if (WeAreDeletingAllSires()) 
    { 
     deletedSire = new ArrayList(); 
     deletedSire.AddRange(sireGroupBE.SireList); 
    } 
    else 
    { 
     deletedSire = GetDeleteSire(sireGroupBE.SireList); 
    } 
} 

wtedy otoczyć swoją aktualną logiczną logiki wewnątrz tych metod sposób przekazywania stanu (argumenty metodyczne lub członkowie klasy) jest kwestią gustu.

Spowoduje to usunięcie booleans z głównej metody na mniejsze metody, które bezpośrednio zadają pytanie i odpowiadają na nie. Widziałem to podejście stosowane w stylu "komentarze są złe". Szczerze mówiąc, uważam to za trochę przesadę, jeśli jesteś samotnym wilkiem, ale w zespole może być o wiele łatwiejsze do odczytania.

Out osobistych preferencji Chciałbym również odwrócić swój pierwszy if wrócić wcześnie, będzie to zmniejszyć poziom wcięcia całego metody:

if (searchAllSireList) 
{ 
    return result; 
} 

DataAccessDialog dlg = BeginWaitMessage(); 
bool isClose = false; 
try 
... 

Ale wtedy można dostać skarcił przez „wielokrotnych powrotów są zło "tłum. Mam praktyka rozwój wrażenie jest jak polityka ...

Answer 2

Właściwie Osobiście zostawić jak jest. Booleańczycy, których masz wszędzie, choć nieefektywne, sprawiają, że ta funkcja jest czytelna i łatwa do zrozumienia.

Możesz zrobić coś takiego, jak połączyć wszystkie sygnały na jednej linii (jak poniżej), ale nie jest to tak łatwe w utrzymaniu jak to, co napisałeś.

x = ((a & b) &! D) | mi;

Answer 3

Może możesz spróbować usunąć wszystkie komentarze. Zmienne bool dodają wartość do zrozumienia kodu, możesz umieścić kilka z nich w linii dla canDeleteSires, ale nie sądzę, że doda to żadnej wartości.

Z drugiej strony ten kod znajduje się w twojej formie, więc może być lepiej w kontrolerze, dzięki czemu możesz zachować prostotę formularza, a kontroler faktycznie kontroluje zachowanie.

Answer 4

Jest to mały Refactor do usuwania jakieś wcięcia:

private DialogResult CheckForSireRestrictionInSubGroup(bool deletingGroup,string currentId) 
{ 
    if (searchAllSireList) 
     return DialogResult.No; 

    DataAccessDialog dlg = BeginWaitMessage(); 
    bool isClose = false; 

    try 
    { 
     ISireGroupBE sireGroupBE = sireController.FindSireGroupSearch(); 

     if (sireGroupBE == null) 
      return DialogResult.No; 

     //if the current group is in fact the seach group before saving 
     bool currentGroupIsSeachGroup = sireGroupBE.TheSireGroup.id == currentId; 

     //if we have setting this group as search group 
     bool selectedAsSearchGroup = this.chkBoxSelectedSireGroup.Checked; 

     //if the group we currently are in is not longer the seach group(chk box was unchecked) 
     bool wasSearchGroup = currentGroupIsSeachGroup && !selectedAsSearchGroup; 

     //if the group is becoming the search group 
     bool becomesSearchGroup = !currentGroupIsSeachGroup && selectedAsSearchGroup; 

     //if the group being deleted is in fact the search group 
     bool deletingSearchGroup = deletingGroup && currentGroupIsSeachGroup; 

     //if the user checked the checkbox but he's deleting it, not a so common case, but 
     //we shouldn't even consider to delete sire in this case 
     bool deletingTemporarySearchGroup = deletingGroup && !currentGroupIsSeachGroup;   

     //if we are not deleting a temporary search group and it's either 
     //becoming one (without deleting it) or we already are the search group 
     bool canDeleteSires = !deletingTemporarySearchGroup && 
           (becomesSearchGroup || currentGroupIsSeachGroup); 

     ArrayList deletedSire = new ArrayList(); 

     //we only delete sires if we are in search group 
     if (canDeleteSires) 
     { 
      if (deletingSearchGroup || wasSearchGroup) 
      { 
       // If we deleted all sires 
       deletedSire.AddRange(sireGroupBE.SireList); 
      } 
      else 
      { 
       //if we delete a few sire from the change of search group 
       deletedSire = GetDeleteSire(sireGroupBE.SireList); 
      } 
     } 

     EndWaitMessage(dlg); 
     isClose = true; 
     return ShowSubGroupAffected(deletedSire); 
    } 
    finally 
    { 
     if (!isClose) 
     { 
      EndWaitMessage(dlg); 
     } 
    } 
    return DialogResult.No; 
}