É errado usar um parâmetro booleano para determinar o comportamento?

De tempos em tempos, tenho visto uma prática que "parece" errada, mas não consigo articular completamente o que há de errado nisso. Ou talvez seja apenas o meu preconceito. Aqui vai:

Um desenvolvedor define um método com um booleano como um de seus parâmetros, e esse método chama outro, e assim por diante, e eventualmente esse booleano é usado, apenas para determinar se uma ação deve ou não ser executada. Isso pode ser usado, por exemplo, para permitir a ação somente se o usuário tiver certos direitos, ou talvez se estamos (ou não) no modo de teste ou no lote ou no modo ao vivo, ou talvez apenas quando o sistema estiver em um certo estado.

Bem, sempre há outra maneira de fazer isso, seja perguntando quando é hora de executar a ação (em vez de passar o parâmetro), ou tendo várias versões do método, ou várias implementações da classe, etc. Minha pergunta é: é muito melhor como melhorar isso, mas sim se realmente está errado (como suspeito) e, se estiver, o que há de errado nisso.

Sim, é provável que haja um cheiro de código, o que levaria a um código não sustentável, difícil de entender e com menor chance de ser facilmente reutilizado.

Como outros pôsteres observaram que o contexto é tudo (não entre em jogo pesado, se for um caso único ou se a prática tiver sido reconhecida como uma dívida técnica deliberadamente incorrida para ser re-fatorada posteriormente), mas de um modo geral, se houver um parâmetro passado em uma função que seleciona um comportamento específico a ser executado, é necessário um refinamento gradual adicional; A divisão dessa função em funções menores produzirá funções mais coesas.

Então, o que é uma função altamente coesa?

É uma função que faz uma coisa e apenas uma coisa.

O problema com um parâmetro passado conforme você descreve é ​​que a função está fazendo mais de duas coisas; ele pode ou não verificar os direitos de acesso dos usuários, dependendo do estado do parâmetro booleano; em seguida, dependendo dessa árvore de decisão, ele executará uma parte da funcionalidade.

Seria melhor separar as preocupações do controle de acesso das preocupações da tarefa, ação ou comando.

Como você já observou, o entrelaçamento dessas preocupações parece errado.

Portanto, a noção de coesão nos ajuda a identificar que a função em questão não é altamente coesa e que podemos refatorar o código para produzir um conjunto de funções mais coesas.

Então a questão poderia ser reafirmada; Dado que todos concordamos que a passagem de parâmetros de seleção comportamental é melhor evitar, como melhoramos as coisas?

Eu me livraria completamente do parâmetro. Ter a capacidade de desativar o controle de acesso, mesmo para testes, é um risco potencial à segurança. Para fins de teste, stub ou mock a verificação de acesso para testar os cenários de acesso permitido e de acesso negado.

Ref: Coesão (ciência da computação)

Parei de usar esse padrão há muito tempo, por uma razão muito simples; custo de manutenção. Várias vezes, descobri que tinha alguma função, frobnicate(something, forwards_flag)que era chamada muitas vezes no meu código, e precisava localizar todos os locais no código em que o valor falsefoi passado como o valor de forwards_flag. Você não pode procurá-los facilmente, então isso se torna uma dor de cabeça na manutenção. E se você precisar corrigir um bug em cada um desses sites, poderá ter um problema infeliz se perder um.

Mas esse problema específico é facilmente corrigido sem alterar fundamentalmente a abordagem:

enum FrobnicationDirection {
  FrobnicateForwards,
  FrobnicateBackwards;
};

void frobnicate(Object what, FrobnicationDirection direction);

Com esse código, seria necessário procurar apenas instâncias de FrobnicateBackwards. Embora seja possível que exista algum código que atribua isso a uma variável, para que você tenha que seguir alguns segmentos de controle, na prática, acho que isso é raro o suficiente para que essa alternativa funcione bem.

No entanto, há outro problema em passar a bandeira dessa maneira, pelo menos em princípio. Isso é que alguns (apenas alguns) sistemas com esse design podem estar expondo muito conhecimento sobre os detalhes de implementação das partes profundamente aninhadas do código (que usa o sinalizador) para as camadas externas (que precisam saber qual valor passar) nesta bandeira). Para usar a terminologia de Larry Constantine, esse design pode ter um acoplamento muito forte entre o levantador e o usuário do sinalizador booleano. Franky, embora seja difícil dizer com algum grau de certeza sobre essa questão, sem saber mais sobre a base de código.

