Refactoring un árbol Si otra cosa

que tiene un árbol si otra cosa que va creciendo a medida que agrego elementos adicionales para que pueda mantener y estoy en la mejor forma de escribirlo para el mantenimiento estoy empezando con este códigoRefactoring un árbol Si otra cosa

private void ControlSelect() 
{ 

    if (PostingType == PostingTypes.Loads && !IsMultiPost) 
    { 
     singleLoadControl.Visible = true; 
      singleTruckControl.Visible = false; 
      multiTruckControl.Visible = false; 
      multiLoadControl.Visible = false; 
    } 
    else if (PostingType == PostingTypes.Trucks && !IsMultiPost) 
    { 
     singleLoadControl.Visible = false; 
      singleTruckControl.Visible = true; 
      multiTruckControl.Visible = false; 
      multiLoadControl.Visible = false; 
    } 
    else if (PostingType == PostingTypes.Loads && IsMultiPost) 
    { 
     singleLoadControl.Visible = false; 
      singleTruckControl.Visible = false; 
      multiTruckControl.Visible = false; 
      multiLoadControl.Visible = true; 
    } 
    else if (PostingType == PostingTypes.Trucks && IsMultiPost) 
    { 
     singleLoadControl.Visible = false; 
     singleTruckControl.Visible = false; 
      multiTruckControl.Visible = true; 
     multiLoadControl.Visible = false; 
    } 
}

y el pensamiento de la re-factorizar a algo como esto

private void ControlSelect() 
{ 
    List<UserControl> controlList = GetControlList(); 

     string visableControl = singleLoadControl.ID; 
     if (PostingType == PostingTypes.Loads && !IsMultiPost) 
     { 
     visableControl = singleLoadControl.ID; 
     } 
     else if (PostingType == PostingTypes.Trucks && !IsMultiPost) 
     { 
     visableControl = singleTruckControl.ID; 
     } 
     else if (PostingType == PostingTypes.Loads && IsMultiPost) 
     { 
     visableControl = multiLoadControl.ID; 
     } 
     else if (PostingType == PostingTypes.Trucks && IsMultiPost) 
     { 
     visableControl = multiTruckControl.ID; 
     } 

     foreach (UserControl userControl in controlList) 
     { 
     userControl.Visible = (userControl.ID == visableControl); 
     } 
} 

private List<UserControl> GetControlList() 
{ 
    List<UserControl> controlList = new List<UserControl> 
     { 
     singleLoadControl, 
      multiTruckControl, 
      singleTruckControl, 
      multiLoadControl 
     }; 
     return controlList; 
}

tomo un impacto en el rendimiento pero puedo gestionar todos mis controles es un solo lugar

mis otros wa pensamiento s para hacer de cada bloque seleccionado de control es un método propio, algo como esto

private void SetSingleLoadControlAsSelected() 
{ 
     singleLoadControl.Visible = true; 
     singleTruckControl.Visible = false; 
     multiTruckControl.Visible = false; 
     multiLoadControl.Visible = false; 
}

no tomo un impacto en el rendimiento, pero estoy manteniendo los controles en lugar de múltiples

Me estoy inclinando por la opción uno solo porque me gusta el aspecto de mantenimiento de ella.

Fuente

2009-09-03 Bob The Janitor

¿Por qué no simplemente descompones toda esa lógica en clases y haces la selección por polimorfismo? –

Este es un olor a código. –

¿qué pasa con

singleLoadControl.Visible = 
     PostingType == PostingTypes.Loads && !IsMultiPost;  
singleTruckControl.Visible = 
     PostingType == PostingTypes.Trucks && !IsMultiPost;  
multiTruckControl.Visible = 
     PostingType == PostingTypes.Loads && IsMultiPost;  
multiLoadControl.Visible = 
     PostingType == PostingTypes.Trucks && IsMultiPost;

si quieres la capacidad para realizar múltiples controles visibles (o añadir más valores enumerados) decorar la enumeración con el atributo [Banderas] de la siguiente manera:

[Flags] 
public enum PostTyp { None=0, IsMultiPost = 1, Loads = 2, Trucks = 4 }

y modificar el código de la siguiente manera:

singleLoadControl.Visible = 
     ((PostingType & (PostTyp.Loads | ~PostTyp.MultiCast)) 
     == PostingType);  
singleTruckControl.Visible = 
     ((PostingType & (PostTyp.Trucks | ~PostTyp.MultiCast)) 
     == PostingType);   
