¿Es más legible una expresión booleana grande que la misma expresión dividida en métodos de predicado? [cerrado]

  

Lo que es más fácil de entender

El último enfoque. No solo es más fácil de entender, sino que también es más fácil escribir, probar, refactorizar y extender. Cada condición requerida puede ser desacoplada y manejada de manera segura a su manera.

  

es problemático porque tienes que leer todos los métodos para entender el código

No es problemático si los métodos se nombran correctamente. De hecho, sería más fácil de entender, ya que el nombre del método describiría la intención de la condición.
Para un observador if MatchesDefinitionId() es más explicativo que if (propVal.PropertyId == context.Definition.Id)

[Personalmente, el primer enfoque me duele los ojos.]

Si este es el único lugar donde se usarían estas funciones de predicado, también puede usar las variables locales bool en su lugar:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    bool matchesDefinitionId = (propVal.PropertyId == context.Definition.Id);
    bool matchesParentId = (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    bool matchesSecondaryFilter = (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    bool hasNoSecondaryFilter = (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);

    return matchesDefinitionId
        && matchesParentId
        && matchesSecondaryFilter || hasNoSecondaryFilter;
}

Estos también podrían desglosarse más y reordenarse para hacerlos más legibles, por ejemplo. con

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

y luego reemplazar todas las instancias de propVal.SecondaryFilter.HasValue . Una cosa que sobresale de inmediato es que hasNoSecondaryFilter usa AND lógico en las propiedades HasValue negadas, mientras que matchesSecondaryFilter usa AND AND lógico en HasValue no-negado, por lo que no es exactamente lo contrario.

En general, se prefiere este último.

Hace que el sitio de llamadas sea más reutilizable. Admite DRY (lo que significa que tiene menos lugares para cambiar cuando cambian los criterios y puede hacerlo de manera más confiable). Y muy a menudo esos sub-criterios son cosas que se reutilizarán de forma independiente en otro lugar, lo que le permite hacerlo.

Ah, y hace que esto sea mucho más fácil para la prueba unitaria, dándote la confianza de que lo has hecho correctamente.

Si está entre estas dos opciones, esta última es mejor. ¡Estas no son las únicas opciones, sin embargo! ¿Qué hay de dividir la función única en múltiples ifs? Pruebe las formas de salir de la función para evitar pruebas adicionales, emulando aproximadamente un "cortocircuito" en una prueba de una sola línea.

Esto es más fácil de leer (es posible que necesites revisar la lógica de tu ejemplo, pero el concepto es verdadero):

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    if( propVal.PropertyId != context.Definition.Id ) return false;

    if( repo.ParentId.HasValue || repo.ParentId != propVal.ParentId ) return false;

    if( propVal.SecondaryFilter.HasValue && 
        context.SecondaryFilter.HasValue && 
        propVal.SecondaryFilter.Value == context.SecondaryFilter ) return true;

    if( !context.SecondaryFilter.HasValue && 
        !propVal.SecondaryFilter.HasValue) return true;

    return false;   
}

Me gusta más la opción 2, pero sugeriría un cambio estructural. Combine los dos controles en la última línea del condicional en una sola llamada.

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    return MatchesDefinitionId(context, propVal)
        && MatchesParentId(propVal)
        && MatchesSecondaryFilterIfPresent(context, propVal);
}

private static bool MatchesSecondaryFilterIfPresent(CurrentSearchContext context, 
                                                    TValToMatch propVal)
{
    return MatchedSecondaryFilter(context, propVal) 
               || HasNoSecondaryFilter(context, propVal);
}

La razón por la que sugiero esto es que las dos comprobaciones son una sola unidad funcional, y el paréntesis de anidamiento en un condicional es propenso a errores: tanto desde el punto de vista de escribir inicialmente el código como desde el punto de vista de la persona que lo lee. Este es especialmente el caso si los subelementos de la expresión no siguen el mismo patrón.

No estoy seguro de si MatchesSecondaryFilterIfPresent() es el mejor nombre para la combinación; pero nada mejor viene inmediatamente a la mente.

Aunque en C #, el código no está muy orientado a objetos. Está utilizando métodos estáticos y lo que parecen campos estáticos (por ejemplo, repo ). En general, se considera que las estadísticas hacen que su código sea difícil de refactorizar y difícil de probar, a la vez que dificulta la reutilización y, para su pregunta: el uso estático como este es menos legible & mantenible que la construcción orientada a objetos.

