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

63

¿Qué es más fácil de entender, una gran declaración booleana (bastante compleja) o la misma declaración desglosada en métodos predicados (mucho código adicional para leer)?

Opción 1, la gran expresión booleana:

    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.Value == context.SecondaryFilter) || (!context.SecondaryFilter.HasValue && !propVal.SecondaryFilter.HasValue));
    }

Opción 2, las condiciones desglosadas en métodos predicados:

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

    private static bool HasNoSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);
    }

    private static bool MatchedSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    }

    private bool MatchesParentId(TValToMatch propVal)
    {
        return (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    }

    private static bool MatchesDefinitionId(CurrentSearchContext context, TValToMatch propVal)
    {
        return propVal.PropertyId == context.Definition.Id;
    }

Prefiero el segundo enfoque, porque veo los nombres de los métodos como comentarios, pero entiendo que es problemático porque tienes que leer todos los métodos para entender lo que hace el código, por lo que abstrae la intención del código.

willem
fuente
13
La opción 2 es similar a lo que Martin Fowler recomienda en su libro de refactorización. Además, los nombres de sus métodos sirven como la intención de todas las expresiones aleatorias, el contenido de los métodos son solo los detalles de implementación que podrían cambiar con el tiempo.
programador
2
¿Es realmente la misma expresión? "O" tiene una precedencia menor que "Y", de todos modos, el segundo le dice a su intención, el otro (primero) es técnico.
thepacker
3
Lo que dice @thepacker. El hecho de que hacerlo de la primera manera te haya llevado a cometer un error es una pista bastante buena de que la primera forma no es fácilmente comprensible para un sector muy importante de tu público objetivo. ¡Usted mismo!
Steve Jessop
3
Opción 3: no me gusta ninguno de los dos. El segundo es ridículamente detallado, el primero no es equivalente al segundo. Los paréntesis ayudan.
David Hammen
3
Esto puede ser pedante, pero no tiene ninguna if declaración en ninguno de los bloques de código. Su pregunta es sobre expresiones booleanas .
Kyle Strand

Respuestas:

88

¿Qué 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 ampliar. Cada condición requerida se puede desacoplar y manejar 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 espectador if MatchesDefinitionId()es más explicativo queif (propVal.PropertyId == context.Definition.Id)

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

maravilla
fuente
12
Si los nombres de los métodos son buenos, entonces también es más fácil de entender.
B 10овић
Y por favor, hágales (nombres de métodos) significativos y cortos. Más de 20 caracteres de nombres de métodos me duelen los ojos. MatchesDefinitionId()está en el límite.
Mindwin
2
@Mindwin Si se trata de elegir entre mantener "cortos" los nombres de los métodos y mantenerlos significativos, digo tomar el último cada vez. Corto es bueno, pero no a expensas de la legibilidad.
Ajedi32
@ Ajedi32 uno no tiene que escribir un ensayo sobre lo que hace el método en el nombre del método, o tener nombres de método gramaticalmente sólidos. Si se mantienen claras las normas de abreviatura (en todo el grupo de trabajo u organización) no habrá problemas con nombres cortos y legibilidad.
Mindwin
Use la ley de Zipf: haga las cosas más detalladas para desalentar su uso.
hoosierEE
44

Si este es el único lugar donde se usarían estas funciones de predicado, también puede usar boolvariables locales 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 aún más y reordenarse para que sean más legibles, por ejemplo, con

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

y luego reemplazando todas las instancias de propVal.SecondaryFilter.HasValue. Una cosa que sobresale de inmediato es que hasNoSecondaryFilterusa AND lógico en las HasValuepropiedades negadas , mientras que matchesSecondaryFilterusa un AND lógico en no negado HasValue, por lo que no es exactamente lo contrario.

Simon Richter
fuente
3
Esta solución es bastante buena y ciertamente he escrito muchos códigos similares. Es muy legible. La desventaja, en comparación con la solución que publiqué, es la velocidad. Con este método, realiza un montón de pruebas condicionales sin importar qué. En mi solución, las operaciones se pueden reducir drásticamente en función de los valores procesados.
BuvinJ
55
@BuvinJ Las pruebas como las que se muestran aquí deberían ser bastante baratas, por lo que, a menos que sepa que algunas de las condiciones son caras o que este sea un código extremadamente sensible al rendimiento, elegiría la versión más legible.
svick
1
@svick Sin duda, es poco probable que esto presente un problema de rendimiento la mayor parte del tiempo. Aún así, si puede reducir las operaciones sin perder legibilidad, ¿por qué no hacerlo? No estoy convencido de que esto sea mucho más legible que mi solución. Sí da "nombres" auto documentados a las pruebas, lo cual es bueno ... Creo que se reduce al caso de uso específico y lo comprensible que son las pruebas por derecho propio.
BuvinJ
Agregar comentarios también puede ayudar a la legibilidad ...
BuvinJ
@BuvinJ Lo que realmente me gusta de esta solución es que al ignorar todo, excepto la última línea, puedo entender rápidamente lo que está haciendo. Creo que esto es más legible.
svick
42

En general, se prefiere este último.

Hace que el sitio de la llamada sea más reutilizable. Es compatible con 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 subcriterios son cosas que se reutilizarán independientemente en otro lugar, lo que le permite hacer eso.

Ah, y hace que estas cosas sean mucho más fáciles de probar por unidad, lo que te da la confianza de que lo has hecho correctamente.

Telastyn
fuente
1
Sí, aunque su respuesta también debe abordar la fijación del uso de repo, que parece un campo / propiedad estático, es decir, una variable global. Los métodos estáticos deben ser deterministas y no utilizar variables globales.
David Arno
3
@DavidArno: si bien eso no es genial, parece tangencial a la pregunta en cuestión. Y sin más código que es verosímil que hay una razón válida semi-para que el diseño funciona así.
Telastyn
1
Sí, no importa el repositorio. Tenía que ofuscar el código un poco, no quieren compartir código de cliente tal cual en la interwebs :)
Willem
23