Para abordar os exemplos específicos que você fornece, eu teria algum grau de preocupação em cada um, mas principalmente por razões de risco / correção. Ou seja, se seu sistema precisa passar por sinalizadores que indicam em que estado o sistema está, você pode achar que possui um código que deveria ter levado isso em conta, mas não verifica o parâmetro (porque não foi passado para esta função). Então você tem um bug porque alguém omitiu passar o parâmetro.

Também vale a pena admitir que um indicador de estado do sistema que precisa ser passado para quase todas as funções é, de fato, essencialmente uma variável global. Muitas das desvantagens de uma variável global serão aplicadas. Penso que, em muitos casos, é uma prática recomendada encapsular o conhecimento do estado do sistema (ou as credenciais do usuário ou a identidade do sistema) dentro de um objeto responsável por agir corretamente com base nesses dados. Em seguida, você passa uma referência a esse objeto em oposição aos dados brutos. O conceito-chave aqui é encapsulamento .

Isso não é necessariamente errado, mas pode representar um cheiro de código .

O cenário básico que deve ser evitado em relação aos parâmetros booleanos é:

public void foo(boolean flag) {
    doThis();
    if (flag)
        doThat();
}

Em seguida, ao ligar, você normalmente liga foo(false)e foo(true)depende do comportamento exato que deseja.

Isso é realmente um problema, porque é um caso de má coesão. Você está criando uma dependência entre métodos que não é realmente necessário.

O que você deve fazer neste caso é sair doThise, doThatcomo métodos públicos e separados, fazer:

doThis();
doThat();

ou

doThis();

Dessa forma, você deixa a decisão correta para o chamador (exatamente como se estivesse passando um parâmetro booleano) sem criar acoplamento.

É claro que nem todos os parâmetros booleanos são usados ​​de maneira tão ruim, mas é definitivamente um cheiro de código e você tem razão em suspeitar se vê muito disso no código-fonte.

Este é apenas um exemplo de como resolver esse problema com base nos exemplos que escrevi. Existem outros casos em que uma abordagem diferente será necessária.

Há um bom artigo de Martin Fowler explicando em mais detalhes a mesma idéia.

PS: se o método, em foovez de chamar dois métodos simples, tivesse uma implementação mais complexa, basta aplicar um pequeno método de extração de refatoração , para que o código resultante seja semelhante à implementação fooque escrevi.

Primeiro: a programação não é tanto uma ciência, mas uma arte. Portanto, raramente existe uma maneira "errada" e "certa" de programar. A maioria dos padrões de codificação são meramente "preferências" que alguns programadores consideram úteis; mas, em última análise, eles são bastante arbitrários. Portanto, eu nunca rotularia uma opção de parâmetro como "errada" por si só - e certamente não é algo tão genérico e útil quanto um parâmetro booleano. O uso de um boolean(ou um int, para esse assunto) para encapsular o estado é perfeitamente justificável em muitos casos.

As decisões de codificação, em geral, devem se basear principalmente no desempenho e na capacidade de manutenção. Se o desempenho não estiver em jogo (e não consigo imaginar como poderia ser em seus exemplos), sua próxima consideração deve ser: quão fácil será para mim (ou um redator futuro) manter? É intuitivo e compreensível? Está isolado? Seu exemplo de chamadas de função em cadeia de fato parece potencialmente frágil a esse respeito: se você decidir mudar bIsUppara bIsDown, quantos outros lugares no código precisarão ser alterados também? Além disso, sua lista de paramater está aumentando? Se sua função possui 17 parâmetros, a legibilidade é um problema e você precisa reconsiderar se está apreciando os benefícios da arquitetura orientada a objetos.

Acho que o artigo de código limpo de Robert C Martins afirma que você deve eliminar argumentos booleanos sempre que possível, pois eles mostram que um método faz mais de uma coisa. Um método deve fazer uma coisa e apenas uma coisa que eu acho que é um de seus lemas.

