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

63

O que é mais fácil de entender, uma grande declaração booleana (bastante complexa) ou a mesma declaração dividida em métodos predicados (muito código extra para ler)?

Opção 1, a grande expressão 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));
    }

Opção 2, As condições divididas em 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;
    }

Eu prefiro a segunda abordagem, porque vejo os nomes dos métodos como comentários, mas entendo que é problemático porque você precisa ler todos os métodos para entender o que o código faz, para abstrair a intenção do código.

willem
fonte
13
A opção 2 é semelhante ao que Martin Fowler recomenda em seu livro de refatoração. Além disso, os nomes dos métodos servem como a intenção de todas as expressões aleatórias, o conteúdo dos métodos são apenas os detalhes da implementação que podem mudar com o tempo.
programador de
2
É realmente a mesma expressão? "Or" tem uma precedência menor que "And". De qualquer forma, o segundo informa sua intenção, o outro (primeiro) é técnico.
thepacker
3
O que @thepacker diz. O fato de fazer da primeira maneira cometeu um erro é uma pista muito boa de que a primeira maneira não é facilmente compreensível para um setor muito importante do seu público-alvo. Você mesmo!
21716 Steve Joplin
3
Opção 3: eu não gosto de nenhum dos dois. O segundo é ridiculamente detalhado, o primeiro não é equivalente ao segundo. Parênteses ajuda.
21816 David Hammen
3
Isso pode ser pedante, mas você não possui nenhuma if instrução em nenhum bloco de código. Sua pergunta é sobre expressões booleanas .
Kyle Strand

Respostas:

88

O que é mais fácil de entender

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 necessária pode ser dissociada e manuseada com segurança à sua maneira.

é problemático porque você precisa ler todos os métodos para entender o código

Não é problemático se os métodos forem nomeados corretamente. De fato, 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 queif (propVal.PropertyId == context.Definition.Id)

[Pessoalmente, a primeira abordagem me machuca os olhos.]

wonderbell
fonte
12
Se os nomes dos métodos forem bons, também será mais fácil entender.
220016
E, por favor, torne-os (nomes de métodos) significativos e curtos. Mais de 20 nomes de métodos de caracteres feriram meus olhos. MatchesDefinitionId()é limítrofe.
Mindwin
2
@Mindwin Se tudo se resume a uma escolha entre manter os nomes dos métodos "curtos" e mantê-los significativos, digo sempre o último. Curto é bom, mas não à custa da legibilidade.
Ajedi32
@ Ajedi32 não é necessário escrever um ensaio sobre o que o método faz no nome do método, nem ter nomes de métodos gramaticalmente sólidos. Se alguém mantiver os padrões de abreviação claros (em todo o grupo de trabalho ou organização), não haverá problemas com nomes curtos e legibilidade.
Mindwin
Use a lei de Zipf: torne as coisas mais detalhadas para desencorajar seu uso.
hoosierEE
44

Se esse for o único local em que essas funções de predicado seriam usadas, você também poderá usar boolvariáveis 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;
}

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

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

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

Simon Richter
fonte
3
Esta solução é muito boa e certamente escrevi muitos códigos semelhantes. É muito legível. A desvantagem, em comparação com a solução que publiquei, é a velocidade. Com esse método, você executa uma pilha de testes condicionais, não importa o quê. Na minha solução, as operações podem ser drasticamente reduzidas com base nos valores processados.
BuvinJ
5
Testes do @BuvinJ como os mostrados aqui devem ser bastante baratos, então, a menos que eu saiba que algumas das condições são caras ou que este seja um código extremamente sensível ao desempenho, eu usaria a versão mais legível.
svick
11
@svick Sem dúvida, é improvável que isso introduza um problema de desempenho na maioria das vezes. Ainda assim, se você pode reduzir as operações sem perder a legibilidade, por que não fazê-lo? Não estou convencido de que isso seja muito mais legível do que minha solução. Ele fornece "nomes" auto-documentados para os testes - o que é legal ... acho que tudo se resume ao caso de uso específico e à compreensibilidade dos testes por si só.
BuvinJ
Adicionando comentários podem ajudar a legibilidade também ...
BuvinJ
@BuvinJ O que eu realmente gosto nessa solução é que, ignorando tudo, exceto a última linha, posso entender rapidamente o que ela está fazendo. Eu acho que isso é mais legível.
svick
42