Si está entre estas dos opciones, entonces la última es mejor. ¡Sin embargo, estas no son las únicas opciones! ¿Qué tal dividir la función individual 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 deba verificar la lógica de su ejemplo, pero el concepto es cierto):

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;   
}
BuvinJ
fuente
3
¿Por qué recibí un voto negativo por esto en cuestión de segundos después de publicarlo? Por favor, agregue un comentario cuando haga voto negativo Esta respuesta funciona igual de rápido y es más fácil de leer. ¿Entonces, cuál es el problema?
BuvinJ
2
@BuvinJ: Absolutamente nada de malo. Lo mismo que el código original, excepto que no tiene que pelear con una docena de paréntesis y una sola línea que se extiende más allá del final de la pantalla. Puedo leer ese código de arriba a abajo y comprenderlo de inmediato. Recuento de WTF = 0.
gnasher729
1
Volver al final que no sea al final de la función hace que el código sea menos legible, no más legible, IMO. Prefiero un solo punto de salida. Algunos buenos argumentos en ambos sentidos en este enlace. stackoverflow.com/questions/36707/…
Brad Thomas
55
@ Brad Thomas, no puedo estar de acuerdo con el único punto de salida. Por lo general, conduce a paréntesis anidados profundos. El regreso termina el camino, así que para mí es mucho más fácil de leer.
Borjab
1
@BradThomas Estoy totalmente de acuerdo con Borjab. Evitar los anidamientos profundos es en realidad la razón por la que uso este estilo con más frecuencia que para dividir declaraciones condicionales largas. Solía ​​encontrarme escribiendo código con toneladas de anidamientos. Luego, comencé a buscar formas de casi nunca profundizar más de uno o dos anidamientos, y mi código se ha vuelto MUCHO más fácil de leer y mantener como resultado. Si puede encontrar una manera de salir de su función, ¡hágalo lo antes posible! Si puede encontrar una manera de evitar anidamientos profundos y condicionales largos, ¡hágalo!
BuvinJ
10

