Este uso de condicionais é um antipadrão?

14

Eu já vi isso muito em nosso sistema legado no trabalho - funções que são mais ou menos assim:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

Em outras palavras, a função tem duas partes. A primeira parte faz algum tipo de processamento (potencialmente contendo loops, efeitos colaterais, etc.) e, ao longo do caminho, pode definir o sinalizador "todo". A segunda parte só é executada se o sinalizador "todo" tiver sido definido.

Parece uma maneira bem feia de fazer as coisas, e acho que a maioria dos casos que eu realmente tive tempo de entender, poderia ser refatorada para evitar o uso da bandeira. Mas isso é um anti-padrão real, uma má ideia ou perfeitamente aceitável?

A primeira refactorização óbvia seria cortá-la em dois métodos. No entanto, minha pergunta é mais sobre se há sempre uma necessidade (em uma linguagem OO moderna) de criar uma variável de sinalizador local, configurando-a potencialmente em vários locais e depois usá-la posteriormente para decidir se deve executar o próximo bloco de código. / p>     

por Kricket 11.10.2011 / 10:42
fonte

12 respostas

23

Eu não sei sobre antipadrão, mas eu extraí três métodos disso.

O primeiro executaria algum trabalho e retornaria um valor booleano.

O segundo executaria qualquer trabalho realizado por "algum outro código"

O terceiro executaria o trabalho auxiliar se o retorno booleano fosse verdadeiro.

Os métodos extraídos provavelmente seriam privados se fosse importante que o segundo apenas (e sempre) fosse chamado se o primeiro método retornasse verdadeiro.

Por nomear bem os métodos, espero que isso torne o código mais claro.

Algo parecido com isto:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Obviamente, há um debate sobre se os retornos iniciais são aceitáveis, mas isso é um detalhe de implementação (como é o padrão de formatação de código).

O ponto é que a intenção do código se torna mais clara, o que é bom ...

Um dos comentários sobre a questão sugere que esse padrão representa um cheiro , e eu concordaria com isso. Vale a pena olhar para ver se você pode tornar a intenção mais clara.

    
por 11.10.2011 / 10:48
fonte
6

Eu acho que a fealdade é devida ao fato de que há muito código em um único método, e / ou variáveis são mal nomeadas (ambas são código cheira por direito próprio - antipadrões são coisas mais abstratas e complexas IMO).

Então, se você extrair a maior parte do código em métodos de nível mais baixo, como sugere o @Bill, o resto ficará limpo (para mim, pelo menos). Por exemplo,

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Ou você pode até mesmo se livrar da bandeira local completamente, escondendo a verificação da bandeira no segundo método e usando um formulário como

calculateTaxRefund(isTaxRefundable(...), ...)

No geral, não vejo uma variável de sinalizador local como necessariamente ruim em si. Qual opção do acima é mais legível (= preferível para mim) depende do número de parâmetros do método, os nomes escolhidos e qual formulário é mais consistente com a lógica interna do código.

    
por 11.10.2011 / 11:19
fonte
4

todo é um nome muito ruim para a variável, mas acho que isso pode ser tudo errado. É difícil ter certeza absoluta sem o contexto.

Digamos que a segunda parte da função classifica uma lista, construída pela primeira parte. Isso deve ser muito mais legível:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

No entanto, a sugestão de Bill também está correta. Isso ainda é mais legível:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);
    
por 11.10.2011 / 11:18
fonte
2

O padrão da máquina de estado parece bem para mim. Os anti-padrões lá são "todo" (nome ruim) e "muitos códigos".

    
por 11.10.2011 / 20:00
fonte
1

Depende mesmo. Se o código guardado por todo (eu espero que você não esteja usando esse nome de verdade como é totalmente anti-mnemônico!) É conceitualmente código de limpeza, então você tem um anti-padrão e deve usar algo como C ++ 's A construção de using do RAII ou do C # para lidar com as coisas.

Por outro lado, se não for conceitualmente não um estágio de limpeza, mas sim apenas processamento adicional que às vezes é necessário e onde a decisão de fazê-lo precisa ser tomada anteriormente, o que está escrito está correto . Considere se trechos de código individuais seriam melhor refatorados em suas próprias funções, é claro, e também se você capturou o significado da variável de sinalizador em seu nome, mas esse padrão de código básico é OK. Em particular, tentar colocar muito em outras funções pode tornar o que está acontecendo menos claro, e isso definitivamente seria um anti-padrão.

    
por 11.10.2011 / 11:23
fonte
1

Muitas das respostas aqui teriam problemas em passar por uma verificação de complexidade, algumas pareciam > 10.

Eu acho que essa é a parte "anti-padrão" do que você está vendo. Encontre uma ferramenta que mede a complexidade ciclomática do seu código - existem plugins para o eclipse. É essencialmente uma medida de quão difícil é testar seu código e envolve o número e os níveis de ramificações de código.