Eu acho que a coisa mais importante aqui é ser prático.

Quando o booleano determinar todo o comportamento, basta criar um segundo método.

Quando o booleano determina apenas um pouco de comportamento no meio, convém mantê-lo em um para reduzir a duplicação de código. Onde possível, você pode até dividir o método em três: Dois métodos de chamada para qualquer opção booleana e um que faz a maior parte do trabalho.

Por exemplo:

private void FooInternal(bool flag)
{
  //do work
}

public void Foo1()
{
  FooInternal(true);
}

public void Foo2()
{
  FooInternal(false);
}

Obviamente, na prática, você sempre terá um ponto entre esses extremos. Normalmente, eu apenas uso o que parece certo, mas prefiro errar com menos duplicação de código.

Eu gosto da abordagem de personalizar o comportamento por meio de métodos do construtor que retornam instâncias imutáveis. Veja como o Guava oSplitter usa:

private static final Splitter MY_SPLITTER = Splitter.on(',')
       .trimResults()
       .omitEmptyStrings();

MY_SPLITTER.split("one,,,,  two,three");

Os benefícios disso são:

• Legibilidade superior
• Separação clara da configuração versus métodos de ação
• Promove a coesão, forçando você a pensar sobre o que é o objeto, o que deve ou não fazer. Nesse caso, é um Splitter. Você nunca colocaria someVaguelyStringRelatedOperation(List<Entity> myEntities)uma classe chamada Splitter, mas pensaria em colocá-la como um método estático em uma StringUtilsclasse.
• As instâncias são pré-configuradas, portanto, prontamente injetáveis ​​em dependência. O cliente não precisa se preocupar em passar trueou falseem um método para obter o comportamento correto.

Definitivamente um cheiro de código . Se não viola o Princípio de Responsabilidade Única , provavelmente viola Tell, Don't Ask . Considerar:

• Você pode resumir o que o seu método faz sem usar conjunções (e, mas etc.)?
• Existem partes do seu método que poderiam ser facilmente agrupadas em outro método menor ? (Isso inclui a criação complexa de uma variável local .)
• Se seu sinalizador determina qual método chamar em um parâmetro, ele está fazendo muito ?
• Esse método pertence a um dos seus parâmetros? (Você pode perceber se ele separa variáveis ​​de instância em seu parâmetro usando getters / setters .)

Se não violar um desses dois princípios, você ainda deve usar uma enumeração. Bandeiras booleanas são o equivalente booleano de números mágicos . foo(false)faz tanto sentido quanto bar(42). As enums podem ser úteis para o Padrão de Estratégia e têm a flexibilidade de permitir que você adicione outra estratégia. (Lembre-se de nomeá-los adequadamente .)

Seu exemplo em particular me incomoda. Por que esse sinalizador é passado por tantos métodos? Parece que você precisa dividir seu parâmetro em subclasses .

TL; DR: não use argumentos booleanos.

Veja abaixo por que eles são ruins e como substituí-los (em negrito).

Argumentos booleanos são muito difíceis de ler e, portanto, difíceis de manter. O principal problema é que o objetivo geralmente é claro quando você lê a assinatura do método em que o argumento é nomeado. No entanto, nomear um parâmetro geralmente não é necessário na maioria dos idiomas. Portanto, você terá antipadrões como RSACryptoServiceProvider#encrypt(Byte[], Boolean)onde o parâmetro booleano determina que tipo de criptografia deve ser usado na função.

Então você receberá uma ligação como:

rsaProvider.encrypt(data, true);

onde o leitor precisa procurar a assinatura do método simplesmente para determinar o que diabos truepode realmente significar. Passar um número inteiro é obviamente tão ruim quanto:

rsaProvider.encrypt(data, 1);

diria o mesmo - ou melhor: apenas o mínimo. Mesmo se você definir constantes a serem usadas para o número inteiro, os usuários da função poderão simplesmente ignorá-las e continuar usando valores literais.

A melhor maneira de resolver isso é usar uma enumeração . Se você precisar passar uma enumeração RSAPaddingcom dois valores: OAEPou PKCS1_V1_5poderá ler imediatamente o código:

