Maneiras elegantes de lidar com se (se mais)

161

Esta é uma pequena coisinha, mas toda vez que tenho que codificar algo assim, a repetição me incomoda, mas não tenho certeza de que nenhuma das soluções seja pior.

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • Existe um nome para esse tipo de lógica?
  • Eu sou um pouco TOC demais?

Estou aberto a sugestões de códigos malignos, mesmo que por curiosidade ...

Benjol
fonte
8
@Emmad Kareem: duas DefaultActionchamadas viola o princípio DRY
Abyx
Obrigado pela sua resposta, mas acho que está tudo bem, exceto por não usar o try / catch, pois pode haver erros que não retornam resultados e causariam cancelamentos (dependendo da sua linguagem de programação).
NoChance
20
Eu acho que a questão principal aqui é que você está trabalhando em níveis inconsistentes de abstração . O nível mais alto de abstração é: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). Os detalhes detalhados de se ter os dados de DoSomething () estão em um nível de abstração mais baixo e, portanto, devem estar em uma função diferente. Esta função terá um nome no nível de abstração mais alto e sua implementação será de baixo nível. As boas respostas abaixo abordam esse problema.
Gilad Naor
6
Por favor especifique um idioma. Possíveis soluções, idiomas padrão e normas culturais de longa data são diferentes para diferentes idiomas e vai levar a diferentes respostas à sua Q.
Caleb
1
Você pode consultar este livro "Refatoração: aprimorando o design do código existente". Existem várias seções sobre a estrutura if-else, prática realmente útil.
Vacker

Respostas:

96

Extraia-o para separar a função (método) e use a returninstrução:

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

Ou, talvez melhor, separe a obtenção de conteúdo e seu processamento:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Upd:

Por que não exceções, por OpenFileque não lança exceção de E / S:
Eu acho que é uma pergunta realmente genérica, em vez de uma pergunta sobre E / S de arquivo. Nomes como FileExists, OpenFilepodem ser confusos, mas se substituí-los por Foo, Baretc, - seria mais claro que DefaultActionpode ser chamado tantas vezes quanto DoSomething, portanto, pode ser um caso não excepcional. Péter Török escreveu sobre isso no final de sua resposta

Por que existe um operador condicional ternário na 2ª variante:
se houvesse tag [C ++], eu escrevi uma ifdeclaração com declaração contentsna parte de sua condição:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Porém, para outras linguagens (tipo C), if(contents) ...; else ...;é exatamente o mesmo que a expressão expression com operador condicional ternário, mas mais longo. Como a parte principal do código era get_contentsfunção, usei apenas a versão mais curta (e também o contentstipo omitido ). Enfim, está além dessa questão.

Abyx
fonte
93
+1 para retornos múltiplos - quando os métodos são feitos suficientemente pequenos , essa abordagem funcionou melhor para mim #
30611
Não sou um grande fã dos múltiplos retornos, embora eu o use ocasionalmente. É bastante razoável em algo simples, mas simplesmente não escala bem. Nosso padrão é evitá-lo para todos os métodos simples, exceto os loucos, porque os métodos tendem a crescer em tamanho mais do que encolhem.
precisa
3
Vários caminhos de retorno podem ter implicações negativas de desempenho em programas C ++, derrotando os esforços do otimizador para empregar RVO (também NRVO, a menos que cada caminho retorne o mesmo objeto).
Functastic
Eu recomendo reverter a lógica na 2ª solução: {if (file exist) {set contents; if (mais ou menos) {retorna o conteúdo; }} retornar nulo; } Simplifica o fluxo e reduz o número de linhas.
Wedge
1
Olá Abyx, notei que você incorporou alguns dos comentários dos comentários aqui: obrigado por fazer isso. Limpei tudo o que foi abordado na sua resposta e em outras respostas.
56

Se a linguagem de programação que você está usando (0) comparações binárias de curto-circuito (ou seja, se não chamar SomeTestse FileExistsretornar falso) e (1) a atribuição retornar um valor (o resultado de OpenFileé atribuído a contentse então esse valor é passado como argumento para SomeTest), você pode usar algo como o seguinte, mas ainda assim seria recomendável comentar o código, observando que o single =é intencional.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

Dependendo da complexidade do if, pode ser melhor ter uma variável de flag (que separa o teste de condições de sucesso / falha com o código que lida com o erro DefaultActionnesse caso)

