Esse uso de condicionais é um antipadrão?

14

Eu já vi isso muito em nosso sistema legado em funcionamento - 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 possui duas partes. A primeira parte realiza algum tipo de processamento (potencialmente contendo loops, efeitos colaterais etc.) e, ao longo do caminho, pode definir o sinalizador "todo". A segunda parte é executada apenas se o sinalizador "todo" tiver sido definido.

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

A primeira refatoração óbvia seria dividi-la em dois métodos. No entanto, minha pergunta é mais sobre se existe a necessidade (em uma linguagem OO moderna) de criar uma variável de flag local, potencialmente configurando-a em vários locais e, em seguida, usá-la posteriormente para decidir se deve executar o próximo bloco de código.

Kricket
fonte
2
Como você o refatora?
Tamás Szelei 11/11
13
Supondo que todo é definido em vários lugares, de acordo com várias condições não triviais e não exclusivas, mal consigo pensar em uma refatoração que faça o menor sentido. Se não houver refatoração, não haverá antipadrão. Exceto o nome da variável todo; deve ser nomeado mais expressivo, como "doSecurityCheck".
user281377
3
@ammoQ: +1; se as coisas são complicadas, é assim que elas são. Uma variável de flag pode fazer muito mais sentido em algumas circunstâncias, pois torna mais claro que uma decisão foi tomada e você pode procurá-la para descobrir onde essa decisão foi tomada.
Donal Fellows
1
@Donal Fellows: Se for necessário procurar o motivo, eu faria da variável uma lista; contanto que esteja vazio, é "falso"; onde quer que o sinalizador esteja definido, um código de razão é adicionado à lista. Portanto, você pode terminar com uma lista como ["blacklisted-domain","suspicious-characters","too-long"]essa que mostra que várias razões foram aplicadas.
user281377
2
Eu não acho que é um anti-padrão, mas é definitivamente um cheiro
Binary Worrier

Respostas:

23

Eu não sei sobre anti-padrão, mas extrairia três métodos disso.

O primeiro executaria algum trabalho e retornaria um valor booleano.

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

O terceiro executaria o trabalho auxiliar se o booleano retornado 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.

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

Algo assim:

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 esse é um detalhe da 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 concordo com isso. Vale a pena olhar para ver se você pode tornar a intenção mais clara.

Bill Michell
fonte
A divisão em duas funções ainda exigiria uma todovariável e provavelmente seria mais difícil de entender.
Pubby
Sim, eu faria isso também, mas minha pergunta era mais sobre o uso da bandeira "todo".
21811 Kricket #
2
Se você terminar if (check_if_needed ()) do_whatever ();, não há bandeira óbvia lá. Eu acho que isso pode quebrar muito o código e potencialmente prejudicar a legibilidade se o código for razoavelmente simples. Afinal, os detalhes do que você faz do_whateverpodem afetar a forma como você testa check_if_needed, de modo que é útil manter todo o código junto na mesma tela. Além disso, isso não garante que check_if_neededpossa evitar o uso de um sinalizador - e, se o fizer, provavelmente usará várias returninstruções para fazê-lo, possivelmente perturbando os advogados estritos de saída única.
Steve314
3
@ Pubby8 ele disse "extraia 2 métodos disso" , resultando em 3 métodos. 2 métodos para o processamento real e o método original que coordena o fluxo de trabalho. Este seria um design muito mais limpo.
MattDavey
Este omite o ... // some other code hereno caso regresso antecipado
Caleth
6

Eu acho que a feiúra se deve ao fato de haver muito código em um único método, e / ou variáveis ​​serem mal nomeadas (ambas com cheiro de código por direito próprio - antipadrões são coisas mais abstratas e complexas da IMO).

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

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

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

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

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

Péter Török
fonte
4

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

Digamos que a segunda parte da função classifique uma lista, criada 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. Ainda é mais legível:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);
pdr
fonte
Por que não dar um passo adiante: if (BuildList (list)) SortList (list);
Phil N DeBlanc
2

O padrão da máquina de estado parece bom para mim. Os anti padrões lá são "todo" (nome incorreto) e "muito código".

ptyx
fonte
Tenho certeza de que é apenas para ilustração, no entanto.
Loren Pechtel 11/11
1
Acordado. O que eu estava tentando transmitir é que bons padrões afogados em código ruim não devem ser responsabilizados pela qualidade do código.
Ptyx
1

Depende mesmo. Se o código guardado por todo(espero que você não esteja usando esse nome de verdade, pois é totalmente não-mnemônico!) É um código de limpeza conceitual, então você tem um antipadrão e deve usar algo como RAII ou C # de C ++ usingconstruir para lidar com as coisas em seu lugar.

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

Donal Fellows
fonte
Claramente, não é uma limpeza - nem sempre é executado. Já encontrei casos como esse antes - ao processar algo, você pode acabar invalidando algum tipo de resultado pré-calculado. Se o cálculo for caro, você só deseja executá-lo, se necessário.
Loren Pechtel 11/11
1

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

Eu acho que essa é a parte "antipadrã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 seu código é testar e envolve o número e os níveis de ramificações de código.

Como um palpite total sobre 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 sua 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. Eles podem ser incrivelmente simples de configurar e reduzem significativamente a complexidade de certos tipos de código - mas como eu disse, essa é uma facada total no escuro.