rsaProvider.encrypt(data, RSAPadding.OAEP);

Booleanos podem ter apenas dois valores. Isso significa que, se você tiver uma terceira opção, precisará refatorar sua assinatura. Geralmente, isso não pode ser realizado facilmente se a compatibilidade com versões anteriores for um problema; portanto, você precisará estender qualquer classe pública com outro método público. Foi o que a Microsoft finalmente fez quando eles introduziram RSACryptoServiceProvider#encrypt(Byte[], RSAEncryptionPadding)onde usavam uma enumeração (ou pelo menos uma classe imitando uma enumeração) em vez de um booleano.

Pode até ser mais fácil usar um objeto ou interface completo como parâmetro, caso o próprio parâmetro precise ser parametrizado. No exemplo acima, o preenchimento OAEP em si pode ser parametrizado com o valor de hash para uso interno. Observe que agora existem 6 algoritmos de hash SHA-2 e 4 algoritmos de hash SHA-3; portanto, o número de valores de enumeração pode explodir se você usar apenas uma única enumeração em vez de parâmetros (essa é possivelmente a próxima coisa que a Microsoft descobrirá )

Parâmetros booleanos também podem indicar que o método ou classe não foi projetado bem. Como no exemplo acima: qualquer biblioteca criptográfica que não seja a .NET não utiliza um sinalizador de preenchimento na assinatura do método.

Quase todos os gurus de software que eu gosto alertam contra argumentos booleanos. Por exemplo, Joshua Bloch adverte contra eles no livro muito apreciado "Java Efetivo". Em geral, eles simplesmente não devem ser usados. Você poderia argumentar que eles poderiam ser usados ​​se houver um parâmetro fácil de entender. Mas mesmo assim: Bit.set(boolean)provavelmente é melhor implementado usando dois métodos : Bit.set()e Bit.unset().

Se você não puder refatorar diretamente o código, poderá definir constantes para, pelo menos, torná-las mais legíveis:

const boolean ENCRYPT = true;
const boolean DECRYPT = false;

...

cipher.init(key, ENCRYPT);

é muito mais legível do que:

cipher.init(key, true);

mesmo se você preferir:

cipher.initForEncryption(key);
cipher.initForDecryption(key);

em vez de.

Estou surpreso que ninguém tenha mencionado parâmetros nomeados .

O problema que vejo com sinalizadores booleanos é que eles prejudicam a legibilidade. Por exemplo, o que faz trueem

myObject.UpdateTimestamps(true);

Faz? Eu não faço ideia. Mas e quanto a:

myObject.UpdateTimestamps(saveChanges: true);

Agora está claro o que o parâmetro que estamos passando deve fazer: estamos dizendo à função para salvar suas alterações. Nesse caso, se a classe não for pública, acho que o parâmetro booleano está correto.

Obviamente, você não pode forçar os usuários da sua classe a usar parâmetros nomeados. Por esse motivo, enumgeralmente é preferível um ou dois métodos separados, dependendo de quão comum é o seu caso padrão. É exatamente isso que o .Net faz:

//Enum used
double a = Math.Round(b, MidpointRounding.AwayFromZero);

//Two separate methods used
IEnumerable<Stuff> ascendingList = c.OrderBy(o => o.Key);
IEnumerable<Stuff> descendingList = c.OrderByDescending(o => o.Key); 

Não consigo articular completamente o que há de errado nisso.

Se parece um cheiro de código, parece um cheiro de código e - bem - cheira a um cheiro de código, provavelmente é um cheiro de código.

O que você quer fazer é:

1) Evite métodos com efeitos colaterais.

2) Manuseie os estados necessários com uma máquina de estado formal e central (como esta ).

Eu concordo com todas as preocupações de usar Parâmetros Booleanos para não determinar o desempenho para; melhorar, legibilidade, confiabilidade, redução da complexidade, redução dos riscos de encapsulamento e coesão deficientes e menor custo total de propriedade com manutenção.

Comecei a projetar hardware em meados dos anos 70, que agora chamamos de SCADA (controle supervisório e aquisição de dados), e esses eram equipamentos ajustados com código de máquina na EPROM, executando controles remotos macro e coletando dados em alta velocidade.