frozenkoi
fonte
É assim que eu faria.
Anthony
13
Bastante nojento colocar tanto código em uma ifdeclaração, na minha opinião.
Moteutsch
15
Eu, pelo contrário, gosto desse tipo de declaração "se algo existe e atende a essa condição". 1
Gorpik
Eu também! Pessoalmente, não gosto da maneira como as pessoas usam retornos múltiplos, algumas premissas não são atendidas. Por que você não inverte esses ifs e executa seu código se eles são atendidos?
Klaar
"Se algo existe e atende a essa condição" está bom. "se algo existe e faz algo tangencialmente relacionado aqui e atende a essa condição", OTOH, é confuso. Em outras palavras, não gosto de efeitos colaterais em uma condição.
Piskvor
26

Mais seriamente do que a repetição da chamada para DefaultAction é o próprio estilo, porque o código é escrito não ortogonal (veja esta resposta por boas razões para escrever ortogonalmente).

Para mostrar por que o código não ortogonal é ruim, considere o exemplo original, quando é introduzido um novo requisito de que não devemos abrir o arquivo se ele estiver armazenado em um disco de rede. Bem, podemos apenas atualizar o código para o seguinte:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Mas também existe um requisito de que também não devemos abrir arquivos grandes com mais de 2 GB. Bem, apenas atualizamos novamente:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Deve ficar muito claro que esse estilo de código será uma enorme dor de manutenção.

Entre as respostas aqui escritas corretamente ortogonalmente estão o segundo exemplo de Abyx e a resposta de Jan Hudec , por isso não vou repetir isso, basta apontar que a adição dos dois requisitos nessas respostas seria apenas

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(ou em goto notexists;vez de return null;), não afetando nenhum outro código além das linhas adicionadas . Por exemplo, ortogonal.

Ao testar, a regra geral deve ser testar exceções, não o caso normal .

hlovdal
fonte
8
+1 para mim. Os retornos antecipados ajudam a evitar o anti-padrão da ponta da seta. Consulte codinghorror.com/blog/2006/01/flattening-arrow-code.html e lostechies.com/chrismissal/2009/05/27/… Antes de ler sobre esse padrão, sempre assinei a 1 entrada / saída por função teoria devido ao que eu fui ensinado 15 anos ou mais atrás. Eu sinto que isso torna o código muito mais fácil de ler e, como você menciona, é mais sustentável.
Moose
3
@ MrMoose: sua menção ao antipadrão de ponta de flecha responde à pergunta explícita de Benjol: "Existe um nome para esse tipo de lógica?" Publique como resposta e você terá meu voto.
out
Esta é uma ótima resposta, obrigado. E @MrMoose: "anti-ponta de flecha" possivelmente responde ao meu primeiro marcador, então sim, publique-o. Não posso prometer que aceitarei, mas merece votos!
Benjol 01/12/11
@outis. Obrigado. Eu adicionei resposta. O anti-padrão de ponta de flecha certamente é relevante no posto de hlovdal e suas cláusulas de guarda funcionam bem para contorná-las. Não sei como você poderia responder o segundo ponto a isso. Eu não estou qualificado para diagnosticar que :)
Sr. Alce
4
+1 em "exceções de teste, não no caso normal".
Roy Tinker
25

Obviamente:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Você disse que está aberto até a soluções más, então, usando o mal goto conta, não?

De fato, dependendo do contexto, essa solução pode ser menos mal do que o mal fazendo a ação duas vezes ou a variável extra do mal. Eu o envolvi em uma função, porque definitivamente não seria bom no meio da função longa (principalmente devido ao retorno no meio). Mas que a função longa não está OK, ponto final.

Quando você tiver exceções, elas serão mais fáceis de ler, especialmente se você puder fazer com que o OpenFile e DoSomething simplesmente joguem uma exceção se as condições não forem atendidas, para que você não precise de verificações explícitas. Por outro lado, em C ++, Java e C # lançando uma exceção é uma operação lenta, portanto, do ponto de desempenho, o goto ainda é preferível.


Nota sobre "evil": C ++ FAQ 6.15 define "evil" como:

Isso significa tal e tal é algo que você deve evitar a maioria do tempo, mas não é algo que você deve evitar todo o tempo. Por exemplo, você acabará usando essas coisas "más" sempre que forem "as menos más das alternativas más".

E isso se aplica gotoneste contexto. Construções estruturadas de controle de fluxo são melhores na maioria das vezes, mas quando você encontra uma situação em que eles acumulam muitos de seus próprios males, como atribuição em condição, aninhando mais de 3 níveis de profundidade, código duplicado ou condições longas, gotopode simplesmente terminar sendo menos mau.

