Invertendo uma instrução IF

37

Então eu tenho programado há alguns anos e recentemente comecei a usar o ReSharper mais. Uma coisa que o ReSharper sempre sugere para mim é "inverter" se "instrução para reduzir o aninhamento".

Digamos que eu tenha este código:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

E o ReSharper sugerirá que eu faça isso:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Parece que o ReSharper SEMPRE sugere que eu inverto IFs, não importa quanto aninhamento esteja acontecendo. Esse problema é que eu meio que gosto de aninhar em pelo menos algumas situações. Para mim, parece mais fácil ler e descobrir o que está acontecendo em certos lugares. Isso nem sempre é o caso, mas às vezes me sinto mais confortável em aninhar.

Minha pergunta é: diferente da preferência pessoal ou da legibilidade, existe uma razão para reduzir o aninhamento? Existe alguma diferença de desempenho ou algo que eu possa não estar ciente?

    
por Barry Franklin 25.07.2017 / 13:42
fonte

8 respostas

61

Depende. No seu exemplo, ter a instrução if não invertida não é um problema, porque o corpo do if é muito curto. No entanto, você geralmente quer inverter as declarações quando os corpos crescem.

Somos apenas humanos e é difícil manter várias coisas ao mesmo tempo em nossas cabeças.

Quando o corpo de uma instrução é grande, inverter a instrução não apenas reduz o aninhamento, mas também diz ao desenvolvedor que eles podem esquecer tudo o que está acima dessa declaração e se concentrar apenas no resto. É muito provável que você use as instruções invertidas para se livrar de valores errados o mais rápido possível. Uma vez que você sabe que estão fora do jogo, a única coisa que resta são os valores que podem ser processados.

    
por 25.07.2017 / 13:55
fonte
33

Não evite negativos. Os humanos gostam de analisá-los mesmo quando a CPU os ama.

No entanto, respeite as expressões idiomáticas. Nós, humanos, somos criaturas de hábitos, então preferimos caminhos bem usados para direcionar caminhos cheios de ervas daninhas.

foo != null , infelizmente, se tornou uma expressão idiomática. Eu mal noto mais as partes separadas. Eu olho para isso e apenas penso, "oh, é seguro pontuar de foo agora".

Se você quiser derrubar isso (oh, algum dia, por favor), você precisa de um caso realmente strong. Você precisa de algo que permita que meus olhos deslizem sem esforço e entendam em um instante.

No entanto, o que você nos deu foi o seguinte:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

O que não é nem mesmo estruturalmente o mesmo!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

Isso não está fazendo o que meu cérebro preguiçoso esperava. Eu pensei que poderia continuar adicionando código independente, mas não posso. Isso me faz pensar em coisas que não quero pensar agora. Você quebrou meu idioma, mas não me deu um melhor. Tudo porque você queria me proteger do único negativo que queimou seu egozinho desagradável em minha alma.

Desculpe, talvez o ReSharper esteja certo em sugerir isso 90% do tempo, mas em cheques com nulidade eu acho que você encontrou um caso no qual é melhor ignorá-lo. Ferramentas simplesmente não são boas em ensinar o que é código legível. Pergunte a um humano. Faça uma revisão por pares. Mas não siga cegamente a ferramenta.

    
por 25.07.2017 / 19:30
fonte
20

É o velho argumento sobre se uma ruptura é pior que o aninhamento.

As pessoas parecem aceitar o retorno antecipado de verificações de parâmetro, mas é desaprovado na maior parte de um método.

Pessoalmente eu evito continuar, quebrar, ir etc, mesmo que isso signifique mais aninhamento. Mas na maioria dos casos você pode evitar ambos.

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Obviamente, o aninhamento dificulta as coisas quando você tem muitas camadas

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

O pior é quando você tem os dois

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}
    
por 25.07.2017 / 14:08
fonte
6

O problema com o continue é que, se você adicionar mais linhas ao código, a lógica ficará complicada e provavelmente conterá bugs. por exemplo,

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

Deseja chamar doSomeOtherFunction() para all someObject ou somente se não for nulo? Com o código original você tem a opção e é mais claro. Com o continue pode-se esquecer "oh, sim, lá atrás nós pulamos nulos" e você nem sequer tem a opção de chamá-lo para todos os objetos, a menos que você coloque essa chamada no início do método que pode não ser possível ou correto por outras razões.

Sim, muito aninhamento é feio e possivelmente confuso, mas neste caso eu usaria o estilo tradicional.

