Se as funções tiverem que fazer verificações nulas antes de fazer o comportamento pretendido, este design é ruim?

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

Erros de referência / ponteiro nulo, na maioria das vezes, são erros do programador. Frequentemente, os erros do programador são mais bem tratados 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 apenas 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 trabalho, mas é muito mais robusta:

3. Estruture sua classe de forma que seja praticamente impossível que _someEntity esteja em um estado inválido. Uma maneira de fazer isso é livrar-se de AssignEntity e 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 todos os argumentos de função quanto à validade antes de qualquer trabalho que você esteja realizando e, em outros, faz sentido dizer ao chamador que eles são responsáveis por garantir que suas entradas sejam válidas e não verificadas. Qual extremidade do espectro em que você está dependerá do domínio do seu problema. A terceira opção tem um benefício significativo em que você pode, até certo ponto, ter o compilador impor que o chamador faz tudo corretamente.

Se a terceira opção não é uma opção, então, na minha opinião, isso realmente não importa, desde que não falhe silenciosamente. Se a não verificação de null faz com que o programa exploda instantaneamente, tudo bem, mas se ele corromper os dados silenciosamente para causar problemas, é melhor verificar e lidar com isso imediatamente.

Como _someEntity pode ser modificado em qualquer estágio, faz sentido testá-lo toda vez que SetName for chamado. Afinal, poderia ter mudado desde a última vez que esse método foi chamado. Mas lembre-se de que o código em SetName não é thread-safe, portanto, você pode executar essa verificação em um thread, ter _someEntity definido como null por outro e, em seguida, o código criará um NullReferenceException .

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

1. faça uma cópia local de _someEntity e faça referência a essa cópia por meio do método
2. use um bloqueio em torno de _someEntity para garantir que ele não seja alterado à medida que o método for executado
3. use um try/catch e execute a mesma ação em qualquer NullReferenceException que ocorra no método como você faria na verificação nula inicial (nesse caso, uma ação nop ).

Mas o que você realmente deve fazer é parar e perguntar a si mesmo: você realmente precisa permitir que _someEntity seja sobrescrito? Por que não configurá-lo uma vez, através do construtor. Dessa forma, você só precisa fazer isso nulo 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 for possível, essa é a abordagem que eu recomendaria em tais situações. Se você não pode estar ciente da segurança do thread ao escolher como lidar com possíveis nulos.

Como um comentário bônus, sinto que seu código é estranho e pode ser melhorado. Todos:

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 tem a mesma funcionalidade, exceto por poder definir o campo por meio do definidor de propriedades, em vez de um método com um nome diferente.

But should the method not have this check?

Esta é a sua escolha

Ao criar um método public , você está oferecendo ao público a oportunidade de chamá-lo. Isso sempre vem junto com um contrato implícito em como chamar esse método e o que esperar ao fazer isso. Este contrato pode (ou não) incluir " yeah, uhhm, se você passar null como 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ê. As coisas importantes são para

• crie uma API útil e consistente e

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

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

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

Os métodos

• público e protegido são aqueles em que você não controla o chamador. Eles devem lançar quando passarem de null. Dessa forma, você treina seus chamadores para nunca lhe passar um nulo, porque eles gostariam que seu programa não falhasse.

Os métodos

• interno e privado são aqueles em que você faz controlam o chamador. Eles devem afirmar quando forem passados como nulos; eles provavelmente irão falhar mais tarde, mas pelo menos você terá a afirmação primeiro e uma oportunidade de invadir o depurador. Mais uma vez, você quer treinar seus chamadores para chamá-lo corretamente, fazendo-o doer quando eles não o fazem.

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

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

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

e no site de chamadas:

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

Porque (1) você está treinando seus chamadores que null é um bom valor, e (2) não está claro qual é a semântica desse valor. Em vez disso, faça isso:

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 chamadas é assim:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

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

As outras respostas apontam que seu código pode ser limpo para não precisar de uma verificação nula onde você o possui, no entanto, para uma resposta geral sobre como uma verificação nula pode ser útil para considerar 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 lhe dá uma vantagem para manutenção. Se alguma vez ocorrer um defeito em seu código onde algo passa um objeto nulo para o método, você obterá informações úteis do método sobre qual parâmetro era nulo. Sem as verificações, você receberá um NullReferenceException na linha:

a.Something = b.Something;

E (dado 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 era nulo, o que pode ser extremamente útil ao tentar desfazer um defeito.

Eu notaria que o padrão no seu código original:

if (x == null) return;

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

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 verificada , você forçará o usuário do método a reagir e permitir que ele se recupere. O programa ainda funciona como esperado.

Se você não fizer uma verificação de referência nula (ou lançar uma exceção não verificada), poderá lançar um NullReferenceException desmarcado, que geralmente não é tratado pelo usuário do método e nem mesmo pode encerrar o aplicativo. Isso significa que a função é obviamente mal entendida e, portanto, a aplicação está com defeito.

Algo de uma extensão para a resposta de @ null, mas na minha experiência, faça verificações nulas independentemente de qual seja.

Boa programação defensiva é a atitude que você codifica a rotina atual isoladamente, presumindo que todo o resto do programa está tentando derrubá-la. Quando você está escrevendo um código de missão crítica, em que a falha não é uma opção, esse é praticamente o único caminho a ser seguido.

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

Se null for um valor aceitável, por ex. qualquer um dos segundo até quinto parâmetros para a rotina MSVC _splitpath (), então, silenciosamente, é o comportamento correto.

Se null nunca deveria acontecer ao chamar a rotina em questão, ou seja, no caso do OP, o contrato da API exige que AssignEntity() tenha sido chamado com um Entity válido e, em seguida, ative uma exceção fazer muito barulho no processo.

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