Jan Hudec
fonte
11
Meu cursor está passando o mouse sobre o botão aceitar ... só para despejar todos os puristas. Oooohh a tentação: D
Benjol 30/11/11
2
Sim Sim! Esta é a maneira absolutamente "correta" de escrever código. A estrutura do código agora é "Se erro, manipule erro. Ação normal. Se erro, manipule erro. Ação normal", exatamente como deveria ser. Todo o código "normal" é escrito com apenas um recuo de nível único, enquanto todo o código relacionado a erros tem dois níveis de recuo. Portanto, o código normal E MAIS IMPORTANTE obtém o lugar visual mais proeminente e é possível ler de maneira muito rápida e fácil o fluxo sequencialmente para baixo. Por todos os meios, aceite esta resposta.
hlovdal
2
E outro aspecto é que o código escrito dessa maneira é ortogonal. Por exemplo, as duas linhas "if (! FileExists (file)) \ n \ tgoto não existe;" agora estão SOMENTE relacionados ao tratamento desse aspecto de erro único (KISS) e, o mais importante, não afeta nenhuma das outras linhas . Esta resposta stackoverflow.com/a/3272062/23118 lista vários bons motivos para manter o código ortogonal.
hlovdal
5
Falando das soluções más: Eu posso ter sua solução sem ir:for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
herby
4
@herby: Sua solução é mais má do que goto, porque você está abusando breakde uma maneira que ninguém espera que seja abusada; portanto, as pessoas que leem o código terão mais problemas para ver onde a ruptura os leva do que com goto, que diz explicitamente. Além disso, você está usando um loop infinito que será executado apenas uma vez, o que será bastante confuso. Infelizmente, do { ... } while(0)também não é exatamente legível, porque você só vê que é apenas um bloco engraçado quando você chega ao final e C não suporta quebra de outros blocos (ao contrário do perl).
Jan Hudec
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
herby
fonte
ou ir o macho extra e criar um método adicional FileExistsAndConditionMet (arquivo) ...
UncleZeiv
@herby SomeTestpode ter a mesma semântica que a existência de arquivo se SomeTestverificar o tipo de arquivo, por exemplo, verificar se .gif é realmente um arquivo GIF.
Abyx
1
Sim. Depende. @Benjol sabe melhor.
Herby
3
... é claro que eu quis dizer "vá além" ":)"
UncleZeiv
2
Isso está levando o ravioli até as extremidades, mesmo que eu não vá (e sou extremo nisso) ... acho que agora é bem legível, considerando o contents && f(contents). Duas funções para salvar mais uma ?!
Herby
12

Uma possibilidade:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Obviamente, isso torna o código um pouco mais complexo de uma maneira diferente. Portanto, é em grande parte uma questão de estilo.

Uma abordagem diferente seria usar exceções, por exemplo:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Isso parece mais simples, porém só é aplicável se

  • sabemos com precisão que tipo de exceções esperar e DefaultAction()se encaixa em cada
  • esperamos que o processamento do arquivo seja bem-sucedido e um arquivo ausente ou uma falha SomeTest()seja claramente uma condição incorreta; portanto, é adequado lançar uma exceção nele.
Péter Török
fonte
19
Noooo ~! Não é uma variável de flag, é definitivamente um caminho errado, porque leva a códigos complexos, difíceis de entender (onde essa bandeira se torna verdadeira) e difíceis de refatorar.
Abyx
Não se você o restringir ao escopo o mais local possível. (function () { ... })()em Javascript, { flag = false; ... }em C-like etc.
herby
+1 para a lógica de exceção, que poderia muito bem ser a solução mais adequada, dependendo do cenário.
Steven Jeuris
4
+1 Este mútuo 'Nooooo!' é engraçado. Eu acho que a variável status e o retorno antecipado são razoáveis ​​em certos casos. Em rotinas mais complexas, eu usaria a variável status porque, em vez de adicionar complexidade, o que realmente faz é tornar a lógica explícita. Nada de errado com isso.
3051111 grossjogel
1
Este é o nosso formato preferido onde eu trabalho. As 2 principais opções utilizáveis ​​parecem ser "retornos múltiplos" e "variáveis ​​de flag". Nenhum deles parece ter qualquer tipo de vantagem real, em média, mas ambos se encaixam em certas circunstâncias melhor do que em outras. Tem que ir com o seu caso típico. Apenas mais uma guerra religiosa "Emacs" vs. "Vi". :-)
Brian Knoblauch 30/11
11