Em geral, este último é preferido.

Isso torna o site de chamada mais reutilizável. Ele suporta DRY (o que significa que você tem menos lugares para alterar quando os critérios mudam e pode fazê-lo de maneira mais confiável). E muitas vezes esses subcritérios são coisas que serão reutilizadas independentemente em outros lugares, permitindo que você faça isso.

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

Telastyn
fonte
11
Sim, embora sua resposta também deva abordar a correção do uso de repo, que parece um campo / propriedade estático, ou seja, uma variável global. Os métodos estáticos devem ser determinísticos e não usar variáveis ​​globais.
David Arno
3
@ DavidArno - embora isso não seja ótimo, parece tangencial à questão em questão. E sem mais código, é plausível que exista uma razão semi-válida para o design funcionar dessa maneira.
Telastyn
11
Sim, não importa o repositório. I teve para ofuscar o código um pouco, não quero compartilhar o código do cliente como está nas interwebs :)
Willem
23

Se estiver entre essas duas opções, a última será melhor. Essas não são as únicas opções, no entanto! Que tal dividir a função única em vários ifs? Teste maneiras de sair da função para evitar testes adicionais, emulando aproximadamente um "curto-circuito" em um teste de linha única.

Isso é mais fácil de ler (pode ser 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;   
}
BuvinJ
fonte
3
Por que recebi um voto negativo por isso alguns segundos após publicá-lo? Por favor, adicione um comentário quando você votar! Essa resposta funciona com a mesma rapidez e é mais fácil de ler. Então qual é o problema?
BuvinJ
2
@BuvinJ: Absolutamente nada de errado com isso. O mesmo que o código original, exceto que você não precisa lutar com uma dúzia de parênteses e uma única linha que se estende até o final da tela. Eu posso ler esse código de cima para baixo e entendê-lo imediatamente. Contagem WTF = 0.
gnasher729
11
Retornar diferente do final da função torna o código menos legível, não mais legível, IMO. Eu prefiro um ponto de saída único. Alguns bons argumentos nos dois sentidos neste link. stackoverflow.com/questions/36707/…
Brad Thomas
5
@ Brad Thomas Não posso concordar com o único ponto de saída. Geralmente, leva a parênteses aninhados profundos. O retorno termina o caminho, então para mim é muito mais fácil ler.
Borjab
11
@BradThomas Concordo plenamente com o Borjab. Evitar aninhamentos profundos é na verdade o motivo pelo qual uso esse estilo com mais frequência do que para interromper longas declarações condicionais. Eu costumo me encontrar escrevendo código com toneladas de aninhamentos. Então, comecei a procurar maneiras de dificilmente ir mais do que um ou dois aninhamentos, e meu código ficou MUITO mais fácil de ler e manter como resultado. Se você puder encontrar uma maneira de sair de sua função, faça-o o mais rápido possível! Se você puder encontrar uma maneira de evitar aninhamentos profundos e condicionais longos, faça isso!
BuvinJ
10

Eu gosto mais da opção 2, mas sugeriria uma mudança estrutural. Combine os dois cheques na última linha do 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 sugiro isso é que as duas verificações são uma única unidade funcional, e o aninhamento de parênteses em uma condição é suscetível a erros: do ponto de vista de escrever o código inicialmente e do ponto de vista da pessoa que o lê. Esse é 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.

Dan Neely
fonte
Muito bom, tentar explicar o que está sendo feito dentro dos métodos é realmente melhor do que apenas reestruturar chamadas.
Klaar
2

Embora em C #, o código não seja muito orientado a objetos. Ele está usando métodos estáticos e o que parece ser campos estáticos (por exemplo repo). Geralmente, a estática dificulta a refatoração e o teste do código, dificultando a reutilização e a sua pergunta: o uso estático como esse é menos legível e sustentável do que a construção orientada a objetos.