Pergunta bônus: Por que existem nulos nessa lista para começar? Pode ser melhor nunca adicionar um nulo em primeiro lugar, caso em que a lógica de processamento é muito mais simples.

    
por 25.07.2017 / 17:58
fonte
2

Essa técnica é útil para certificar pré-condições quando o principal volume de seu método ocorre quando essas condições são atendidas.

Nós frequentemente invertemos ifs no meu trabalho e empregamos um fantasma em outro lugar. Costumava ser bastante frequente que, ao revisar um PR, eu pudesse apontar como meu colega poderia remover três níveis de aninhamento! Acontece com menos frequência, já que lançamos nossos ifs.

Aqui está um exemplo de brinquedo de algo que pode ser implementado

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Código como este, onde há UM caso interessante (onde o resto simplesmente retorna ou pequenos blocos de instruções) é comum. É trivial reescrever o acima como

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

Aqui, nosso código significativo, as 10 linhas não incluídas, não tem três recuos na função. Se você tiver o primeiro estilo, você será tentado a refatorar o longo bloco em um método ou tolerar o recuo. É onde a poupança entra. Em um lugar, isso é uma economia trivial, mas em muitos métodos em muitas classes, você economiza a necessidade de gerar uma função extra para tornar o código mais limpo e você não tem tanto hediondo recuo de vários níveis .

Em resposta ao exemplo de código do OP, eu concordo que isso é um colapso excessivamente grande. Especialmente desde que em 1TB, o estilo que eu uso, a inversão faz com que o código aumente de tamanho.

    
por 26.07.2017 / 03:10
fonte
2

A ideia é o retorno antecipado. O início de return em um loop se torna inicial em continue .

Muitas boas respostas sobre essa questão: Devo retornar de uma função cedo ou usar uma declaração if?

Observe que, embora a questão seja encerrada como baseada em opinião, 430+ votos a favor do retorno antecipado, ~ 50 votos são "depende" e 8 são contra retorno antecipado. Portanto, há um consenso excepcionalmente strong (ou isso, ou eu sou ruim em contar, o que é plausível).

Pessoalmente, se o corpo do loop estivesse sujeito à continuação antecipada e longo o suficiente para que importasse (3-4 linhas), tenho a tendência de extrair o corpo do loop em uma função em que eu uso retorno inicial em vez de continuar antecipadamente.

    
por 26.07.2017 / 12:04
fonte
0

As sugestões de resharpers costumam ser úteis, mas devem ser sempre avaliadas criticamente. Ele procura por alguns padrões de código pré-definidos e sugere transformações, mas não pode levar em conta todos os fatores que tornam o código mais ou menos legível para os seres humanos.

Todas as outras coisas sendo iguais, menos aninhamento é preferível, e é por isso que o ReSharper sugere uma transformação que reduz o aninhamento. Mas todas as outras coisas são não iguais: neste caso, a transformação requer a introdução de um continue , o que significa que o código resultante é realmente mais difícil de seguir.

Em alguns casos, trocar um nível de aninhamento por continue poderia de fato ser uma melhoria, mas isso depende da semântica do código em questão, que está além do entendimento das regras mecânicas do ReSharpers.

No seu caso, a sugestão de ReSharper tornaria o código pior. Então apenas ignore isso. Ou melhor: não use a transformação sugerida, mas pense em como o código poderia ser melhorado. Observe que você pode estar atribuindo a mesma variável várias vezes, mas somente a última atribuição tem efeito, uma vez que a anterior seria apenas sobrescrita. Isso faz parecer um bug. A sugestão de ReSharpers não corrige o bug, mas torna um pouco mais óbvio que alguma lógica duvidosa esteja acontecendo.

    
por 26.07.2017 / 13:01
fonte
0

Acho que o ponto principal aqui é que o propósito do loop dita a melhor maneira de organizar o fluxo dentro do loop. Se você está filtrando alguns elementos antes de percorrer o restante, então continue -ing out faz sentido. No entanto, se você estiver fazendo diferentes tipos de processamento dentro de um loop, talvez faça mais sentido tornar esse if realmente óbvio, como:

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Observe que, no Java Streams ou em outras expressões funcionais, é possível separar a filtragem do loop, usando:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

Por acaso, essa é uma das coisas que adoro no caso do inverted if ( unless do Perl) aplicado a declarações únicas, o que permite o seguinte tipo de código, que eu acho muito fácil de ler:

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}
    
por 03.08.2017 / 01:11
fonte