Refactorizando un método con demasiados bool en él

Question

Tengo este método en C#, y deseo refactorizarlo. Hay demasiados bools y líneas. ¿Cuál sería la mejor refactorización? Hacer una nueva clase parece un poco exagerado, y cortar simplemente en dos parece difícil. Cualquier idea o puntero sería apreciado.

método para refactorizar

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

Una opción es refactorizar a cabo cada uno de los booleanos primarios (canDeleteSires, deletingSearchGroup || wasSearchGroup) en métodos con nombres que describen la versión legible de la lógica:

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

A continuación, encapsulan la lógica booleana actual dentro de estos métodos , cómo se pasa el estado (argumentos del método o miembros de la clase) es una cuestión de gusto.

Esto eliminará los booleanos del método principal en métodos más pequeños que hacen y responden una pregunta directamente. He visto este enfoque utilizado en el estilo de desarrollo de "comentarios son malvados". Para ser honesto, me parece un poco exagerado si eres un lobo solitario, pero en un equipo puede ser mucho más fácil de leer.

Fuera de preferencia personal también me invertir su primera sentencia if para regresar temprano, esto reducirá el nivel de sangría de todo el método:

if (searchAllSireList) 
{ 
    return result; 
} 

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

Pero entonces usted podría obtener castigado por los "múltiples retornos son malvado "multitud". Me da la impresión de la práctica del desarrollo es como la política ...

Answer 2

En realidad, yo personalmente lo dejaría como está. Los booleanos que tiene en todas partes, aunque ineficientes, hacen que esta función sea legible y fácil de entender.

Puede hacer algo así como combinar todos los bools en una sola línea (como a continuación), sin embargo, eso no es tan fácil de mantener como lo que escribió.

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

Answer 3

Quizás puedas intentar eliminar todos los comentarios. Las variables bool que tiene agregan valor a la comprensión del código, puede poner varios de ellos en línea para canDeleteSires, pero no creo que eso agregue ningún valor.

Por otro lado, ese código está en su forma, por lo que puede ser mejor en su controlador para que pueda mantener la forma simple y que el controlador realmente controle el comportamiento.

Answer 4

Este es un pequeño refactor para la eliminación de algunas abolladuras:

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