Se as funções precisam fazer verificações nulas antes de executar o comportamento pretendido, esse design é ruim?

67

Portanto, não sei se esse é um design de código bom ou ruim, por isso acho melhor perguntar.

Costumo criar métodos que processam dados envolvendo classes e, muitas vezes, faço muitas verificações nos métodos para garantir que não receba referências nulas ou outros erros antes.

Para um exemplo muito básico:

// fields and properties
private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}
public void SetName(string name)
{
    if (_someEntity == null) return; //check to avoid null ref
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

Então, como você pode ver, estou verificando se há nulo sempre. Mas o método não deve ter essa verificação?

Por exemplo, o código externo deve limpar os dados com antecedência, para que os métodos não precisem ser validados como abaixo:

 if(entity != null) // this makes the null checks redundant in the methods
 {
     Manager.AssignEntity(entity);
     Manager.SetName("Test");
 }

Em resumo, os métodos devem ser "validadores de dados" e, em seguida, processam os dados, ou devem ser garantidos antes de chamá-lo, e se você não validar antes de chamar o método, deve ocorrer um erro (ou capturar o erro)?

WDUK
fonte
7
O que acontece quando alguém falha na verificação nula e o SetName lança uma exceção de qualquer maneira?
Robert Harvey
5
O que acontece se eu realmente quiser que algumaEntity seja nula? Você decide o que deseja, someEntity pode ser nulo ou não, e então escreve seu código de acordo. Se você deseja que nunca seja Nulo, pode substituir cheques por afirmações.
precisa saber é o seguinte
5
Você pode assistir os limites de Gary Bernhardt falarem sobre fazer uma divisão clara entre a limpeza de dados (ou seja, verificar nulos etc.) em um shell externo e ter um sistema interno que não precisa se preocupar com essas coisas - e apenas faça trabalhos.
Mgarciaisaia
11
Com C # 8 você potencialmente nunca precisa se preocupar com exceções referência nula com os ajustes corretos ligado: blogs.msdn.microsoft.com/dotnet/2017/11/15/...
JᴀʏMᴇᴇ
3
Essa é uma das razões pelas quais a equipe de linguagem C # deseja introduzir tipos de referência nulos (e não nulos) -> github.com/dotnet/csharplang/issues/36
Mafii

Respostas:

168

O problema com o seu exemplo básico não é a verificação nula, é a falha silenciosa .

Erros nulos de ponteiro / referência, na maioria das vezes, são erros de programador. Os erros do programador geralmente são mais bem resolvidos ao falhar imediatamente e em voz alta. Você tem três maneiras gerais de lidar com o problema nessa situação:

  1. Não se preocupe em verificar e deixe o tempo de execução lançar a exceção nullpointer.
  2. Verifique e lance uma exceção com uma mensagem melhor que a mensagem básica do NPE.

Uma terceira solução é mais trabalhosa, mas muito mais robusta:

  1. Estruture sua classe de modo que seja praticamente impossível _someEntityestar em um estado inválido. Uma maneira de fazer isso é se livrar AssignEntitye exigir isso como um parâmetro para instanciação. Outras técnicas de injeção de dependência também podem ser úteis.

Em algumas situações, faz sentido verificar a validade de todos os argumentos de função antes de qualquer trabalho que você esteja realizando; em outras, faz sentido informar ao responsável pela chamada que é responsável por garantir que suas entradas sejam válidas e não verificar. Qual extremidade do espectro em que você vai depender do domínio do seu problema. A terceira opção tem um benefício significativo, pois você pode, até certo ponto, fazer com que o compilador imponha que o chamador faça tudo corretamente.

Se a terceira opção não for uma opção, na minha opinião, isso realmente não importa, contanto que não falhe silenciosamente. Se a não verificação de nulo faz com que o programa exploda instantaneamente, tudo bem, mas se, em vez disso, corromper os dados silenciosamente para causar problemas no caminho, é melhor verificar e lidar com eles imediatamente.

whatsisname
fonte
10
@aroth: Qualquer projeto de qualquer coisa, incluindo o software, virá com restrições. O próprio ato de projetar é selecionar para onde vão essas restrições, e não é de minha responsabilidade nem devo aceitar qualquer entrada e esperar que faça algo útil com ela. Eu sei se nullestá errado ou é inválido, porque na minha biblioteca hipotética defini os tipos de dados e o que é válido e o que não é. Você chama isso de "forçar suposições", mas adivinhe: é o que toda biblioteca de software existente faz. Há momentos em que nulo é um valor válido, mas isso nem sempre é "".
Whatsisname
4
"que seu código está quebrado porque não manipula nullcorretamente" - Esse é um mal-entendido sobre o que significa manipulação adequada. Para a maioria dos programadores Java, isso significa lançar uma exceção para qualquer entrada inválida. Usando @ParametersAreNonnullByDefault, significa também para todos null, a menos que o argumento esteja marcado como @Nullable. Fazer qualquer outra coisa significa prolongar o caminho até que finalmente chegue a outro lugar e, assim, também prolongar o tempo perdido na depuração. Bem, ignorando os problemas, você pode conseguir que ele nunca sopra, mas o comportamento incorreto é bastante garantido.
Maaartinus
4
@aroth Caro Senhor, eu odiaria trabalhar com qualquer código que você escrever. Explosões são infinitamente melhores do que fazer algo sem sentido, que é a única outra opção para entrada inválida. As exceções obrigam-me, quem liga, a decidir qual é a coisa certa a fazer nesses casos, quando o método não pode adivinhar o que eu pretendia. Exceções verificadas e extensão desnecessária Exceptionsão debates completamente independentes. (Concordo que ambos são problemáticos.) Para este exemplo específico, nullobviamente não faz sentido aqui se o objetivo da classe é invocar métodos no objeto.
Jpmc26 /
3
"seu código não deve forçar suposições no mundo do chamador" - É impossível não forçar suposições ao chamador. É por isso que precisamos tornar essas suposições o mais explícitas possível.
Diogo Kollross
3
@aroth seu código está quebrado porque está passando meu código como nulo quando deveria estar passando algo que tem Name como uma propriedade. Qualquer comportamento que não seja explodir é algum tipo de versão bizarro do backdoor da digitação dinâmica IMHO.
mbrig
56

Como _someEntitypode ser modificado em qualquer estágio, faz sentido testá-lo sempre que SetNamefor chamado. Afinal, poderia ter mudado desde a última vez em que esse método foi chamado. Mas lembre-se de que o código SetNamenão é seguro para threads; portanto, você pode executar essa verificação em um thread, _someEntitydefinido nullpor outro e, em seguida, o código exibirá um sinal de NullReferenceExceptionqualquer maneira.

Uma abordagem para esse problema, portanto, é ficar ainda mais defensivo e seguir um destes procedimentos:

  1. faça uma cópia local _someEntitye faça referência a essa cópia através do método,
  2. use um bloqueio _someEntitypara garantir que ele não seja alterado à medida que o método for executado,
  3. use try/catchae execute a mesma ação em qualquer NullReferenceExceptionuma que ocorra dentro do método que você executaria na verificação nula inicial (nesse caso, uma nopação).

Mas o que você realmente deve fazer é parar e se perguntar: você realmente precisa _someEntityser substituído? Por que não configurá-lo uma vez, através do construtor. Dessa forma, você só precisa fazer essa verificação nula uma vez:

private readonly Entity _someEntity;

public Constructor(Entity someEntity)
{
    if (someEntity == null) throw new ArgumentNullException(nameof(someEntity));
    _someEntity = someEntity;
}

public void SetName(string name)
{
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

Se possível, essa é a abordagem que eu recomendaria em tais situações. Se você não se preocupa com a segurança do encadeamento ao escolher como lidar com possíveis nulos.

Como um comentário adicional, sinto que seu código é estranho e pode ser melhorado. Tudo de:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}

pode ser substituído por:

public Entity SomeEntity { get; set; }

Que possui a mesma funcionalidade, exceto por poder definir o campo por meio do configurador de propriedades, em vez de um método com um nome diferente.

David Arno
fonte
5
Por que isso responde à pergunta? A pergunta aborda a verificação nula e isso é praticamente uma revisão de código.
IEatBagels
17
@TopinFrassi, explica que a atual abordagem de verificação nula não é segura para threads. Em seguida, aborda outras técnicas de programação defensiva para contornar isso. Em seguida, termina com a sugestão de usar a imutabilidade para evitar a necessidade de ter essa programação defensiva em primeiro lugar. E, como um bônus adicional, oferece conselhos sobre como estruturar melhor o código.
David Arno
23

Mas o método não deve ter essa verificação?

Esta é a sua escolha.

Ao criar um publicmétodo, você está oferecendo ao público a oportunidade de chamá-lo. Isso sempre vem junto com um contrato implícito sobre como chamar esse método e o que esperar ao fazê-lo. Este contrato pode (ou não) incluir " sim, uhhm, se você passar nullcomo um valor de parâmetro, ele explodirá na sua cara ". Outras partes desse contrato podem ser " oh, BTW: toda vez que dois threads executam esse método ao mesmo tempo, um gatinho morre ".

Que tipo de contrato você deseja criar depende de você. O importante é

  • crie uma API que seja útil, consistente e

  • documentá-lo bem, especialmente para esses casos extremos.

Quais são os casos prováveis ​​de como seu método está sendo chamado?

nulo
fonte
Bem, a segurança do thread é a exceção, não a regra, quando há acesso simultâneo. Portanto, o uso do estado oculto / global é documentado, assim como qualquer concorrência de caso é segura.
Deduplicator
14

As outras respostas são boas; Eu gostaria de estendê-los fazendo alguns comentários sobre modificadores de acessibilidade. Primeiro, o que fazer se nullnunca for válido:

  • métodos públicos e protegidos são aqueles em que você não controla o chamador. Eles devem jogar quando passados ​​nulos. Dessa forma, você treina seus chamadores para nunca passarem nulos, porque eles gostariam que o programa deles não falhasse.

  • internas e privadas métodos são aqueles onde você não controlar o chamador. Eles devem afirmar quando passaram nulos; eles provavelmente travarão mais tarde, mas pelo menos você obteve a afirmação primeiro e uma oportunidade de invadir o depurador. Mais uma vez, você deseja treinar seus chamadores para chamá-lo corretamente, causando danos quando eles não o fazem.

Agora, o que você faria se isso pode ser válido para um nulo para ser passado em? Eu evitaria essa situação com o padrão de objeto nulo . Ou seja: crie uma instância especial do tipo e exija que ela seja usada sempre que um objeto inválido for necessário . Agora você está de volta ao mundo agradável de jogar / afirmar cada uso de null.

Por exemplo, você não deseja estar nesta situação:

class Person { ... }
...
class ExpenseReport { 
  public void SubmitReport(Person approver) { 
    if (approver == null) ... do something ...

e no site da chamada:

// I guess this works but I don't know what it means
expenses.SubmitReport(null); 

Porque (1) você está treinando seus chamadores que nulo é um bom valor e (2) não está claro quais são as semânticas desse valor. Em vez disso, faça o seguinte:

class ExpenseReport { 
  public static readonly Person ApproverNotRequired = new Person();
  public static readonly Person ApproverUnknown = new Person();
  ...
  public void SubmitReport(Person approver) { 
    if (approver == null) throw ...
    if (approver == ApproverNotRequired) ... do something ...
    if (approver == ApproverUnknown) ... do something ...

E agora o site de chamada fica assim:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

e agora o leitor do código sabe o que significa.

Eric Lippert
fonte
Melhor ainda, não tenha uma cadeia de ifs para os objetos nulos, mas use o polimorfismo para fazê-los se comportar corretamente.
21978 Konrad Rudolph
@KonradRudolph: De fato, essa é frequentemente uma boa técnica. Eu gosto de usar isso para estruturas de dados, onde eu faço um EmptyQueuetipo que lança quando desenfileirado e assim por diante.
Eric Lippert
@EricLippert - Perdoe-me, porque eu ainda estou aprendendo aqui, mas acho que se a Personclasse era imutável, e não continha um construtor argumento zero, como você construir suas ApproverNotRequiredou ApproverUnknowncasos? Em outras palavras, quais valores você passaria no construtor?
2
Adoro esbarrar nas suas respostas / comentários em todo o SE, eles são sempre perspicazes. Assim que eu vi afirmando para os métodos internos / privados I rolado para baixo à espera que seja uma resposta Eric Lippert, não estava desapontado :)
Bob Esponja
4
Vocês estão pensando demais no exemplo específico que eu dei. O objetivo era obter a ideia do padrão de objeto nulo rapidamente. Não era para ser um exemplo de bom design em um sistema de automação de documentos e salários. Em um sistema real, fazer "aprovador" ter o tipo "pessoa" provavelmente é uma má idéia, pois não corresponde ao processo de negócios; pode não haver aprovador, você pode precisar de vários e assim por diante. Como sempre observo ao responder perguntas nesse domínio, o que realmente queremos é um objeto de política . Não fique preso no exemplo.
Eric Lippert
8

As outras respostas apontam que seu código pode ser limpo para não precisar de uma verificação nula onde você a possui, no entanto, para obter uma resposta geral sobre o que pode ser uma verificação nula útil, considere o seguinte código de exemplo:

public static class Foo {
    public static void Frob(Bar a, Bar b) {
        if (a == null) throw new ArgumentNullException(nameof(a));
        if (b == null) throw new ArgumentNullException(nameof(b));

        a.Something = b.Something;
    }
}

Isso oferece uma vantagem para a manutenção. Se ocorrer algum defeito no seu código em que algo passe um objeto nulo para o método, você obterá informações úteis do método sobre qual parâmetro foi nulo. Sem as verificações, você receberá uma NullReferenceException na linha:

a.Something = b.Something;

E (considerando que a propriedade Something é um tipo de valor), a ou b pode ser nulo e você não terá como saber qual do rastreamento de pilha. Com as verificações, você saberá exatamente qual objeto foi nulo, o que pode ser extremamente útil ao tentar remover um defeito.

Gostaria de observar que o padrão no seu código original:

if (x == null) return;

Pode ter seus usos em determinadas circunstâncias, também pode (possivelmente com mais freqüência) fornecer uma maneira muito boa de mascarar problemas subjacentes em sua base de código.

Paddy
fonte
4

Não, não, mas depende.

Você só faz uma verificação de referência nula se quiser reagir a ela.

Se você fizer uma verificação de referência nula e lançar uma exceção , forçará o usuário do método a reagir e permitir que ele se recupere. O programa ainda funciona conforme o esperado.

Se você não fizer uma verificação de referência nula (ou lançar uma exceção desmarcada), ela poderá lançar uma desmarcada NullReferenceException, que geralmente não é tratada pelo usuário do método e pode até desligar o aplicativo. Isso significa que a função é obviamente completamente incompreendida e, portanto, o aplicativo está com defeito.

Herr Derb
fonte
4

Algo como uma extensão da resposta do @ null, mas, na minha experiência, faça verificações nulas, não importa o quê.

Uma boa programação defensiva é a atitude de codificar a rotina atual isoladamente, supondo que todo o resto do programa esteja tentando travá-la. Quando você está escrevendo um código de missão crítica, onde a falha não é uma opção, esse é o único caminho a percorrer.

Agora, como você realmente lida com um nullvalor, isso depende muito do seu caso de uso.

Se nullfor um valor aceitável, por exemplo, qualquer um dos parâmetros do segundo ao quinto da rotina _splitpath () da MSVC, lidar com ele silenciosamente é o comportamento correto.

Se nullnunca deveria acontecer ao chamar a rotina em questão, ou seja, no caso do OP, o contrato da API precisa AssignEntity()ser chamado com um válido e, em Entityseguida, lance uma exceção ou falhe, garantindo que você faça muito barulho no processo.

Nunca se preocupe com o custo de uma verificação nula, eles são muito baratos se acontecerem e, com os compiladores modernos, a análise de código estático pode e irá eliminá-los completamente se o compilador determinar que isso não acontecerá.

dgnuff
fonte
Ou evite-o completamente (48 minutos e 29 segundos: "Nunca use ou permita um local nulo perto de seu programa" ).
Peter Mortensen