Isso está em um nível mais alto de abstração:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

E isso preenche os detalhes.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}
Jeanne Pindar
fonte
11

Funções devem fazer uma coisa. Eles deveriam fazê-lo bem. Eles deveriam fazer isso apenas.
- Robert Martin em Código Limpo

Algumas pessoas acham essa abordagem um pouco extremada, mas também é muito limpa. Permita-me ilustrar em Python:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Quando ele diz que funções devem fazer uma coisa, ele quer dizer uma coisa. processFile()escolhe uma ação com base no resultado de um teste, e é tudo o que faz. fileMeetsTest()combina todas as condições do teste, e é tudo o que faz. contentsTest()transfere a primeira linha para firstLineTest()e é tudo o que faz.

Parece um monte de funções, mas é praticamente parecido com o inglês direto:

Para processar o arquivo, verifique se ele atende ao teste. Se isso acontecer, faça alguma coisa. Caso contrário, execute a ação padrão. O arquivo atende ao teste, se existir, e passa no teste de conteúdo. Para testar o conteúdo, abra o arquivo e teste a primeira linha. O teste para a primeira linha ...

É um pouco prolixo, mas observe que se um mantenedor não se importa com os detalhes, ele pode parar de ler após apenas as 4 linhas de código inseridas processFile()e ainda terá um bom conhecimento de alto nível sobre o que a função faz.

Karl Bielefeldt
fonte
5
+1 É um bom conselho, mas o que constitui "uma coisa" depende da camada atual de abstração. processFile () é "uma coisa", mas duas coisas: fileMeetsTest () e doSomething () ou defaultAction (). Receio que o aspecto "uma coisa" possa confundir iniciantes que não entendem a priori o conceito.
Caleb
1
É uma boa meta ... Isso é tudo o que tenho a dizer sobre isso ... ;-)
Brian Knoblauch
1
Não gosto de passar argumentos implicitamente como variáveis ​​de instância assim. Você fica cheio de variáveis ​​de instância "inúteis" e há muitas maneiras de danificar seu estado e quebrar os invariantes.
Hugomg 01/12/11
@ Caleb, ProcessFile () está realmente fazendo uma coisa. Como Karl diz em seu post, ele está usando um teste para decidir qual ação executar e adiando a implementação real das possibilidades de ação para outros métodos. Se você adicionar muitas ações alternativas, os critérios de finalidade única para o método ainda serão atendidos, desde que não ocorra aninhamento de lógica no método imediato.
S.Robins
6

Com relação ao que é chamado, ele pode se transformar facilmente no padrão anti-ponta de seta à medida que seu código cresce para lidar com mais requisitos (como mostra a resposta fornecida em https://softwareengineering.stackexchange.com/a/122625/33922 ) e então cai na armadilha de ter grandes seções de códigos com instruções condicionais aninhadas que se assemelham a uma seta.

Veja Links como;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

Há muito mais sobre esse e outros anti-padrões encontrados no Google.

Algumas ótimas dicas que Jeff fornece em seu blog sobre isso são;

1) Substitua as condições pelas cláusulas de guarda.

2) Decomponha blocos condicionais em funções separadas.

3) Converter cheques negativos em cheques positivos

4) Sempre retorne oportunisticamente o mais rápido possível da função.

Veja também alguns dos comentários no blog de Jeff sobre as sugestões de Steve McConnells sobre retorno antecipado;

"Use um retorno quando melhorar a legibilidade: em certas rotinas, depois de saber a resposta, você deseja devolvê-lo à rotina de chamadas imediatamente. Se a rotina for definida de tal forma que não exija nenhuma limpeza adicional depois de detecta um erro, não retornar imediatamente significa que você precisa escrever mais código ".

...

"Minimize o número de retornos em cada rotina: é mais difícil entender uma rotina quando, lendo-a na parte inferior, você não está ciente da possibilidade de que ela retorne a algum lugar acima. Por esse motivo, use retornos criteriosamente - somente quando eles melhorarem. legibilidade ".

Eu sempre assinei a teoria de 1 entrada / saída por função, devido ao que me ensinaram há 15 anos. Eu sinto que isso torna o código muito mais fácil de ler e, como você menciona, é mais sustentável

Mr Moose
fonte
6