Me gusta más la opción 2, pero sugeriría un cambio estructural. Combine las dos verificaciones 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 anidado 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 a mi mente de inmediato.

Dan Neely
fuente
Muy bueno, tratar de explicar lo que se está haciendo dentro de los métodos es realmente mejor que simplemente reestructurar las llamadas.
klaar
2

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 sostiene que las estadísticas hacen que su código sea difícil de refactorizar y difícil de probar, al tiempo que dificultan la reutilización y, a su pregunta: el uso estático como este es menos legible y mantenible que la construcción orientada a objetos.

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

C # tiene un rico conjunto de interfaces y utilidades de sistema para hacer comparaciones en objetos y sus campos. Más allá de lo obvio .Equalsmétodo, para empezar, mira en IEqualityComparer, IEquatabley los servicios públicos como System.Collections.Generic.EqualityComparer.Default.

Erik Eidt
fuente
0

Este último es definitivamente preferido, he visto casos con la primera forma y casi siempre es imposible de leer. Cometí el error de hacerlo de la primera manera y me pidieron que lo cambiara a métodos predicados.

Fisgonear
fuente
0

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

Recuerde: un buen comentario le dice al lector lo que estaba pensando cuando escribió el código.

Con los cambios como he sugerido, probablemente seguiría el enfoque anterior, ya que es menos abarrotado y difuso. Las llamadas de subrutina son como notas al pie: proporcionan información útil pero interrumpen el flujo de lectura. Si los predicados fueran más complejos, los dividiría en métodos separados para que los conceptos que encarnan se puedan construir en fragmentos comprensibles.

Mark Wood
fuente
Se merece un +1. Buen alimento para el pensamiento, aunque no es una opinión popular basada en las otras respuestas. Gracias :)
willem
1
@willem No, no merece +1. Dos enfoques no son lo mismo. Los comentarios adicionales son estúpidos e innecesarios.
B 10овић
2
Un buen código NUNCA depende de que los comentarios sean entendibles. De hecho, los comentarios son el peor desorden que podría tener un código. El código debería hablar por sí mismo. Además, los dos enfoques que OP quiere evaluar nunca pueden ser "casi iguales", sin importar cuántos espacios en blanco se agreguen.
wonderbell
Es mejor tener un nombre de función significativo que tener que leer el comentario. Como se indica en el libro "Código limpio", un comentario es una falla al expresar el código de lanzamiento. ¿Por qué explicar lo que está haciendo cuando la función podría haberlo dicho mucho más claramente?
Borjab
0

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

Ahora, echemos un vistazo a su primera opción, y reconozcamos que su sangría y salto de línea no fueron tan útiles, ni el condicional se estructuraba tan bien:

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);
}
Deduplicador
fuente
0

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

    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 mitad de formato decente (si el formato es complicado, es porque la condición if es complicada), y tiene al menos la oportunidad de descubrir si algo no tiene sentido. En comparación con su basura formateada si, cualquier otra cosa es mejor. Pero parece que solo puede hacer extremos: 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 escribiria

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;
}
gnasher729
fuente
-4

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

Idealmente, creo que compararía metaprográticamente las propiedades, por lo que no tiene que definir un nuevo método o si se bifurca cada vez que desea comparar un nuevo conjunto de propiedades.

No estoy seguro acerca 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
}
usuario1152226
fuente
1
No debería ser un problema implementar algo como esto en C #.
Snoop
No veo cómo una combinación booleana de ~ 5 llamadas compareContextProp(propVal, "PropertyId", context.Definition.Id)sería más fácil de leer que la combinación booleana del OP de ~ 5 comparaciones del formulario propVal.PropertyId == context.Definition.Id. Es significativamente más largo y agrega una capa adicional sin ocultar realmente la complejidad del sitio de la llamada. (si importa, no voté en contra)
Ixrec