Você deve converter esse código em um formulário mais orientado a objetos. Ao fazê-lo, você descobrirá que existem lugares sensatos para colocar código que faz comparação de objetos, campos, etc. É provável que você possa pedir aos objetos que se comparem, o que reduziria sua declaração if grande a um solicitação simples de comparação (por exemplo if ( a.compareTo (b) ) { }, que pode incluir todas as comparações de campos).

O C # possui um rico conjunto de interfaces e utilitários de sistema para fazer comparações entre objetos e seus campos. Para além do óbvio .Equalsmétodo, para começar, olhar para IEqualityComparer, IEquatablee utilitários como System.Collections.Generic.EqualityComparer.Default.

Erik Eidt
fonte
0

Definitivamente, este último é o preferido, já vi casos com o primeiro caminho e é quase sempre impossível ler. Cometi o erro de fazê-lo da primeira maneira e me pediram para alterá-lo para predicar métodos.

Snoop
fonte
0

Eu diria que os dois são praticamente os mesmos, se você adicionar algum espaço em branco para facilitar a leitura e alguns comentários para ajudar o leitor nas 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 sugeri, eu provavelmente adotaria a abordagem anterior, pois ela é menos confusa e difusa. As chamadas de sub-rotina são como notas de rodapé: fornecem informações úteis, mas interrompem o fluxo da leitura. Se os predicados fossem mais complexos, eu os dividiria em métodos separados, para que os conceitos incorporados possam ser construídos em pedaços compreensíveis.

Mark Wood
fonte
Merece um +1. Bom alimento para o pensamento, embora não seja uma opinião popular com base nas outras respostas. Obrigado :)
willem
11
@ willem Não, ele não merece +1. Duas abordagens não são as mesmas. Os comentários adicionais são estúpidos e desnecessários.
220016
2
Um bom código NUNCA depende de comentários para ser compreensível. De fato, os comentários são a pior confusão que um código poderia ter. O código deve falar por si. Além disso, as duas abordagens que o OP deseja avaliar nunca podem ser "praticamente iguais", não importa quantos espaços em branco se adicione.
WonderBar # 03:
É melhor ter um nome de função completo do que ter que ler o comentário. Conforme declarado no livro "Código Limpo", um comentário é uma falha na expressão do código de lançamento. Por que explicar o que você está fazendo quando a função poderia tê-la declarado com muito mais clareza?
Borjab
0

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

Agora, vejamos a sua primeira opção e admitimos que nem o seu recuo e quebra de linha foram tão úteis nem o condicional foi estruturado tão bem:

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);
}
Desduplicador
fonte
0

O primeiro é absolutamente horrível. Você está usando || 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))));

É pelo menos meio caminho decentemente formatado (se a formatação é complicada, é porque a condição if é complicada), e você tem pelo menos uma chance de descobrir se há algo sem sentido. Comparado com o seu lixo formatado se, qualquer outra coisa for melhor. Mas você parece capaz de fazer apenas extremos: uma bagunça completa de uma declaração if ou quatro métodos completamente inúteis.

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

cond1 ? cond2 : cond3

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

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

Idealmente, acho que você compararia metaprogrmaticamente propriedades, para não definir um novo método ou se ramificar toda vez que quiser comparar um novo conjunto de propriedades.

Eu não tenho certeza sobre c #, mas em javascript algo assim 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
}
user1152226
fonte
11
Não deve ser um problema implementar algo assim em C #.
Snoop
Não estou vendo como compareContextProp(propVal, "PropertyId", context.Definition.Id)seria mais fácil ler uma combinação booleana de ~ 5 chamadas do que a combinação booleana do OP de ~ 5 comparações do formulário propVal.PropertyId == context.Definition.Id. É significativamente mais longo e adiciona uma camada extra sem realmente esconder qualquer complexidade do site de chamada. (se isso realmente importa, eu não downvote)
Ixrec