Isso está de acordo com as regras DRY, no-goto e no-multiple-return, é escalável e legível na minha opinião:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
mouviciel
fonte
1
A conformidade com os padrões não é necessariamente igual ao bom código. No momento, estou indeciso sobre esse trecho de código.
precisa
isso apenas substitui 2 defaultAction (); com 2 iguais se condições e adiciona uma variável flag que imo é muito pior.
Ryathal 30/11
3
O benefício de usar uma construção como essa é que, à medida que o número de testes aumenta, o código não começa a aninhar mais ifs dentro de outros ifs. Além disso, o código para lidar com o caso malsucedido ( DefaultAction()) está apenas em um local e, para fins de depuração, o código não pula as funções auxiliares e adiciona pontos de interrupção às linhas nas quais a successvariável é alterada pode mostrar rapidamente quais testes foram aprovados (acima do acionado ponto de interrupção) e quais não foram testados (abaixo).
Congelados1
1
Yeeaah, eu sou o tipo de gostar dela, mas eu acho que eu iria mudar o nome successpara ok_so_far:)
Benjol
Isso é muito semelhante ao que faço quando (1) o processo é muito linear quando tudo dá certo e (2) você teria o anti-padrão de flecha. No entanto, tento evitar adicionar uma variável extra, o que geralmente é fácil se você pensar em termos de pré-requisitos para a próxima etapa (que é sutilmente diferente de perguntar se uma etapa anterior falhou). Se o arquivo existir, abra o arquivo. Se o arquivo estiver aberto, leia o conteúdo. Se eu tiver conteúdo, processe-o, caso contrário, execute a ação padrão.
Adrian McCarthy
3

Eu o extrairia para um método separado e, em seguida:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

o que também permite

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

então, você pode remover as DefaultActionchamadas e deixar a execução DefaultActiondo chamador:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

Também gosto da abordagem de Jeanne Pindar .

Konrad Morawski
fonte
3

Para este caso em particular, a resposta é fácil o suficiente ...

Há uma condição de corrida entre FileExistse OpenFile: o que acontece se o arquivo for removido?

A única maneira sensata de lidar com esse caso específico é pular FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

Isso resolve perfeitamente esse problema e torna o código mais limpo.

Em geral: tente repensar o problema e invente outra solução que evite o problema completamente.

Martin Wickman
fonte
2

Outra possibilidade, se você não gosta de ver muitos outros, é abandonar completamente o uso do else e lançar uma declaração de retorno extra. Else é uma espécie de supérfluo, a menos que exigem lógica mais complexa para determinar se há mais do que apenas duas possibilidades de ação.

Assim, seu exemplo pode se tornar:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Pessoalmente, não me importo de usar a cláusula else , pois ela afirma explicitamente como a lógica deve funcionar e, portanto, melhora a legibilidade do seu código. Algumas ferramentas de embelezamento de código, no entanto, preferem simplificar para uma única instrução if para desencorajar a lógica de aninhamento.

S.Robins
fonte
2

O caso mostrado no código de exemplo geralmente pode ser reduzido para uma única ifinstrução. Em muitos sistemas, a função de abertura de arquivo retornará um valor inválido se o arquivo ainda não existir. Às vezes, esse é o comportamento padrão; outras vezes, ele deve ser especificado por meio de um argumento. Isso significa que o FileExiststeste pode ser descartado, o que também pode ajudar nas condições de corrida resultantes da exclusão do arquivo entre o teste de existência e a abertura do arquivo.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Isso não aborda diretamente o problema da mistura no nível de abstração, pois evita completamente o problema de vários testes não encadeados, embora acabar com o teste de existência de arquivo não seja incompatível com a separação dos níveis de abstração. Supondo que identificadores de arquivo inválidos sejam equivalentes a "false" e identificadores de arquivo fecham quando ficam fora do escopo:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
outis
fonte
2

Estou de acordo com frozenkoi, no entanto, para C # de qualquer maneira, pensei que ajudaria a seguir a sintaxe dos métodos TryParse.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();
bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
Biff MaGriff
fonte
1

Seu código é feio porque você está fazendo muito em uma única função. Você deseja processar o arquivo ou executar a ação padrão. Comece dizendo que:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Programadores Perl e Ruby escrevem processFile(file) || defaultAction()

Agora vá escrever ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
kevin cline
fonte
1