Debes convertir este código a una forma más orientada a objetos. Cuando lo haga, encontrará que hay lugares razonables para poner código que hace la comparación de objetos, campos, etc. Es probable que luego pueda pedir a los objetos que se comparen entre sí, lo que reduciría su gran declaración if a una simple solicitud de comparación (por ejemplo, if ( a.compareTo (b) ) { } , que podría incluir todas las comparaciones de campos)

C # tiene un amplio conjunto de interfaces y utilidades del sistema para hacer comparaciones de objetos y sus campos. Más allá del método obvio de .Equals , para empezar, busque en IEqualityComparer , IEquatable , y utilidades como System.Collections.Generic.EqualityComparer.Default .

Definitivamente se prefiere este último, he visto casos con la primera forma y casi siempre es imposible de leer. He cometido el error de hacerlo de la primera manera y se me pidió que lo cambiara a métodos de predicado.

Yo diría que los dos son casi iguales, SI se agregan espacios en blanco para facilitar la lectura y algunos comentarios para ayudar al lector en las partes más oscuras.

Recuerda: un buen comentario le dice al lector lo que estabas pensando cuando escribiste el código.

Con cambios como he sugerido, probablemente iría con el enfoque anterior, ya que es menos abarrotado y difuso. Las llamadas de subrutinas son como las notas a pie de página: proporcionan información útil pero interrumpen el flujo de la lectura. Si los predicados fueran más complejos, entonces los dividiría en métodos separados para que los conceptos que ellos incorporan puedan construirse en partes comprensibles.

Bueno, si hay partes que tal vez desee reutilizar, obviamente, la mejor idea es separarlas en funciones separadas con nombres apropiados.
Incluso si nunca puede reutilizarlos, hacerlo podría permitirle estructurar mejor sus condiciones y darles una etiqueta que describa lo que significan .

Ahora, veamos su primera opción y admitamos que ni la sangría ni el corte de línea fueron tan útiles, ni el condicional fue tan bien estructurado:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal) {
    return propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue
        || repo.ParentId == propVal.ParentId
        && propVal.SecondaryFilter.HasValue == context.SecondaryFilter.HasValue
        && (!propVal.SecondaryFilter.HasValue || propVal.SecondaryFilter.Value == context.SecondaryFilter.Value);
}

El primero es absolutamente horrible. Has estado utilizando || por dos cosas en la misma línea; es un error en su código o una intención de ofuscar su código.

    return (   (   propVal.PropertyId == context.Definition.Id
                && !repo.ParentId.HasValue)
            || (   repo.ParentId == propVal.ParentId
                && (   (   propVal.SecondaryFilter.HasValue
                        && context.SecondaryFilter.HasValue 
                        && propVal.SecondaryFilter.Value == context.SecondaryFilter)
                    || (   !context.SecondaryFilter.HasValue
                        && !propVal.SecondaryFilter.HasValue))));

Eso es, al menos, a la mitad de un formato decente (si el formato es complicado, porque la condición if es complicada), y al menos tienes la oportunidad de descubrir si hay algo sin sentido. Comparado con su basura formateada si, cualquier otra cosa es mejor. Pero parece que solo puedes hacer extremos: o un desorden completo de una declaración if, o cuatro métodos completamente inútiles.

Tenga en cuenta que (cond1 & & cond2) || (! cond1 & & cond3) se puede escribir como

cond1 ? cond2 : cond3

lo que reduciría el desorden. Yo escribiría

if (propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue) {
    return true;
} else if (repo.ParentId != propVal.ParentId) {
    return false;
} else if (propVal.SecondaryFilter.HasValue) {
    return (   context.SecondaryFilter.HasValue
            && propVal.SecondaryFilter.Value == context.SecondaryFilter); 
} else {
    return !context.SecondaryFilter.HasValue;
}

No me gustan ninguna de esas soluciones, son difíciles de entender y de leer. La abstracción a métodos más pequeños solo para métodos más pequeños no siempre resuelve el problema.

Idealmente, creo que compararía metaprogrmáticamente las propiedades, por lo que no tiene un método nuevo definido o si se ramifica cada vez que quiera comparar un nuevo conjunto de propiedades.

No estoy seguro de c #, pero en javascript algo como esto sería MUCHO mejor y al menos podría reemplazar MatchesDefinitionId y MatchesParentId

function compareContextProp(obj, property, value){
  if(obj[property])
    return obj[property] == value
  return false
}