¿Cómo edito una cadena de instrucciones if-else if para que se adhieran a los principios del Código de limpieza del tío Bob?

Idealmente, creo que debería extraer su lógica para obtener el código / número de alerta en su propio método. Por lo tanto, su código existente se reduce hasta

{
    addAlert(GetConditionCode());
}

y tienes GetConditionCode () encapsula la lógica para verificar las condiciones. Quizás también sea mejor usar un Enum que un número mágico.

private AlertCode GetConditionCode() {
    if (CheckCondition1()) return AlertCode.OnFire;
    if (CheckCondition2()) return AlertCode.PlagueOfBees;
    if (CheckCondition3()) return AlertCode.Godzilla;
    if (CheckCondition4()) return AlertCode.ZombieSharkNado;
    return AlertCode.None;
}

La medida importante es la complejidad del código, no el tamaño absoluto. Suponiendo que las diferentes condiciones son realmente solo llamadas de función, al igual que las acciones no son más complejas que las que has mostrado, diría que no hay nada de malo en el código. Ya es tan simple como puede ser.

Cualquier intento de "simplificar" aún más complicará las cosas.

Por supuesto, puede reemplazar la palabra clave else con un return como han sugerido otros, pero eso es solo una cuestión de estilo, no un cambio en la complejidad en absoluto.

Aparte:

Mi consejo general sería: nunca se vuelva religioso con respecto a cualquier regla para el código limpio: la mayoría de los consejos de codificación que ve en Internet son buenos si se aplican en un contexto adecuado, pero aplicar esos mismos consejos en todas partes puede serle de utilidad. una entrada en el IOCCC . El truco es siempre lograr un equilibrio que permita a los seres humanos razonar fácilmente sobre su código.

Usa métodos demasiado grandes, y estás jodido. Usa funciones demasiado pequeñas, y estás jodido. Evita las expresiones ternarias, y estás jodido. Usa expresiones ternarias en todas partes, y estás jodido. Tenga en cuenta que hay lugares que requieren funciones de una línea y lugares que requieren funciones de 50 líneas (sí, ¡existen!). Tenga en cuenta que hay lugares que requieren las declaraciones if() y que hay lugares que requieren el operador ?: . Use el arsenal completo que está a su disposición e intente usar siempre la herramienta más adecuada que pueda encontrar. Y recuerda, no te pongas religioso ni siquiera con este consejo.

Es controvertido si esto es 'mejor' que el simple if ... más para un caso determinado. Pero si quieres probar otra cosa, esta es una forma común de hacerlo.

Coloque sus condiciones en los objetos y ponga esos objetos en una lista

foreach(var condition in Conditions.OrderBy(i=>i.OrderToRunIn))
{
    if(condition.EvaluatesToTrue())
    {
        addAlert(condition.Alert);
        break;
    }
}

Si se requieren varias acciones a condición de que puedas hacer una recursión loca

void RunConditionalAction(ConditionalActionSet conditions)
{
    foreach(var condition in conditions.OrderBy(i=>i.OrderToRunIn))
    {
        if(condition.EvaluatesToTrue())
        {
            RunConditionalAction(condition);
            break;
        }
    }
}

Obviamente si. Esto solo funciona si tienes un patrón en tu lógica. Si intenta realizar una acción condicional recursiva super genérica, la configuración del objeto será tan complicada como la instrucción if original. Estarás inventando tu propio nuevo lenguaje / framework.

Pero su ejemplo tiene un patrón

Un caso de uso común para este patrón sería la validación. En lugar de:

bool IsValid()
{
    if(condition1 == false)
    {
        throw new ValidationException("condition1 is wrong!");
    }
    elseif(condition2 == false)
    {
    ....

}

Se convierte

[MustHaveCondition1]
[MustHaveCondition2]
public myObject()
{
    [MustMatchRegExCondition("xyz")]
    public string myProperty {get;set;}
    public bool IsValid()
    {
        conditions = getConditionsFromReflection()
        //loop through conditions
    }
}

Considere usar return; después de que una condición haya tenido éxito, le ahorra todos los else s. Incluso podría ser return addAlert(1) directamente si ese método tiene un valor de retorno.

He visto construcciones como esta consideradas más limpias a veces:

switch(true) {
    case cond1(): 
        statement1; break;
    case cond2():
        statement2; break;
    case cond3():
        statement3; break;
    // .. etc
}

Ternario con espaciado a la derecha también puede ser una buena alternativa:

cond1() ? statement1 :
cond2() ? statement2 :
cond3() ? statement3 : (null);

Supongo que también puedes intentar crear una matriz con par que contenga la condición y la función e iterar sobre ella hasta que se cumpla la primera condición, que, como veo, sería igual a la primera respuesta de Ewan.

Como variante de la respuesta de @ Ewan, puedes crear una cadena (en lugar de una "lista plana") de condiciones como esta:

abstract class Condition {
  private static final  Condition LAST = new Condition(){
     public void alertOrPropagate(DisplayInterface display){
        // do nothing;
     }
  }
  private Condition next = Last;

  public Condition setNext(Condition next){
    this.next = next;
    return this; // fluent API
  }

  public void alertOrPropagate(DisplayInterface display){
     if(isConditionMeet()){
         display.alert(getMessage());
     } else {
       next.alertOrPropagate(display);
     }
  }
  protected abstract boolean isConditionMeet();
  protected abstract String getMessage();  
}

De esta manera, puede aplicar sus condiciones en un orden definido y la infraestructura (la clase abstracta que se muestra) omite los cheques restantes después de que se haya cumplido el primero.

Aquí es donde es superior al enfoque de "lista plana" donde tiene que implementar el "salto" en el bucle que aplica las condiciones.

Simplemente configura la cadena de condición:

Condition c1 = new Condition1().setNext(
  new Condition2().setNext(
   new Condition3()
 )
);

Y comience la evaluación con una simple llamada:

c1.alertOrPropagate(display);

En primer lugar, el código original no es terrible IMO. Es bastante comprensible y no hay nada inherentemente malo en él.

Luego, si no te gusta, crea la idea de @Ewan para usar una lista, pero elimina su patrón foreach break :

public class conditions
{
    private List<Condition> cList;
    private int position;

    public Condition Head
    {
        get { return cList[position];}
    }

    public bool Next()
    {
        return (position++ < cList.Count);
    }
}


while not conditions.head.check() {
  conditions.next()
}
conditions.head.alert()

Ahora adapta esto en el idioma que elijas, haz de cada elemento de la lista un objeto, una tupla, lo que sea, y eres bueno.

EDITAR: parece que no está tan claro como pensé, así que déjame explicarte más. conditions es una lista ordenada de algún tipo; head es el elemento actual que se está investigando; al principio, es el primer elemento de la lista, y cada vez que se llama next() se convierte en el siguiente; check() y alert() son el checkConditionX() y addAlert(X) del OP.

La pregunta carece de algunos detalles. Si las condiciones son:

• sujeto a cambios o
• repetido en otras partes de la aplicación o sistema o
• modificado en ciertos casos (como diferentes compilaciones, pruebas, implementaciones)

o si el contenido en addAlert es más complicado, entonces una mejor solución en c # sería:

//in some central spot
IEnumerable<Tuple<Func<bool>, int>> Conditions = new ... {
  Tuple.Create(CheckCondition1, 1),
  Tuple.Create(CheckCondition2, 2),
  ...
}

//at the original place
var matchingCondition = Conditions.Where(c=>c.Item1()).FirstOrDefault();
if(matchingCondition != null) 
  addAlert(matchingCondition.Item2)

Las tuplas no son tan hermosas en c # < 8, pero elegido por conveniencia.

Las ventajas con este método, incluso si no se aplica ninguna de las opciones anteriores, es que la estructura está tipificada estáticamente. No puedes equivocarte accidentalmente, por ejemplo, faltando un else .

La mejor manera de reducir Complejidad ciclomática en los casos en los que tienes mucho if->then statements es usar un < em> diccionario o lista (depende del idioma) para almacenar el valor clave (si es un valor de declaración o algún valor de) y luego un valor / función resultado.

Por ejemplo, en lugar de (C #):

if (i > 10) { return "Two"; }
else if (i > 8) { return "Four" }
else if (i > 4) { return "Eight" }
return "Ten";  //etc etc say anything after 3 or 4 values

Yo puedo simplemente

var results = new Dictionary<int, string>
{
  { 10, "Two" },
  { 8, "Four"},
  { 4, "Eight"},
  { 0, "Ten"},
}

foreach(var key in results.Keys)
{
  if (i > results[key]) return results.Values[key];
}

Si está utilizando lenguajes más modernos, puede almacenar más lógica y simplemente valores (c #). Esto es realmente solo funciones en línea, pero también puede apuntar a otras funciones también si la lógica es simplemente poner en línea.

var results = new Dictionary<Func<int, bool>, Func<int, string>>
{
  { (i) => return i > 10; ,
    (i) => return i.ToString() },
  // etc
};

foreach(var key in results.Keys)
{ 
  if (key(i)) return results.Values[key](i);
}

  

Estoy intentando seguir las sugerencias de código limpio del tío Bob y, específicamente, para mantener los métodos cortos.

     

Aunque no puedo acortar esta lógica:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

Su código ya es demasiado corto, pero la lógica en sí no debe cambiarse. A primera vista, parece que te estás repitiendo con cuatro llamadas a checkCondition() , y es evidente que cada una es diferente después de volver a leer el código cuidadosamente. Debe agregar el formato y los nombres de función adecuados, por ejemplo:

if (is_an_apple()) {
  addAlert(1);
}
else if (is_a_banana()) {
  addAlert(2);
}
else if (is_a_cat()) {
  addAlert(3);
}
else if (is_a_dog()) {
  addAlert(4);
}

Su código debe ser legible por encima de todo lo demás. Después de haber leído varios de los libros de Uncle Bob, creo que ese es el mensaje que intenta transmitir constantemente.

Suponiendo que todas las funciones se implementan en el mismo componente, podría hacer que las funciones retengan algún estado para deshacerse de las múltiples ramas en el flujo.

EG: checkCondition1() se convertiría en evaluateCondition1() , en el que verificará si se cumple la condición anterior; si es así, entonces almacena en caché algún valor para ser recuperado por getConditionNumber() .

checkCondition2() se convertiría en evaluateCondition2() , en el que verificará si se cumplen las condiciones anteriores. Si la condición anterior no se cumplió, entonces verifica el escenario de condición 2, almacenando en caché un valor para ser recuperado por getConditionNumber() . Y así sucesivamente.

clearConditions();
evaluateCondition1();
evaluateCondition2();
evaluateCondition3();
evaluateCondition4();
if (anyCondition()) { addAlert(getConditionNumber()); }

EDIT:

Aquí se explica cómo debería implementarse la verificación de condiciones costosas para que este enfoque funcione.

bool evaluateCondition34() {
    if (!anyCondition() && A && B && C) {
        conditionNumber = 5693;
        return true;
    }
    return false;
}

...

bool evaluateCondition76() {
    if (!anyCondition() && !B && C && D) {
        conditionNumber = 7658;
        return true;
    }
    return false;
}

Por lo tanto, si tiene demasiados controles costosos para realizar, y las cosas en este código siguen siendo privadas, este enfoque ayuda a mantenerlo, permitiendo cambiar el orden de los controles si es necesario.

clearConditions();
evaluateCondition10();
evaluateCondition9();
evaluateCondition8();
evaluateCondition7();
...
evaluateCondition34();
...
evaluateCondition76();

if (anyCondition()) { addAlert(getConditionNumber()); }

Esta respuesta solo proporciona una sugerencia alternativa de las otras respuestas, y probablemente no será mejor que el código original si consideramos solo 4 líneas de código. Aunque no es una enfoque terrible (y ninguno de ellos dificulta el mantenimiento, como han dicho otros) dado el escenario que mencioné (demasiadas comprobaciones, solo la función principal expuesta como pública, todas las funciones son detalles de implementación de la misma clase).

Más de dos cláusulas "else" obligan al lector del código a recorrer toda la cadena para encontrar la que le interesa. Utilice un método como: void AlertUponCondition (condición de condición) {   interruptor (condición)   {     caso Condition.Con1:       ...       descanso;     caso Condition.Con2:       ...       descanso;     etc ...   } Donde "Condición" es una enumeración adecuada. Si es necesario, devuelve un bool o valor. Llámalo así:   AlertOnCondition (GetCondition ());

Realmente no puede ser más simple, Y es más rápido que la cadena if-else una vez superas unos pocos casos.

No puedo hablar por su situación particular porque el código no es específico, pero ...

como ese es a menudo un olor para un modelo faltante de OO. Realmente tiene cuatro tipos de cosas, cada una asociada con su propio tipo de alerta, pero en lugar de reconocer estas entidades y crear una instancia de clase para cada una, las trata como una sola cosa e intenta compensarlas más tarde, en un momento en que realmente Necesito saber con qué está tratando para continuar.

Es posible que el polimorfismo se haya adaptado mejor a ti.

Desconfíe del código con métodos largos que contengan construcciones largas si- cientes si-entonces. A menudo quieres un árbol de clases allí con algunos métodos virtuales.