Em que ponto a brevidade não é mais uma virtude?

100

Uma correção de bug recente exigiu que eu consultasse o código escrito por outros membros da equipe, onde encontrei isso (é C #):

return (decimal)CostIn > 0 && CostOut > 0 ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 : 0;

Agora, permitindo que haja uma boa razão para todos esses elencos, isso ainda parece muito difícil de seguir. Houve um pequeno erro no cálculo e tive que desvinculá-lo para corrigir o problema.

Eu conheço o estilo de codificação dessa pessoa a partir da revisão de código, e sua abordagem é que a menor é quase sempre melhor. E, claro, há valor lá: todos nós já vimos cadeias desnecessariamente complexas de lógica condicional que poderiam ser arrumadas com alguns operadores bem posicionados. Mas ele é claramente mais hábil do que eu em seguir cadeias de operadores espremidos em uma única declaração.

Isto é, obviamente, uma questão de estilo. Mas alguma coisa foi escrita ou pesquisada em reconhecer o ponto em que a busca pela brevidade do código deixa de ser útil e se torna uma barreira para a compreensão?

Nota de rodapé:

O motivo dos lançamentos é o Entity Framework. O banco de dados precisa armazená-los como tipos anuláveis. Decimal? não é equivalente a Decimal em C # e precisa ser convertido.

    
por Matt Thrower 05.01.2017 / 17:17
fonte

14 respostas

159

Para responder à sua pergunta sobre a pesquisa existente

But has anything been written or researched on recognizing the point where striving for code brevity stops being useful and becomes a barrier to comprehension?

Sim, tem havido trabalho nesta área.

Para entender essas coisas, você precisa encontrar uma maneira de calcular uma métrica para que as comparações possam ser feitas em uma base quantitativa (em vez de apenas realizar a comparação baseada em sagacidade e intuição , como as outras respostas fazem). Uma métrica potencial que foi analisada é

Complexidade Ciclomática ÷ Linhas de Código de Origem ( SLOC )

No seu exemplo de código, essa proporção é muito alta, porque tudo foi compactado em uma linha.

The SATC has found the most effective evaluation is a combination of size and [Cyclomatic] complexity. The modules with both a high complexity and a large size tend to have the lowest reliability. Modules with low size and high complexity are also a reliability risk because they tend to be very terse code, which is difficult to change or modify.

Link

Aqui estão algumas referências se você estiver interessado:

McCabe, T. e A. Watson (1994), Complexidade de Software (CrossTalk: Jornal de Engenharia de Software de Defesa).

Watson, A. H., & McCabe, T. J. (1996). Teste Estruturado: Uma Metodologia de Teste Usando a Métrica de Complexidade Cyclomática (Publicação Especial NIST 500-235). Retirado 14 de maio de 2011, do site da McCabe Software: link

Rosenberg, L., Hammer, T., Shaw, J. (1998). Métricas de Software e Confiabilidade (Anais do Simpósio Internacional IEEE em Engenharia de Confiabilidade de Software). Obtido em 14 de maio de 2011, no site da Penn State University: link

Minha opinião e solução

Pessoalmente, eu tenho nunca valor de brevidade, apenas legibilidade. Às vezes, a brevidade ajuda a legibilidade, outras vezes não. O que é mais importante é que você está escrevendo Código realmente óbvio (ROC) em vez do código somente de escrita (WOC).

Apenas por diversão, aqui está como eu escreveria, e peça aos membros da minha equipe para escreverem:

if ((costIn <= 0) || (costOut <= 0)) return 0;
decimal changeAmount = costOut - costIn;
decimal changePercent = changeAmount / costOut * 100;
return changePercent;

Note também que a introdução das variáveis de trabalho tem o efeito colateral de ativar a aritmética de ponto fixo ao invés da aritmética inteira, então a necessidade de todos os lançamentos para decimal é eliminada.

    
por 05.01.2017 / 17:46
fonte
46

Brevidade é boa quando reduz a confusão em torno das coisas que importam, mas quando se torna terse , acumulando muitos dados relevantes muito densos para serem facilmente seguidos, os dados relevantes tornam-se desorganizados e você tem um problema.

Neste caso em particular, os lançamentos para decimal estão sendo repetidos várias vezes; provavelmente seria melhor reescrevê-lo em geral como algo como:

var decIn = (decimal)CostIn;
var decOut = (decimal)costOut;
return decIn > 0 && CostOut > 0 ? ((decOut - decIn ) / decOut) * 100 : 0;

De repente, a linha que contém a lógica é muito mais curta e cabe em uma linha horizontal, para que você possa ver tudo sem ter que rolar, e o significado é muito mais aparente.

    
por 05.01.2017 / 17:24
fonte
7

Embora eu não possa citar nenhuma pesquisa particular sobre o assunto, sugiro que todos os elencos dentro do seu código violem o princípio "Não Repita a Si Mesmo". O que seu código parece estar tentando fazer é converter costIn e costOut em Decimal e executar algumas verificações de integridade nos resultados de tais conversões e executar operações adicionais nesses valores convertidos se as verificações passar. Na verdade, seu código executa uma das verificações de integridade em um valor não convertido, aumentando a possibilidade de que o CostOut possa manter um valor maior que zero, mas menor que a metade do tamanho do menor que zeroDecimal que pode representar . O código seria muito mais claro se definisse variáveis do tipo Decimal para manter os valores convertidos e, em seguida, atuasse sobre eles.

Parece curioso que você esteja mais interessado na proporção das representações Decimal de costIn e costOut do que a proporção dos valores reais de costIn e costOut , a menos que o código seja também vai usar as representações decimais para algum outro propósito. Se o código fizer uso adicional dessas representações, isso seria mais um argumento para a criação de variáveis para conter essas representações, em vez de ter uma sequência contínua de conversões por todo o código.

    
por 05.01.2017 / 19:30
fonte
5

Eu olho para o código e pergunto "como pode um custo ser 0 (ou menos)?". Que caso especial isso indica? Código deve ser

bool BothCostsAreValidProducts = (CostIn > 0) && (CostOut > 0);
if (!BothCostsAreValidProducts)
  return NO_PROFIT;
else {
   // that calculation here...
}

Eu estou supondo que os nomes aqui: alterar BothCostsAreValidProducts e NO_PROFIT , conforme o caso.

    
por 05.01.2017 / 18:19
fonte
5
Brevidade deixa de ser uma virtude quando nos esquecemos de que é significa um fim em vez de uma virtude em si. Nós gostamos de brevidade porque ela se correlaciona com a simplicidade, e gostamos de simplicidade porque o código mais simples é mais fácil de entender e mais fácil de modificar e conter menos bugs. No final, queremos que o código atinja esse objetivo:

  1. Cumpra os requisitos de negócios com o mínimo de trabalho

  2. Evite erros

  3. Permita-nos fazer alterações no futuro que continuam a cumprir 1 e 2

Estes são os objetivos. Qualquer princípio ou método de design (seja KISS, YAGNI, TDD, SOLID, provas, sistemas de tipos, metaprogramação dinâmica, etc.) são apenas virtuosos na medida em que nos ajudam a atingir esses objetivos.

A linha em questão parece ter perdido a visão do objetivo final. Embora seja curto, não é simples. Ele realmente contém redundância desnecessária, repetindo a mesma operação de fundição várias vezes. O código de repetição aumenta a complexidade e a probabilidade de erros. A mistura do casting com o cálculo real também torna o código difícil de seguir.

A linha tem três preocupações: Guardas (caixa especial 0), tipo conversão e cálculo. Cada preocupação é bastante simples quando tomada isoladamente, mas como tudo foi misturado na mesma expressão, torna-se difícil de seguir.

Não está claro por que CostOut não é convertido na primeira vez em que é usado wile CostIn is. Pode haver uma boa razão, mas a intenção não é clara (pelo menos não sem contexto), o que significa que um mantenedor estaria receoso de mudar esse código porque pode haver algumas suposições ocultas. E isso é um anátema para a manutenção.

Como CostIn é convertido antes de comparar com 0, presumo que seja um valor de ponto flutuante. (Se fosse um int não haveria motivo para lançar). Mas se CostOut é um float então o código pode esconder um erro obscuro de divisão por zero, já que um valor de ponto flutuante pode ser pequeno mas diferente de zero, mas zero quando convertido para um decimal (pelo menos eu acredito que isso seria possível.)

Portanto, o problema não é a brevidade ou a falta dela, o problema é a lógica repetida e a confluência de preocupações que levam a um código de difícil manutenção.

A introdução de variáveis para manter os valores lançados provavelmente aumentaria o tamanho do código contado no número de tokes, mas diminuiria a complexidade, separaria as preocupações e melhoraria a clareza, o que nos aproxima do objetivo de código que é mais fácil de entender e manter .

    
por 07.01.2017 / 12:02
fonte
2

Em meus anos de experiência, acredito que a brevidade final é a do tempo - o tempo domina tudo o resto. Isso inclui tanto o tempo de desempenho - quanto tempo um programa demora para fazer um trabalho - quanto o tempo de manutenção - quanto tempo leva para adicionar recursos ou corrigir bugs. (Como você equilibra os dois depende de quantas vezes o código em questão está sendo executado versus melhorado - lembre-se que a otimização prematura ainda é a raiz de todo o mal .) Brevidade de código é para melhorar a brevidade de ambos; códigos mais curtos geralmente são executados mais rapidamente e geralmente são mais fáceis de entender e, portanto, de manter. Se isso não acontecer, então é negativo.

No caso mostrado aqui, acho que a brevidade do texto foi mal interpretada como brevidade da contagem de linhas, em detrimento da legibilidade, o que pode aumentar o tempo de manutenção. (Também pode levar mais tempo para executar, dependendo de como o casting é feito, mas a menos que a linha acima seja executada milhões de vezes, provavelmente não é uma preocupação.) Nesse caso, as miniaturas repetitivas decimam da legibilidade, já que é mais difícil veja qual é o cálculo mais importante. Eu teria escrito da seguinte forma:

decimal dIn = (decimal)CostIn;
decimal dOut = (decimal)CostOut;
return dIn > 0 && CostOut > 0 ? ((dOut - dIn) / dOut) * 100 : 0;

(Edit: este é o mesmo código da outra resposta, então lá vai você.)

Sou fã do operador ternário ? : , então deixaria isso em branco.

    
por 05.01.2017 / 17:31
fonte
2

Como quase todas as respostas acima, a legibilidade deve ser sempre o seu objetivo principal. Eu também no entanto pensando formatação pode ser uma maneira mais eficaz para conseguir isso sobre a criação de variáveis e novas linhas.

return ((decimal)CostIn > 0 && CostOut > 0) ?
       100 * ( (decimal)CostOut - (decimal)CostIn ) / (decimal)CostOut:
       0;

Eu concordo strongmente com o argumento da complexidade ciclomática na maioria dos casos, no entanto, sua função parece ser pequena e simples o suficiente para lidar melhor com um bom caso de teste. Por curiosidade, por que a necessidade de converter para decimal?

    
por 05.01.2017 / 21:22
fonte
1

Brevidade não é uma virtude em absoluto. A legibilidade é a virtude.

Brevidade pode ser uma ferramenta para alcançar a virtude, ou, como no seu exemplo, pode ser uma ferramenta para alcançar algo exatamente oposto. De um jeito ou de outro, quase não tem valor próprio. A regra de que o código deve ser "o mais curto possível" pode ser substituída por "tão obsceno quanto possível" da mesma forma - todos são igualmente sem sentido e potencialmente prejudiciais se não servem a um propósito maior.

Além disso, o código que você postou nem segue a regra da brevidade. Se as constantes fossem declaradas com sufixo M, a maioria dos (decimal) casts horríveis poderia ser evitada, pois o compilador promoveria os int a decimal restantes. Acredito que a pessoa que você está descrevendo esteja apenas usando a brevidade como desculpa. Muito provavelmente não deliberadamente, mas ainda assim.

    
por 08.01.2017 / 18:29
fonte
1

Para mim, parece que um grande problema de legibilidade está na completa falta de formatação.

Eu escreveria assim:

return (decimal)CostIn > 0 && CostOut > 0 
            ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 
            : 0;

Dependendo de o tipo de CostIn e CostOut ser um tipo de ponto flutuante ou um tipo integral, alguns dos lançamentos também podem ser desnecessários. Ao contrário de float e double , os valores integrais são promovidos implicitamente para decimal .

    
por 08.01.2017 / 07:09
fonte
0

O código pode ser escrito com pressa, mas o código acima deve no meu mundo ser escrito com nomes de variáveis muito melhores.

E se eu ler o código corretamente, ele tentará fazer um cálculo de margem bruta.

var totalSales = CostOut;
var totalCost = CostIn;
var profit = (decimal)(CostOut - CostIn);
var grossMargin = 0m; //profit expressed as percentage of totalSales

if(profit > 0)
{
    grossMargin = profit/totalSales*100
}
    
por 06.01.2017 / 14:07
fonte
0

Estou supondo que CostIn * CostOut são inteiros | É assim que eu escreveria M (Dinheiro) é decimal

return CostIn > 0 && CostOut > 0 ? 100M * (CostOut - CostIn) / CostOut : 0M;
    
por 06.01.2017 / 01:18
fonte
0

O código é escrito para ser entendido pelas pessoas; a brevidade, neste caso, não compra muito e aumenta a carga do mantenedor. Para esta brevidade, você deve expandir isso tornando o código mais auto documentável (melhores nomes de variáveis) ou adicionando mais comentários explicando por que ele funciona dessa maneira.

Quando você escreve um código para resolver um problema hoje, esse código pode ser um problema amanhã, quando os requisitos mudarem. Manutenção sempre tem que ser levada em conta e melhorar a compreensibilidade do código é essencial.

    
por 06.01.2017 / 20:31
fonte
0

Brevidade não é mais uma virtude quando

  • Existe uma divisão sem uma verificação prévia para zero.
  • Não há verificação para null.
  • Não há limpeza.
  • TENTE CATCH versus lança a cadeia alimentar onde o erro pode ser manipulado.
  • Suposições são feitas sobre a ordem de conclusão de tarefas assíncronas
  • Tarefas usando atraso em vez de reagendar no futuro
  • O IO desnecessário é usado
  • Não usando atualização otimista
por 05.01.2017 / 18:56
fonte
0

Se isso estivesse passando nos testes da unidade de validação, então seria ótimo, se uma nova especificação fosse adicionada, um novo teste ou uma implementação aprimorada fosse necessária, e era necessário "desenredar" a definição do código, que é quando o problema surgiria.

Obviamente, um bug no código mostra que há outro problema com o Q / A, que era um descuido, então o fato de haver um bug que não foi detectado é motivo de preocupação.

Ao lidar com requisitos não funcionais, como "legibilidade" de código, ele precisa ser definido pelo gerente de desenvolvimento e gerenciado pelo desenvolvedor líder e respeitado pelos desenvolvedores para garantir implementações adequadas.

Tente garantir uma implementação padronizada de código (padrões e convenções) para garantir a "legibilidade" e a facilidade de "manutenção". Mas se esses atributos de qualidade não forem definidos e impostos, você terá o código como o exemplo acima.

Se você não gosta de ver esse tipo de código, tente colocar a equipe em acordo sobre os padrões e convenções de implementação e escreva-a e faça revisões aleatórias de código ou verificações pontuais para validar se a convenção está sendo respeitada. / p>     

por 07.01.2017 / 01:57
fonte