Como edito uma cadeia de if-else se as instruções aderirem aos princípios do Código Limpo do Uncle Bob?

Idealmente, acho que você deve extrair sua lógica para obter o código / número de alerta em seu próprio método. Assim, o código existente é reduzido até

{
    addAlert(GetConditionCode());
}

e você tem GetConditionCode () encapsular a lógica para verificar as condições. Talvez também seja melhor usar um Enum do que um número mágico.

private AlertCode GetConditionCode() {
    if (CheckCondition1()) return AlertCode.OnFire;
    if (CheckCondition2()) return AlertCode.PlagueOfBees;
    if (CheckCondition3()) return AlertCode.Godzilla;
    if (CheckCondition4()) return AlertCode.ZombieSharkNado;
    return AlertCode.None;
}

A medida importante é a complexidade do código, não o tamanho absoluto. Supondo que as diferentes condições sejam realmente apenas chamadas de função única, assim como as ações não são mais complexas do que as que você mostrou, eu diria que não há nada de errado com o código. Já é tão simples quanto pode ser.

Qualquer tentativa de simplificar ainda mais irá complicar as coisas.

É claro que você pode substituir a palavra-chave else por return , como outras pessoas sugeriram, mas isso é apenas uma questão de estilo, não uma mudança na complexidade.

Além de:

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 lugares que exigem funções de uma linha e lugares que exigem funções de 50 linhas (sim, elas existem!). Perceba que existem locais que exigem declarações if() e que existem locais que exigem o operador ?: . Use o arsenal completo que está à sua disposição e tente sempre usar a ferramenta mais adequada que você encontrar. E lembre-se, não fique religioso nem mesmo sobre esse conselho.

É controverso se isso é "melhor" do que o simples se..seja para um determinado caso. Mas se você quiser tentar outra coisa, essa é uma maneira comum de fazer isso.

Ponha suas condições em objetos e coloque esses objetos em uma lista

foreach(var condition in Conditions.OrderBy(i=>i.OrderToRunIn))
{
    if(condition.EvaluatesToTrue())
    {
        addAlert(condition.Alert);
        break;
    }
}

Se várias ações forem necessárias sob condição, você pode fazer uma recursão maluca

void RunConditionalAction(ConditionalActionSet conditions)
{
    foreach(var condition in conditions.OrderBy(i=>i.OrderToRunIn))
    {
        if(condition.EvaluatesToTrue())
        {
            RunConditionalAction(condition);
            break;
        }
    }
}

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 / framework.

Mas o seu exemplo tem um padrão

Um caso de uso comum para esse padrão seria a validação. Em vez de:

bool IsValid()
{
    if(condition1 == false)
    {
        throw new ValidationException("condition1 is wrong!");
    }
    elseif(condition2 == false)
    {
    ....

}

Torna-se

[MustHaveCondition1]
[MustHaveCondition2]
public myObject()
{
    [MustMatchRegExCondition("xyz")]
    public string myProperty {get;set;}
    public bool IsValid()
    {
        conditions = getConditionsFromReflection()
        //loop through conditions
    }
}

Considere o uso de return; após uma condição ter sido bem-sucedida, você economiza todos os else s. Você pode até mesmo ser capaz de return addAlert(1) diretamente se esse método tiver um valor de retorno.

Eu vi construções como essas consideradas mais limpas às vezes:

switch(true) {
    case cond1(): 
        statement1; break;
    case cond2():
        statement2; break;
    case cond3():
        statement3; break;
    // .. etc
}

O ternário com espaçamento correto também pode ser uma boa alternativa:

cond1() ? statement1 :
cond2() ? statement2 :
cond3() ? statement3 : (null);

Eu acho que você também pode tentar criar um array com par contendo condição e função e iterar até que a primeira condição seja atendida - o que, como eu vejo, seria igual à primeira resposta de Ewan.

Como uma variante da resposta de @ Ewan, você pode criar uma cadeia (em vez de uma "lista simples") de condições como esta:

abstract class Condition {
  private static final  Condition LAST = new Condition(){
     public void alertOrPropagate(DisplayInterface display){
        // do nothing;
     }
  }
  private Condition next = Last;

  public Condition setNext(Condition next){
    this.next = next;
    return this; // fluent API
  }

  public void alertOrPropagate(DisplayInterface display){
     if(isConditionMeet()){
         display.alert(getMessage());
     } else {
       next.alertOrPropagate(display);
     }
  }
  protected abstract boolean isConditionMeet();
  protected abstract String getMessage();  
}

Desta forma, você pode aplicar suas condições em uma ordem definida e a infra-estrutura (a classe abstrata mostrada) pula as verificações restantes após a primeira ter sido atendida.

É onde é superior à abordagem de "lista simples", na qual você precisa implementar o "salto" no loop que aplica as condições.

Você simplesmente configura a cadeia de condição:

Condition c1 = new Condition1().setNext(
  new Condition2().setNext(
   new Condition3()
 )
);

E comece a avaliação com uma simples chamada:

c1.alertOrPropagate(display);

Primeiro, o código original não é péssimo para o IMO. É bastante compreensível e não há nada de inerentemente ruim nisso.

Então, se você não gostar, crie a ideia do @ Ewan de usar uma lista, mas removendo o padrão foreach break , um tanto antinatural:

public class conditions
{
    private List<Condition> cList;
    private int position;

    public Condition Head
    {
        get { return cList[position];}
    }

    public bool Next()
    {
        return (position++ < cList.Count);
    }
}


while not conditions.head.check() {
  conditions.next()
}
conditions.head.alert()

Agora adapte isso na sua linguagem de escolha, faça de cada elemento da lista um objeto, uma tupla, qualquer coisa e você é bom.

EDIT: parece que não é tão claro que eu pensei, então deixe-me explicar mais. conditions é uma lista ordenada de algum tipo; head é o elemento atual que está sendo investigado - no começo é o primeiro elemento da lista, e cada vez que next() é chamado, ele se torna o seguinte; check() e alert() são checkConditionX() e addAlert(X) do OP.

A questão carece de alguns detalhes. Se as condições forem:

• sujeito a alterações ou
• repetido em outras partes do aplicativo ou sistema ou
• modificada em certos casos (como construções, testes e implementações diferentes)

ou se o conteúdo em addAlert for mais complicado, então uma solução possivelmente melhor em digamos c # seria:

//in some central spot
IEnumerable<Tuple<Func<bool>, int>> Conditions = new ... {
  Tuple.Create(CheckCondition1, 1),
  Tuple.Create(CheckCondition2, 2),
  ...
}

//at the original place
var matchingCondition = Conditions.Where(c=>c.Item1()).FirstOrDefault();
if(matchingCondition != null) 
  addAlert(matchingCondition.Item2)

As tuplas não são tão bonitas em c # < 8, mas escolhido para convienence.

Os profissionais com esse método, mesmo se nenhuma das opções acima se aplicarem, é que a estrutura é digitada estaticamente. Você não pode acidentalmente errar, digamos, perdendo um else .

A melhor maneira de reduzir a complexidade Cyclomatic nos casos em que você tem muito if->then statements é usar um < em> 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, um resultado de valor / função.

Por exemplo, em vez de (C #):

if (i > 10) { return "Two"; }
else if (i > 8) { return "Four" }
else if (i > 4) { return "Eight" }
return "Ten";  //etc etc say anything after 3 or 4 values

Eu posso simplesmente

var results = new Dictionary<int, string>
{
  { 10, "Two" },
  { 8, "Four"},
  { 4, "Eight"},
  { 0, "Ten"},
}

foreach(var key in results.Keys)
{
  if (i > results[key]) return results.Values[key];
}

Se você estiver usando linguagens mais modernas, você pode armazenar mais lógica e simplesmente valores também (c #). Isto é realmente apenas funções inline, mas você também pode apenas apontar para outras funções também, se a lógica for narly para colocar em linha.

var results = new Dictionary<Func<int, bool>, Func<int, string>>
{
  { (i) => return i > 10; ,
    (i) => return i.ToString() },
  // etc
};

foreach(var key in results.Keys)
{ 
  if (key(i)) return results.Values[key](i);
}

I'm trying to follow Uncle Bob's clean code suggestions and specifically to keep methods short.

I find myself unable to shorten this logic though:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

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 é apenas aparente que cada uma é diferente depois de reler o código cuidadosamente. Você deve adicionar nomes de formatação e função adequados, por exemplo:

if (is_an_apple()) {
  addAlert(1);
}
else if (is_a_banana()) {
  addAlert(2);
}
else if (is_a_cat()) {
  addAlert(3);
}
else if (is_a_dog()) {
  addAlert(4);
}

Seu código deve estar 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.

Supondo que todas as funções sejam implementadas no mesmo componente, você pode fazer com que as funções retenham algum estado para se livrar das múltiplas ramificações no fluxo.

EG: checkCondition1() se tornaria evaluateCondition1() , no qual ele verificaria se a condição anterior foi atendida; Em caso afirmativo, ele armazena em cache algum valor a ser recuperado por getConditionNumber() .

checkCondition2() se tornaria evaluateCondition2() , no qual verificaria se as condições anteriores foram atendidas. Se a condição anterior não foi atendida, ela verifica o cenário de condição 2, armazenando em cache um valor a ser recuperado por getConditionNumber() . E assim por diante.

clearConditions();
evaluateCondition1();
evaluateCondition2();
evaluateCondition3();
evaluateCondition4();
if (anyCondition()) { addAlert(getConditionNumber()); }

EDITAR:

Veja como a verificação de condições caras precisaria ser implementada para que essa abordagem funcione.

bool evaluateCondition34() {
    if (!anyCondition() && A && B && C) {
        conditionNumber = 5693;
        return true;
    }
    return false;
}

...

bool evaluateCondition76() {
    if (!anyCondition() && !B && C && D) {
        conditionNumber = 7658;
        return true;
    }
    return false;
}

clearConditions();
evaluateCondition10();
evaluateCondition9();
evaluateCondition8();
evaluateCondition7();
...
evaluateCondition34();
...
evaluateCondition76();

if (anyCondition()) { addAlert(getConditionNumber()); }

Esta resposta apenas fornece alguma sugestão alternativa das outras respostas, e provavelmente não será melhor do que o código original se considerarmos apenas 4 linhas de código. Embora, este não seja um abordagem terrível (e nem dificulta a manutenção como outros já disseram) dado o 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 do mesmo classe).

Quaisquer mais que 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) {   interruptor (condição)   {     caso Condition.Con1:       ...       pausa;     caso Condition.Con2:       ...       pausa;     etc ...   } Onde "Condição" é um enum apropriado. Se necessário, retorne um bool ou valor. Chame assim:   AlertOnCondition (GetCondition ());

Realmente não pode ser mais simples, E é mais rápido que a cadeia if-else uma vez você excede alguns casos.

Eu não posso falar pela sua situação em particular porque o código não é específico, mas ...

código como esse é muitas vezes 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 ao invés de reconhecer essas entidades e criar uma instância de classe para cada uma, você as trata como uma coisa e tenta compensar isso mais tarde, em um momento em que você realmente precisa saber com o que você está lidando para prosseguir.

O polimorfismo pode ter melhor lhe convier.

Suspeite de código com métodos longos que contenham construções longas ou complexas if-then. Você geralmente quer uma árvore de classes com alguns métodos virtuais.