multiTruckControl.Visible = 
     ((PostingType & (PostTyp.Loads | PostTyp.MultiCast)) 
     == PostingType);   
multiLoadControl.Visible = 
     ((PostingType & (PostTyp.Trucks | PostTyp.MultiCast)) 
     == PostingType);

Fuente

2009-09-03 17:14:46

Esto seguirá funcionando. Todos los valores serán falsos, excepto uno con el algoritmo anterior. –

@JS tiene razón. Solo uno se establecerá en visible ya que todas las expresiones booleanas son exclusivas –

Funciona para el escenario publicado, pero creo que este enfoque podría resultar difícil de mantener si se agregan más valores y controles de enumeración o si se requieren combinaciones. Sin embargo, si nunca cambia, creo que esta es una buena y ordenada refactorización. –

Qué tal esto:

 singleLoadControl.Visible = false; 
    singleTruckControl.Visible = false; 
    multiTruckControl.Visible = false; 
    multiLoadControl.Visible = false; 

    if (PostingType == PostingTypes.Loads && !IsMultiPost) 
    { 
      singleLoadControl.Visible = true; 
    } 
    else if (PostingType == PostingTypes.Trucks && !IsMultiPost) 
    { 
      singleTruckControl.Visible = true; 
    } 
    else if (PostingType == PostingTypes.Loads && IsMultiPost) 
    { 
     multiLoadControl.Visible = true; 
    } 
    else if (PostingType == PostingTypes.Trucks && IsMultiPost) 
    { 
      multiTruckControl.Visible = true; 
}

Fuente

2009-09-03 17:18:22 Sergio

Esto es ciertamente mejor que el código OP, pero creo que está en el camino hacia una refactorización aún más estricta, tal como lo sugirieron Charles Bretana, Chris Brandsma o yo. –

Como parece estar usando una enumeración, recomendaría un conmutador con una carcasa predeterminada para hacer frente a valores desconocidos. Creo que este enfoque hace las intenciones más claras que hacer todas las comprobaciones en la tarea.

switch (PostingType) 
{ 
case PostingTypes.Loads: 
    singleLoadControl.Visible = !IsMultiPost; 
    multiTruckControl.Visible = IsMultiPost; 
    singleTruckControl.Visible = false; 
    multiTruckLoadControl.Visible = false; 
    break; 

case PostingTypes.Trucks: 
    singleLoadControl.Visible = false; 
    multiTruckControl.Visible = false; 
    singleTruckControl.Visible = !IsMultiPost; 
    multiLoadControl.Visible = IsMultiPost; 
    break; 

default: 
    throw InvalidOperationException("Unknown enumeration value."); 
}

Fuente

2009-09-03 17:23:43

+1 para la legibilidad/claridad del código frente a asignaciones inteligentes y bitwiseness (¿eso es incluso una palabra !?). –

Si usted está esperando a ser la adición de una gran cantidad de diferentes parámetros, se puede crear una matriz multi-dimensional de objetos

Arr[0][0] = singleLoadControl 
Arr[0][1] = singleTruckControl 
Arr[1][0] = multiLoadControl 
Arr[1][1] = multiTruckControl

Esto es bastante miedo, pero lo hace para más simple si las declaraciones. Si de todos modos vas a tener muchas referencias a los controles, preferiría usar una representación basada en código de cuáles son esas cargas. Ese conjunto se puede envolver en una clase que le permiten acceder a los elementos que utilizan algo más parecido a:

ControlClassInstance.single.truck

Tendrías código como este:

p1 = IsMultiPost ? ControlClassInstance.multi : ControlClassInstance.single 
p2 = p1[PostingType] //(this call would mean adding an indexer)

Este tipo de solución es demasiado sofisticado a menos que esperes que las cosas se compliquen ... y que también sean pobres.

Fuente

2009-09-03 17:24:56 Brian

No estoy seguro de sofisticado es la palabra correcta para esto. Ofuscado, inmanejable, confuso, excesivo; estas palabras parecen un mejor ajuste. Si bien el uso se vuelve más claro, la implementación es complicada hasta el punto de que otros pueden tener problemas para comprenderla y mantenerla, lo que podría conducir a errores fáciles. –

@Jeff: Sí, agrega demasiada sobrecarga. En realidad, solo se justificaría si se agregaran muchas propiedades y si los cambios de uso que fomenta se desbordarían en otros lugares. Pero en ese caso, probablemente haya una mejor manera. ::: Shrug ::: – Brian

