Estou tentando seguir as sugestões de código limpo do tio Bob e especificamente para manter os métodos curtos.
Eu me acho incapaz de reduzir essa lógica:
if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}
Não consigo remover os elses e, assim, separar a coisa toda em bits menores, fazer com que o "else" no "else if" ajude o desempenho - avaliar essas condições é caro e se eu puder evitar avaliar as condições abaixo, faça com que uma das primeiras é verdade, eu quero evitá-los.
Mesmo semântica, avaliar a próxima condição se a anterior foi atendida não faz sentido do ponto de vista comercial.
editar: Esta questão foi identificada como uma possível duplicata de maneiras Elegant de lidar com se (se houver) outra coisa .
Eu acredito que essa é uma pergunta diferente (você pode ver isso também comparando as respostas dessas perguntas).
- Minha pergunta é verificar se a primeira condição de aceitação termina rapidamente .
- A questão vinculada está tentando ter todas as condições de aceitação para fazer algo. (melhor visto nesta resposta a essa pergunta: https://softwareengineering.stackexchange.com/a/122625/96955 )
fonte
Respostas:
Idealmente, acho que você deve extrair sua lógica para obter o código / número de alerta em seu próprio método. Portanto, seu código existente é reduzido completamente para
e você GetConditionCode () encapsula a lógica para verificar as condições. Talvez também seja melhor usar um Enum do que um número mágico.
fonte
addAlert
precise verificar a condição de alerta falsoAlertCode.None
.A medida importante é a complexidade do código, não o tamanho absoluto. Supondo que as condições diferentes sejam realmente apenas chamadas de função única, assim como as ações não são mais complexas do que o que você mostrou, eu diria que não há nada errado com o código. Já é tão simples quanto pode ser.
Qualquer tentativa de "simplificar" ainda mais complicará as coisas.
Obviamente, você pode substituir a
else
palavra-chave por umareturn
como outros sugeriram, mas isso é apenas uma questão de estilo, não uma mudança na complexidade.A parte, de lado:
Meu conselho geral seria: nunca ficar religioso sobre nenhuma regra de código limpo: a maioria dos conselhos de codificação que você vê na Internet é boa se aplicada em um contexto adequado, mas aplicar radicalmente esse mesmo conselho em todos os lugares pode lhe dar uma entrada no o IOCCC . O truque é sempre encontrar um equilíbrio que permita que os seres humanos raciocinem facilmente sobre seu código.
Use métodos muito grandes e você está ferrado. Use funções muito pequenas e você está ferrado. Evite expressões ternárias, e você está ferrado. Use expressões ternárias em todos os lugares e você está ferrado. Perceba que existem locais que exigem funções de uma linha e locais que exigem funções de 50 linhas (sim, eles existem!). Perceba que existem locais que solicitam
if()
declarações e que locais que solicitam o?:
operador. Use o arsenal completo à sua disposição e tente sempre usar a ferramenta mais adequada que você puder encontrar. E lembre-se, não fique religioso nem mesmo com esse conselho.fonte
else if
por um internoreturn
seguido por um simplesif
(remover oelse
) pode dificultar a leitura do código . Quando o código dizelse if
, eu sei imediatamente que o código no próximo bloco só será executado se o anterior não. Sem confusão, sem confusão. Se é simplesif
, pode ser executado ou não, independentemente de o anterior ter sido executado. Agora terei que gastar um pouco de esforço mental para analisar o bloco anterior e observar que ele termina com areturn
. Prefiro gastar esse esforço mental analisando a lógica de negócios.else if
forma uma unidade semântica. (Não é necessariamente uma única unidade para o compilador, mas tudo bem.)...; return; } if (...
Não; muito menos se estiver espalhado em várias linhas. Isso é algo que eu realmente terei que ver para ver o que está fazendo, em vez de ser capaz de entender diretamente, apenas vendo o par de palavras-chaveelse if
.else if
construção, especialmente porque sua forma encadeada é um padrão tão conhecido. No entanto, o código do formulárioif(...) return ...;
também é um padrão conhecido, então eu não condenaria isso completamente. No entanto, vejo isso como um problema menor: a lógica do fluxo de controle é a mesma nos dois casos, e um único olhar mais atento a umaif(...) { ...; return; }
escada me diz que é realmente equivalente a umaelse if
escada. Vejo a estrutura de um único termo, deduzo seu significado, percebo que ele se repete em todos os lugares e sei o que se passa.else if
ereturn
. por exemploelse if(...) { return alert();}
É controverso se isso é 'melhor' do que o normal se ... ou seja, para qualquer caso. Mas se você quiser tentar outra coisa, essa é uma maneira comum de fazê-lo.
Coloque suas condições em objetos e coloque esses objetos em uma lista
Se várias ações forem necessárias, com a condição de que você possa fazer alguma recursão maluca
Obviamente sim. Isso só funciona se você tiver um padrão para sua lógica. Se você tentar fazer uma ação condicional recursiva super genérica, a configuração do objeto será tão complicada quanto a instrução if original. Você estará inventando sua própria nova linguagem / estrutura.
Mas o seu exemplo não têm um padrão
Um caso de uso comum para esse padrão seria a validação. Ao invés de :
Torna-se
fonte
if...else
escada para a construção daConditions
lista. O ganho líquido é negativo, pois a construção deConditions
levará tanto código quanto o código OP, mas o indireto adicionado vem com um custo de legibilidade. Eu definitivamente preferiria uma escada com código limpo.conditions
é construído ... ARG! Não há atributos de anotação! Porque Deus? Ow meus olhos!Considere usar
return;
depois que uma condição for bem-sucedida, ela economiza todos oselse
s. Você pode até conseguirreturn addAlert(1)
diretamente se esse método tiver um valor de retorno.fonte
if
s ... Essa pode ser uma suposição razoável e, novamente, pode não ser.Já vi construções como essa consideradas mais limpas às vezes:
O ternário com espaçamento correto também pode ser uma alternativa interessante:
Eu acho que você também pode tentar criar uma matriz com um par contendo condição e função e iterar sobre ela até que a primeira condição seja atendida - o que, como eu vejo, seria igual à primeira resposta de Ewan.
fonte
case
etiquetas?Como uma variante da resposta de @ Ewan, você pode criar uma cadeia (em vez de uma "lista simples") de condições como esta:
Dessa forma, você pode aplicar suas condições em uma ordem definida e a infraestrutura (a classe abstrata mostrada) ignora as verificações restantes depois que a primeira é atendida.
É aqui que é superior à abordagem da "lista simples", na qual é necessário implementar o "pulo" no loop que aplica as condições.
Você simplesmente configura a cadeia de condições:
E comece a avaliação com uma chamada simples:
fonte
Primeiro de tudo, o código original não é terrível IMO. É bastante compreensível e não há nada inerentemente ruim nisso.
Então, se você não gostar, crie a idéia de @ Ewan de usar uma lista, mas remova seu
foreach break
padrão um tanto antinatural :Agora adapte isso no seu idioma de escolha, torne cada elemento da lista um objeto, uma tupla, o que for, e você será bom.
EDIT: parece que não está tão claro como pensei, então deixe-me explicar melhor.
conditions
é uma lista ordenada de algum tipo;head
é o elemento atual que está sendo investigado - no início, é o primeiro elemento da lista e, a cada vez quenext()
é chamado, passa a ser o seguinte;check()
ealert()
são oscheckConditionX()
eaddAlert(X)
do OP.fonte
while not
invés deforeach break
.A questão carece de alguns detalhes. Se as condições forem:
ou se o conteúdo
addAlert
for mais complicado, uma solução possivelmente melhor, digamos, c # seria:As tuplas não são tão bonitas em c # <8, mas são escolhidas por conveniência.
Os profissionais desse método, mesmo que nenhuma das opções acima se aplique, é que a estrutura é digitada estaticamente. Você não pode errar acidentalmente, digamos, perdendo um
else
.fonte
A melhor maneira de reduzir a complexidade ciclomática nos casos em que você tem muito
if->then statements
é usar um dicionário ou lista (dependente do idioma) para armazenar o valor da chave (se o valor da instrução ou algum valor de) e, em seguida, o resultado do valor / função.Por exemplo, em vez de (C #):
Eu posso simplesmente
Se você estiver usando linguagens mais modernas, poderá armazenar mais lógica, em seguida, simplesmente valores (c #). Na verdade, são apenas funções inline, mas você também pode apontar para outras funções também se a lógica for apenas colocar em linha.
fonte
Seu código já é muito curto, mas a lógica em si não deve ser alterada. À primeira vista, parece que você está se repetindo com quatro chamadas para
checkCondition()
, e é aparente que cada uma delas é diferente depois de reler o código com cuidado. Você deve adicionar nomes de formatação e função adequados, por exemplo:Seu código deve ser legível acima de tudo. Depois de ler vários livros do tio Bob, acredito que essa é a mensagem que ele está constantemente tentando transmitir.
fonte
Supondo que todas as funções sejam implementadas no mesmo componente, você pode fazer com que as funções mantenham algum estado para se livrar das várias ramificações no fluxo.
EG:
checkCondition1()
se tornariaevaluateCondition1()
, no qual verificaria se a condição anterior foi atendida; Nesse caso, ele armazena em cache algum valor a ser recuperadogetConditionNumber()
.checkCondition2()
tornar-se-iaevaluateCondition2()
, no qual verificaria se as condições anteriores foram cumpridas. Se a condição anterior não foi atendida, ele verifica o cenário de condição 2, armazenando em cache um valor a ser recuperadogetConditionNumber()
. E assim por diante.EDITAR:
Veja como a verificação de condições caras precisaria ser implementada para que essa abordagem funcionasse.
Portanto, se você tiver muitas verificações caras a serem executadas e o conteúdo deste código permanecer privado, essa abordagem ajudará a mantê-la, permitindo alterar a ordem das verificações, se necessário.
Essa resposta fornece apenas algumas sugestões alternativas das outras respostas e provavelmente não será melhor que o código original se considerarmos apenas 4 linhas de código. Embora essa não seja uma abordagem terrível (e nem torne a manutenção mais difícil, como já foi dito), devido ao cenário que mencionei (muitas verificações, apenas a principal função exposta como pública, todas as funções são detalhes de implementação da mesma classe).
fonte
anyCondition() != false
.true
, o OP não deseja que a condição 3 seja avaliada.anyCondition() != false
funçõesevaluateConditionXX()
. Isso é possível de implementar. Se a abordagem do uso do estado interno não for desejada, eu entendo, mas o argumento de que isso não funciona não é válido.Mais de duas cláusulas "else" obrigam o leitor do código a percorrer toda a cadeia para encontrar a que interessa. Use um método como: void AlertUponCondition (Condição de condição) {switch (condition) {case Condition.Con1: ... break; case Condition.Con2: ... break; etc ...} Onde "Condição" é uma enumeração adequada. Se necessário, retorne um valor ou bool. Chame assim: AlertOnCondition (GetCondition ());
Realmente não pode ser mais simples, E é mais rápido que a cadeia if-else, quando você excede alguns casos.
fonte
Não posso falar pela sua situação em particular porque o código não é específico, mas ...
código como esse geralmente é um cheiro para um modelo OO ausente. Você realmente tem quatro tipos de coisas, cada uma associada ao seu próprio tipo de alerta, mas, em vez de reconhecer essas entidades e criar uma instância de classe para cada uma, você as trata como uma coisa e tenta compensá-las mais tarde, no momento em que realmente Você precisa saber com o que está lidando para prosseguir.
O polimorfismo pode ter sido melhor para você.
Suspeite de código com métodos longos que contêm construções if-then longas ou complexas. Você geralmente deseja uma árvore de classes com alguns métodos virtuais.
fonte