A lógica é chamada de máquinas Mealey & Moore , que chamamos agora de máquinas de estado finito . Isso também deve ser feito nas mesmas regras acima, a menos que seja uma máquina de tempo real com um tempo de execução finito e, em seguida, atalhos devem ser feitos para servir ao objetivo.

Os dados são síncronos, mas os comandos são assíncronos e a lógica de comando segue a lógica booleana sem memória, mas com comandos seqüenciais baseados na memória do estado anterior, presente e desejado seguinte. Para que isso funcionasse na linguagem de máquina mais eficiente (apenas 64kB), foi tomado muito cuidado para definir todos os processos de maneira heurística IBM HIPO. Isso às vezes significava passar variáveis ​​booleanas e executar ramificações indexadas.

Mas agora com muita memória e facilidade de OOK, hoje, o encapsulamento é um ingrediente essencial, mas uma penalidade quando o código foi contado em bytes para tempo real e o código da máquina SCADA.

Não é necessariamente errado, mas no seu exemplo concreto da ação, dependendo de algum atributo do "usuário", eu passaria por uma referência ao usuário em vez de um sinalizador.

Isso esclarece e ajuda de várias maneiras.

Qualquer pessoa que leia a instrução de chamada perceberá que o resultado será alterado dependendo do usuário.

Na função que é chamada em última análise, você pode implementar facilmente regras de negócios mais complexas porque pode acessar qualquer um dos atributos do usuário.

Se uma função / método na "cadeia" faz algo diferente dependendo de um atributo do usuário, é muito provável que uma dependência semelhante nos atributos do usuário seja introduzida em alguns dos outros métodos na "cadeia".

Na maioria das vezes, eu consideraria essa codificação ruim. Posso pensar em dois casos, no entanto, onde isso pode ser uma boa prática. Como já existem muitas respostas dizendo por que é ruim, ofereço duas vezes quando pode ser bom:

A primeira é se cada chamada na sequência faz sentido por si mesma. Faria sentido se o código de chamada pudesse ser alterado de true para false ou false para true, ou se o método chamado pudesse ser alterado para usar o parâmetro booleano diretamente, em vez de transmiti-lo. A probabilidade de dez chamadas desse tipo é pequena, mas pode acontecer e, se o fizer, seria uma boa prática de programação.

O segundo caso é um pouco exagerado, pois estamos lidando com um booleano. Mas se o programa tiver vários threads ou eventos, passar parâmetros é a maneira mais simples de rastrear dados específicos de threads / eventos. Por exemplo, um programa pode obter entrada de dois ou mais soquetes. O código em execução em um soquete pode precisar gerar mensagens de aviso, e um em execução em outro pode não. Então (mais ou menos) faz sentido que um conjunto booleano em um nível muito alto seja passado por muitas chamadas de método para os locais onde as mensagens de aviso podem ser geradas. Os dados não podem ser salvos (exceto com grande dificuldade) em qualquer tipo de global, porque vários encadeamentos ou eventos intercalados precisariam de seu próprio valor.

Para ter certeza, no último caso, eu provavelmente criaria uma classe / estrutura cujo único conteúdo fosse um booleano e passaria isso por aí. Eu provavelmente precisaria de outros campos em pouco tempo - como para onde enviar as mensagens de aviso, por exemplo.

O contexto é importante. Tais métodos são bastante comuns no iOS. Como apenas um exemplo usado com frequência, o UINavigationController fornece o método -pushViewController:animated:e o animatedparâmetro é um BOOL. O método executa essencialmente a mesma função de qualquer maneira, mas anima a transição de um controlador de exibição para outro se você passar YES e não se você passar NO. Isso parece inteiramente razoável; seria tolice ter que fornecer dois métodos no lugar deste, apenas para que você possa determinar se deve ou não usar animação.

Pode ser mais fácil justificar esse tipo de coisa no Objective-C, em que a sintaxe de nomeação de método fornece mais contexto para cada parâmetro do que em linguagens como C e Java. No entanto, acho que um método que usa um único parâmetro pode facilmente assumir um valor booleano e ainda fazer sentido:

file.saveWithEncryption(false);    // is there any doubt about the meaning of 'false' here?