Como um palpite total de uma possível solução, o layout do seu código me faz pensar em "Tarefas", se isso acontecer em muitos lugares, talvez o que você realmente queira seja uma arquitetura orientada a tarefas - cada tarefa Sendo o seu próprio objeto e no meio da tarefa você tem a capacidade de enfileirar a próxima tarefa, instanciando outro objeto de tarefa e jogando-o na fila. Estes podem ser incrivelmente simples de configurar e reduzem significativamente a complexidade de certos tipos de código - mas, como eu disse, isso é uma tentativa total no escuro.

    
por 11.10.2011 / 17:43
fonte
1

Usando o exemplo do pdr acima, como é um bom exemplo, vou dar mais um passo.

Ele tinha:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Então percebi que o seguinte funcionaria:

if(BuildList(list)) 
    SortList(list)

Mas não é tão claro.

Então, para a pergunta original, por que não:

BuildList(list)
SortList(list)

E deixar SortList decidir se requer classificação?

Você vê que seu método BuildList parece saber sobre ordenação, já que ele retorna um bool indicando como tal, mas isso não faz sentido para um método projetado para construir uma lista.

    
por 11.10.2011 / 13:25
fonte
0

Sim, isso parece ser um problema porque você precisa acompanhar todos os lugares em que você está marcando o sinalizador LIGAR / DESLIGAR. É melhor incluir a lógica dentro de uma condição aninhada em vez de retirar a lógica.

Também modelos de domínio ricos, nesse caso, apenas um forro fará grandes coisas dentro do objeto.

    
por 11.10.2011 / 15:45
fonte
0

Se o sinalizador for definido apenas uma vez, mova o sinal ...
código até diretamente após o
... // algum outro código aqui
então acabe com a bandeira.

Caso contrário, divida o
  ... // lotes de código aqui
... // algum outro código aqui
...
codificar em funções separadas, se possível, por isso, é claro que esta função tem uma responsabilidade que é a lógica de ramo.

Sempre que possível, separe o código dentro de você   ... // lotes de código aqui
em duas ou mais funções, algumas que fazem algum trabalho (que é um comando) e outras que retornam o valor todo (o que é uma consulta) ou o tornam muito explícito que estão modificando-o (uma consulta usando efeitos colaterais)

O código em si não é o antipadrão que está acontecendo aqui ... Eu suspeito que misturar a lógica de ramificação com o fazer real de coisas (comandos) é o antipadrão que você está procurando.

    
por 08.06.2018 / 17:21
fonte
0

Às vezes, acho que preciso implementar esse padrão. Às vezes, você deseja executar várias verificações antes de prosseguir com uma operação. Por razões de eficiência, os cálculos que envolvem certas verificações não são feitos, a menos que pareça absolutamente necessário verificar. Então você normalmente vê código como:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Isso poderia ser simplificado separando a validação do processo real de execução da operação. Assim, você veria mais como:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Obviamente, isso varia de acordo com o que você está tentando alcançar, embora escrito assim, tanto seu código de "sucesso" como seu código de "falha" são escritos uma vez, o que simplifica a lógica e mantém o mesmo nível de desempenho. A partir daí, seria uma boa idéia ajustar níveis inteiros de validação dentro de métodos internos que retornam indicações booleanas de sucesso ou falha que simplificam ainda mais as coisas, embora alguns programadores gostem de métodos extremamente longos por algum motivo estranho.

    
por 11.10.2011 / 14:39
fonte
0

Este não é um padrão . A interpretação mais geral é que você está definindo uma variável booleana e ramificando em seu valor mais tarde. Isso é programação procedural normal, nada mais.

Agora, seu exemplo específico pode ser reescrito como:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

Isso pode ser mais fácil de ler. Ou talvez não. Depende do resto do código que você omitiu. Concentre-se em tornar esse código mais sucinto.

    
por 13.06.2018 / 21:11
fonte
-1

Os sinalizadores locais usados para o fluxo de controle devem ser reconhecidos como uma forma de goto disfarçado. Se um flag é usado apenas dentro de uma função, ele pode ser eliminado escrevendo duas cópias da função, rotulando uma como "flag is true" e a outra como "flag is false", e tendo substituindo todas as operações que configuram o flag quando está claro, ou limpa quando está definido, com um salto entre as duas versões da função.

Em muitos casos, o código que usa um sinalizador será mais limpo do que qualquer alternativa possível que use goto , mas isso nem sempre é verdade. Em alguns casos, usar goto para pular um pedaço de código pode ser mais limpo do que usar sinalizadores para fazer isso [apesar de algumas pessoas poderem inserir um certo desenho animado de raptor aqui].

Acho que o principal princípio orientador deve ser que o fluxo lógico do programa deve se assemelhar à descrição da lógica de negócios na medida do possível. Se os requisitos de lógica de negócios forem descritos em termos de estados que se dividem e se mesclam de maneiras estranhas, fazer a lógica do programa do mesmo modo pode ser mais limpo do que tentar usar sinalizadores para ocultar essa lógica. Por outro lado, se a maneira mais natural de descrever regras de negócios seria dizer que uma ação deveria ser feita se certas ações fossem realizadas, a maneira mais natural de expressar isso seria usar uma bandeira que é definida ao executar as últimas ações e é claro.

    
por 08.06.2018 / 20:19
fonte