Uma expressão booleana grande é mais legível que a mesma expressão dividida em métodos predicados? [fechadas]

What is easier to understand

A última abordagem. Não é apenas mais fácil de entender, mas é mais fácil escrever, testar, refatorar e estender também. Cada condição exigida pode ser dissociada e manuseada de maneira segura.

it's problematic because you have to read all the methods to understand the code

Não é problemático se os métodos forem nomeados corretamente. Na verdade, seria mais fácil entender como o nome do método descreveria a intenção da condição.
Para um espectador if MatchesDefinitionId() é mais explicativo do que if (propVal.PropertyId == context.Definition.Id)

[Pessoalmente, a primeira abordagem machuca meus olhos.]

Se este é o único local onde essas funções de predicado seriam usadas, você também pode usar variáveis bool locais:

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;
}

Eles também podem ser divididos e reordenados para torná-los mais legíveis, por exemplo, com

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

e, em seguida, substituindo todas as instâncias de propVal.SecondaryFilter.HasValue . Uma coisa que imediatamente se destaca é que hasNoSecondaryFilter usa o AND lógico nas propriedades HasValue negadas, enquanto matchesSecondaryFilter usa um AND lógico em HasValue não negado - portanto, não é exatamente o oposto.

Em geral, o último é o preferido.

Isso torna o site de chamadas mais reutilizável. Ele suporta DRY (o que significa que você tem menos lugares para mudar quando os critérios mudam e pode fazer isso de forma mais confiável). E muitas vezes esses sub-critérios são coisas que serão reutilizadas independentemente em outro lugar, permitindo que você faça isso.

Ah, e isso facilita muito o teste de unidade, dando a você a certeza de que fez isso corretamente.

Se for entre essas duas opções, a segunda é melhor. Estas não são as únicas opções, no entanto! Que tal dividir a única função em múltiplos ifs? Teste as maneiras de sair da função para evitar testes adicionais, emulando um "curto-circuito" em um teste de linha única.

Isso é mais fácil de ler (talvez seja necessário verificar novamente a lógica do seu exemplo, mas o conceito é verdadeiro):

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;   
}

Eu gosto mais da opção 2, mas sugiro uma mudança estrutural. Combine as duas verificações na última linha da condicional em uma única chamada.

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);
}

A razão pela qual eu sugiro isso é que as duas verificações são uma única unidade funcional e o parêntese aninhado em uma condicional está propenso a erros: Tanto do ponto de vista inicial de escrever o código quanto do ponto de vista da pessoa que está lendo. Isso é especialmente o caso se os subelementos da expressão não seguirem o mesmo padrão.

Não tenho certeza se MatchesSecondaryFilterIfPresent() é o melhor nome para a combinação; mas nada melhor vem imediatamente à mente.

Embora em C #, o código não é muito orientado a objetos. Está a utilizar métodos estáticos e o que parece ser campos estáticos (por exemplo, repo ). Em geral, considera-se que as estáticas dificultam a refatoração e a dificuldade de teste do código, ao mesmo tempo que dificultam a capacidade de reutilização e, para sua pergunta: o uso estático como esse é menos legível & sustentável do que a construção orientada a objetos.

Você deve converter esse código em um formulário mais orientado a objetos. Quando você fizer isso, descobrirá que há locais sensatos para colocar código que faz comparação de objetos, de campos, etc. É provável que você possa pedir que os objetos se comparem, o que reduziria sua grande instrução if a um pedido simples para comparar (por exemplo, if ( a.compareTo (b) ) { } , que pode incluir todas as comparações de campo).

O C # possui um rico conjunto de interfaces e utilitários de sistema para fazer comparações em objetos e seus campos. Além do óbvio método .Equals , para começar, procure IEqualityComparer , IEquatable e utilitários como System.Collections.Generic.EqualityComparer.Default .

O último é definitivamente preferido, tenho visto casos com o primeiro caminho e é quase sempre impossível de ler. Eu cometi o erro de fazer isso da primeira maneira e fui solicitado a mudá-lo para métodos predicados.

Eu diria que os dois são os mesmos, se você adicionar algum espaço em branco para legibilidade e alguns comentários para ajudar o leitor sobre as partes mais obscuras.

Lembre-se: um bom comentário diz ao leitor o que você estava pensando quando escreveu o código.

Com mudanças como as que sugeri, provavelmente usaria a abordagem anterior, pois é menos desordenada e difusa. As chamadas de sub-rotina são como notas de rodapé: fornecem informações úteis, mas interrompem o fluxo de leitura. Se os predicados fossem mais complexos, eu os dividiria em métodos separados para que os conceitos que eles incorporam possam ser compilados em partes compreensíveis.

Bem, se houver partes que você possa querer reutilizar, separá-las em funções separadas e nomeadas corretamente é obviamente a melhor idéia.
Mesmo que você nunca possa reutilizá-los, isso pode permitir que você estruture melhor suas condições e forneça um rótulo descrevendo o que eles significam .

Agora, vamos olhar para a sua primeira opção, e admitir que nem o seu recuo e quebra de linha foram tão úteis, nem a estrutura condicional foi tão bem assim:

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);
}

O primeiro é absolutamente horrível. Você tem usado || por duas coisas na mesma linha; isso é um bug no seu código ou uma intenção de ofuscar seu 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))));

Isso é pelo menos formatado decentemente (se a formatação é complicada, porque a condição if é complicada), e você tem pelo menos uma chance de descobrir se alguma coisa lá é absurda. Comparado ao seu lixo formatado se, qualquer outra coisa é melhor. Mas você parece ser capaz de fazer apenas extremos: ou uma confusão completa de uma declaração if, ou quatro métodos completamente inúteis.

Observe que (cond1 & cond2) || (! cond1 & cond3) pode ser escrito como

cond1 ? cond2 : cond3

que reduziria a bagunça. Eu escreveria

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;
}

Não gosto de nenhuma dessas soluções, elas são difíceis de avaliar e difíceis de ler. A abstração para métodos menores apenas para métodos menores nem sempre resolve o problema.

Idealmente, acho que você compararia metaprogrmamaticamente as propriedades, portanto, não é necessário definir um novo método ou ramificar toda vez que quiser comparar um novo conjunto de propriedades.

Eu não tenho certeza sobre c #, mas em JavaScript algo como isso seria MUITO melhor e poderia pelo menos substituir MatchesDefinitionId e MatchesParentId

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