Claro que você só pode ir tão longe em cenários como estes, mas aqui está um caminho a percorrer:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Você pode querer filtros adicionais. Então faça o seguinte:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Embora isso possa fazer sentido também:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

Qual é o melhor? Isso depende do problema do mundo real que você está enfrentando.
Mas o que se deve tirar é: você pode fazer muito com composição e polimorfismo.

back2dos
fonte
1

O que há de errado com o óbvio

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

Parece-me bastante normal? Para esse tipo de procedimento grande, onde muitas pequenas coisas precisam acontecer, a falha de qualquer uma delas impediria a segunda. As exceções o tornam um pouco mais limpo, se for uma opção.

Steve Bennett
fonte
0

Percebo que essa é uma pergunta antiga, mas notei um padrão que não foi mencionado; principalmente, definir uma variável para determinar posteriormente o (s) método (s) que você gostaria de chamar (fora do if ... else ...).

Esse é outro ângulo para facilitar o trabalho do código. Também permite quando você pode adicionar outro método a ser chamado ou alterar o método apropriado que precisa ser chamado em determinadas situações.

Em vez de precisar substituir todas as menções do método (e talvez perder alguns cenários), todas elas são listadas no final do bloco if ... else ... e são mais simples de ler e alterar. Costumo usar isso quando, por exemplo, vários métodos podem ser chamados, mas dentro do aninhado se ... else ... um método pode ser chamado em várias correspondências.

Se você definir uma variável que define o estado, poderá ter muitas opções profundamente aninhadas e atualizar o estado quando algo deve ou não ser executado.

Isso pode ser usado como no exemplo da pergunta em que estamos verificando se 'DoSomething' ocorreu e, se não, execute a ação padrão. Ou você pode ter um estado para cada método que você deseja chamar, definir quando aplicável e chamar o método aplicável fora do if ... else ...

No final das instruções aninhadas if ... else ..., você verifica o estado e age de acordo. Isso significa que você precisa apenas de uma única menção de um método em vez de todos os locais em que ele deve ser aplicado.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
Steve Padmore
fonte
0

Para reduzir o IF aninhado:

1 / retorno antecipado;

2 / expressão composta (atento a curto-circuito)

Portanto, seu exemplo pode ser refatorado assim:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
DQ_vn
fonte
0

Eu vi muitos exemplos com "return", que também uso, mas às vezes quero evitar a criação de novas funções e usar um loop:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Se você deseja escrever menos linhas ou odeia loops infinitos como eu, pode alterar o tipo de loop para "do ... while (0)" e evitar a última "interrupção".

XzKto
fonte
0

Que tal esta solução:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

Eu assumi que o OpenFile retorna um ponteiro, mas isso também pode funcionar com o retorno do tipo de valor, especificando algum valor padrão não retornável (códigos de erro ou algo parecido).

É claro que não espero alguma ação possível por meio do método SomeTest no ponteiro NULL (mas você nunca sabe), portanto isso também pode ser visto como uma verificação extra do ponteiro NULL para a chamada SomeTest (conteúdo).

chedi
fonte
0

Claramente, a solução mais elegante e concisa é usar uma macro de pré-processador.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

O que permite escrever um código bonito como este:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

Pode ser difícil confiar na formatação automática se você usar essa técnica com frequência, e alguns IDEs podem gritar um pouco com você sobre o que ela supõe erroneamente estar malformado. E como diz o ditado, tudo é uma troca, mas suponho que não seja um preço ruim a pagar para evitar os males do código repetido.

Peter Olson
fonte
Para algumas pessoas, e em algumas línguas, macros de pré-processamento são :) código mal
Benjol
@ Benjol Você disse que estava aberto a sugestões ruins, não? ;)
Peter Olson
Sim, com certeza, era apenas wrt seu "evitar os males" :)
Benjol
4
Isso é tão horrível, eu só tinha que upvote-lo: D
back2dos
Shirley, você não está falando sério !!!!!!
Jim No Texas
-1

Como você perguntou por curiosidade e sua pergunta não está marcada com um idioma específico (mesmo que esteja claro que você tinha idiomas imperativos em mente), vale a pena acrescentar que os idiomas que suportam a avaliação lenta permitem uma abordagem completamente diferente. Nessas linguagens, as expressões são avaliadas apenas quando necessário, para que você possa definir "variáveis" e usá-las somente quando fizer sentido. Por exemplo, em uma linguagem de ficção com preguiçosos let/ inestruturas de você esquecer controle de fluxo e escreve:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()
tokland
fonte