¿Cómo edito una cadena de declaraciones if-else if para cumplir con los principios del Código Limpio del Tío Bob?

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

Sin embargo, 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);}

No puedo eliminar los elses y así separar todo en pedazos más pequeños, porque el "else" en el "else if" ayuda al rendimiento: evaluar esas condiciones es costoso y si puedo evitar evaluar las siguientes condiciones, causa una de las primeras Es cierto, quiero evitarlos.

Incluso hablando semánticamente, evaluar la siguiente condición si se cumplió la anterior no tiene sentido desde el punto de vista comercial.

editar: esta pregunta se identificó como un posible duplicado de formas elegantes de manejar if (if else) else .

Creo que esta es una pregunta diferente (puedes ver eso también al comparar las respuestas de esas preguntas).

• Mi pregunta es verificar si la primera condición de aceptación finaliza rápidamente .
• La pregunta vinculada es tratar de tener todas las condiciones para aceptar y hacer algo. (mejor visto en esta respuesta a esa pregunta: https://softwareengineering.stackexchange.com/a/122625/96955 )

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 llegar a

{
    addAlert(GetConditionCode());
}

y tiene GetConditionCode () encapsula la lógica para verificar las condiciones. Quizás también sea mejor usar una enumeración 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 única, al igual que las acciones no son más complejas de lo que ha mostrado, diría que el código no tiene nada de malo. Ya es tan simple como puede ser.

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

Por supuesto, puede reemplazar la elsepalabra clave con un returncomo otros han sugerido, pero eso es solo una cuestión de estilo, no un cambio en la complejidad.

Aparte:

Mi consejo general sería, nunca volverse religioso sobre ninguna 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 radicalmente ese mismo consejo en todas partes puede ganarle una entrada en El IOCCC . El truco siempre es 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 if()declaraciones, 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 recuerde, no se vuelva religioso incluso sobre este consejo también.

Es controvertido si esto es 'mejor' que el simple si ... para cualquier caso dado. Pero si quieres probar algo más, esta es una forma común de hacerlo.

Pon tus condiciones en objetos y pon esos objetos en una lista

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

Si se requieren múltiples acciones con la condición, puede 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, sí. Esto solo funciona si tiene un patrón para su lógica. Si intenta realizar una acción condicional recursiva súper genérica, la configuración del objeto será tan complicada como la instrucción if original. Inventarás tu propio nuevo lenguaje / marco.

Pero su ejemplo hace tener un patrón

Un caso de uso común para este patrón sería la validación. En vez 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 elses. Incluso podría hacerlo 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 espacio correcto también puede ser una buena alternativa:

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

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

Como una variante de la respuesta de @ Ewan, podría 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 forma, puede aplicar sus condiciones en un orden definido y la infraestructura (se muestra la clase abstracta) omite las comprobaciones restantes después de que se haya cumplido el primero.

Aquí es donde es superior al enfoque de "lista plana", donde debe 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 ello.

Luego, si no le gusta, desarrolle la idea de @ Ewan de usar una lista pero elimine su foreach breakpatrón poco natural :

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 adapte esto en su idioma de elección, haga que cada elemento de la lista sea un objeto, una tupla, lo que sea, y estará bien.

EDITAR: parece que no está tan claro, pensé, así que déjame explicarte más. conditionses una lista ordenada de algún tipo; heades el elemento actual que se está investigando: al principio es el primer elemento de la lista, y cada vez que next()se llama se convierte en el siguiente; check()y alert()son los checkConditionX()y addAlert(X)desde el 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 addAlertes más complicado, entonces una solución posiblemente mejor en digamos 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 bellas en c # <8, sino que se eligen por conveniencia.

Las ventajas de este método, incluso si ninguna de las opciones anteriores se aplica, es que la estructura está estáticamente tipada. No puedes equivocarte accidentalmente por, digamos, perder un else.

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

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

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 que simplemente valores también (c #). Esto es realmente solo funciones en línea, pero también puede señalar otras funciones también si la lógica es 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 tratando de seguir las sugerencias de código limpio del tío Bob y específicamente para mantener los métodos cortos.

Sin embargo, 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 checkCondition(), y solo 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. Después de leer varios de los libros del tío Bob, creo que ese es el mensaje que constantemente intenta transmitir.

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

EG: checkCondition1()se convertiría evaluateCondition1(), en el cual comprobaría si se cumplían las condiciones anteriores; si es así, almacena en caché algún valor para ser recuperado getConditionNumber().

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

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

EDITAR:

Así es como 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 demasiadas verificaciones costosas para realizar y las cosas en este código permanecen privadas, este enfoque ayuda a mantenerlo, permitiendo cambiar el orden de las verificaciones si es necesario.

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

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

Esta respuesta solo proporciona algunas sugerencias alternativas de las otras respuestas, y probablemente no será mejor que el código original si consideramos solo 4 líneas de código. Aunque, este no es un enfoque terrible (y tampoco hace que el mantenimiento sea más difícil como lo han dicho otros) dado el escenario que mencioné (demasiadas verificaciones, 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 "más" obligan al lector del código a recorrer toda la cadena para encontrar el que le interesa. Utilice un método como: void AlertUponCondition (condición condición) {switch (condición) {case Condition.Con1: ... break; Condición del caso. Con2: ... descanso; etc ...} Donde "Condición" es una enumeración adecuada. Si es necesario, devuelva un valor bool o. 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 que excede algunos casos.

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

Un código como ese es a menudo un olor para un modelo OO carente. 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 cosa y trata de compensarlas más tarde, en un momento en que realmente necesita saber con qué está tratando para continuar.

El polimorfismo puede haber sido mejor para ti.

Sospeche del código con métodos largos que contienen construcciones largas o complejas de si-entonces. A menudo quieres un árbol de clases allí con algunos métodos virtuales.