Bill K
fonte
1

Usando o exemplo de pdr acima, como é um bom exemplo, darei um passo adiante.

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 ter:

BuildList(list)
SortList(list)

E deixe SortList decidir se requer classificação?

Você vê que o método BuildList parece saber sobre classificação, pois retorna um bool indicando como tal, mas isso não faz sentido para um método projetado para criar uma lista.

Ian
fonte
E, claro, o próximo passo é perguntar por que esse é um processo de duas etapas. Em qualquer lugar que eu vejo código como que eu refatorar a um método chamado BuildAndSortList (lista)
Ian
Esta não é uma resposta. Você mudou o comportamento do código.
D Drmmr
Na verdade não. Novamente, não acredito que estou respondendo a algo que publiquei há 7 anos, mas que diabos :) O que eu estava argumentando é que o SortList conteria o condicional. Se você tivesse um teste de unidade que afirmasse que a lista só seria classificada se a condição x fosse atendida, ela ainda passaria. Ao mover o condicional em sortlist, você evita ter que sempre write (if (algo), então sortlist (...))
Ian
0

Sim, isso parece ser um problema, porque você precisa continuar acompanhando todos os lugares em que está marcando a bandeira como LIGADA / DESLIGADA. É melhor incluir a lógica apenas dentro como uma condição aninhada, em vez de retirar a lógica.

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

java_mouse
fonte
0

Se o sinalizador for definido apenas uma vez, mova o código
...
para imediatamente após o
... // algum outro código aqui
e elimine o sinalizador.

Caso contrário, divida o
... // muitos códigos aqui
... // algum outro código aqui
...
codifique em funções separadas, se possível, portanto é claro que essa função tem uma responsabilidade que é a lógica da ramificação.

Sempre que possível, separe o código dentro de
... // muitos códigos aqui
descritos em duas ou mais funções, algumas que funcionam (que é um comando) e outras que retornam o valor todo (que é uma consulta) ou o fazem muito explícito, eles a estão modificando (uma consulta usando efeitos colaterais)

O código em si não é o anti-padrão que está acontecendo aqui ... Suspeito que a lógica de ramificação combinada com a execução real de coisas (comandos) seja o anti-padrão que você está procurando.

andrew pate
fonte
o que este post acrescenta que faltam respostas existentes?
esoterik
@esoterik Às vezes, a oportunidade de adicionar um pouco de CQRS costuma ser negligenciada quando se trata de sinalizadores ... a lógica para decidir alterar um sinalizador representa uma consulta, enquanto que o trabalho representa um comando. Às vezes, separar os dois pode tornar o código mais claro. Também vale a pena apontar no código acima que ele pode ser simplificado porque o sinalizador é definido apenas em um ramo. Sinto que as bandeiras não são um antipadrão e, se o nome delas realmente torna o código mais expressivo, elas são boas. Eu sinto que onde os sinalizadores são criados, definidos e usados ​​devem estar próximos no código, se possível.
andrew pate
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 determinadas verificações não são feitos, a menos que pareça absolutamente necessário. Então você normalmente vê códigos 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 pode ser simplificado separando a validação do processo real de execução da operação, para que você veja 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, varia de acordo com o que você está tentando alcançar, embora seja escrito assim, tanto o código de "sucesso" quanto o 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 ideia ajustar níveis inteiros de validação em 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.

Neil
fonte
No exemplo que você deu, acho que preferiria ter uma função shouldIDoIt (fieldsValidated, valuesExist) que retorne a resposta. Isso ocorre porque a determinação de sim / não é feita de uma só vez, em contraste com o código que vejo aqui no trabalho, onde a decisão de prosseguir é espalhada em alguns pontos diferentes e não contíguos.
Kricket
@ KelseyRider, esse era precisamente o ponto. A separação da validação da execução permite incluir a lógica em um método, a fim de simplificar a lógica geral do programa em if (isValidated ()) doOperation ()
Neil
0

Este não é um padrão . A interpretação mais geral é que você está definindo uma variável booleana e ramificando seu valor posteriormente. Isso é programação processual 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 restante do código que você omitiu. Concentre-se em tornar esse código mais sucinto.

D Drmmr
fonte
-1

Os sinalizadores locais usados ​​para o fluxo de controle devem ser reconhecidos como uma forma de gotodisfarce. Se um sinalizador for usado apenas dentro de uma função, ele poderá ser eliminado gravando duas cópias da função, rotulando um como "sinalizador é verdadeiro" e o outro como "sinalizador é falso" e substituindo todas as operações que configuram o sinalizador quando está claro, ou apaga quando está definido, com um salto entre as duas versões da função.

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

Penso que o principal princípio orientador deve ser que o fluxo lógico do programa se assemelhe à descrição da lógica de negócios na medida do possível. Se os requisitos da lógica de negócios forem descritos em termos de estados que se dividem e se fundem de maneiras estranhas, ter a lógica do programa da mesma forma 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 for dizer que uma ação deve ser executada se outras ações tiverem sido executadas, a maneira mais natural de expressar isso pode ser usar um sinalizador que é definido ao executar as últimas ações e é claro.

supercat
fonte