singleLoadControl.Visible = false; 
    singleTruckControl.Visible = false; 
    multiTruckControl.Visible = false; 
    multiLoadControl.Visible = false; 


    singleLoadControl.Visible = (PostingType == PostingTypes.Loads && !IsMultiPost); 
    singleTruckControl.Visible = (PostingType == PostingTypes.Trucks && !IsMultiPost); 
    multiLoadControl.Visible = (PostingType == PostingTypes.Loads && IsMultiPost); 
    multiTruckControl.Visible = (PostingType == PostingTypes.Trucks && IsMultiPost);

Fuente

2009-09-03 17:33:31

No hay ninguna razón para inicializar todos los valores a falso ya que esto sucederá en virtud de sus declaraciones posteriores. –

Estaba pensando en algo como esto, pero simplemente usando una instrucción switch para seleccionar la instancia específica para habilitar, pero la respuesta de Jeff Yate funciona de todos modos igual (y probablemente sea un poco más elegante con ese booleano) –

Absolutamente sí. Pero algo en las respuestas de los post me hizo pensar que esto era parte de mucho más código. Así que los dejé en. –

Si sería de otro modo sensible (por ejemplo, si estos ya son controles personalizados específicos de dominio), se podría encapsular la lógica interior de los mismos controles (Replace Conditional with Polymorphism).Tal vez crear una interfaz de esta manera:

public interface IPostingControl { 
    void SetVisibility(PostingType postingType, bool isMultiPost); 
}

Entonces cada control sería responsable de sus propias reglas de visibilidad:

public class SingleLoadControl: UserControl, IPostingControl { 

    // ... rest of the implementation 
    public void SetVisibility(PostingType postingType, bool isMultiPost) { 
     this.Visible = postingType == PostingType.Load && !isMultiPost); 
    } 
}

Por último, en su página, simplemente iterar sobre su IPostingControls y llame SetVisibility(postingType, isMultiPost).

Fuente

2009-09-03 17:40:36

Una idea interesante, pero si la única razón para especializarse es agregar manejo para esta lógica de visibilidad, no creo que sea una buena refactorización. No quiero especializar TextBox solo para agregar código específico para su manejo cuando deberían estar visibles. Ni siquiera creo que cada instancia deba saber en qué estado desea su dueño. –

De acuerdo, esto es exagerado si los controles en cuestión son solo TextBoxes (o cualquier otro UserControl listo para usar), no es probable que el dominio cambie, y la lógica está solo en un solo lugar. Puede que incluso no valga la pena si solo dos de ellos son verdaderos. Pero si uno o ninguno de ellos son ciertos, definitivamente valdría la pena! –

esto emparejaría estrechamente el control con este uso específico –

-1

private void ControlSelect() 
{ 
    if (PostingType == PostingTypes.Loads && !IsMultiPost) 
    { 
     singleLoadControl.Visible = true; 
     singleTruckControl.Visible = false; 
     multiTruckControl.Visible = false; 
     multiLoadControl.Visible = false; 
     return; 
    } 
    if (PostingType == PostingTypes.Trucks && !IsMultiPost) 
    { 
     singleLoadControl.Visible = false; 
     singleTruckControl.Visible = true; 
     multiTruckControl.Visible = false; 
     multiLoadControl.Visible = false; 
     return; 
    } 
    if (PostingType == PostingTypes.Loads && IsMultiPost) 
    { 
     singleLoadControl.Visible = false; 
     singleTruckControl.Visible = false; 
     multiTruckControl.Visible = false; 
     multiLoadControl.Visible = true; 
     return; 
    } 
    if (PostingType == PostingTypes.Trucks && IsMultiPost) 
    { 
     singleLoadControl.Visible = false; 
     singleTruckControl.Visible = false; 
     multiTruckControl.Visible = true; 
     multiLoadControl.Visible = false; 
     return; 
    } 
}

Fuente

2009-09-03 18:20:04 yigal

esto es exactamente lo que NO trato de hacer –

-1 esta es una sugerencia horrible, en mi humilde opinión. Hace que el código empeore en lugar de mejorar, presentando múltiples oportunidades para errores de mantenimiento. –

¿Has considerado el State Pattern?

Fuente

2009-09-03 19:03:19 krdluzni

Respuesta

Cuestiones relacionadas