Em um novo trabalho, estou sendo sinalizado em revisões de código para código como este:
PowerManager::PowerManager(IMsgSender* msgSender)
: msgSender_(msgSender) { }
void PowerManager::SignalShutdown()
{
msgSender_->sendMsg("shutdown()");
}
Foi-me dito que o último método deveria ler:
void PowerManager::SignalShutdown()
{
if (msgSender_) {
msgSender_->sendMsg("shutdown()");
}
}
i.e., eu preciso colocar um NULL em volta da variável msgSender_ , mesmo que seja um membro de dados privados. É difícil para mim me impedir de usar expletivos para descrever como me sinto sobre esse pedaço de "sabedoria". Quando eu peço uma explicação, recebo uma série de histórias de horror sobre como um programador júnior, durante alguns anos, ficou confuso sobre como uma classe deveria funcionar e apagou acidentalmente um membro que ele não deveria ter (e configurou para NULL depois, aparentemente), e as coisas explodiram no campo logo após o lançamento de um produto, e nós "aprendemos da maneira difícil, confie em nós" que é melhor apenas NULL verificar tudo .
Para mim, isso parece programação de cultos de carga , simples e simples. Alguns colegas bem-intencionados estão sinceramente tentando me ajudar a "entender" e ver como isso me ajudará a escrever um código mais robusto, mas ... não posso deixar de sentir que são eles que não entendem .
É razoável que um padrão de codificação exija que cada ponteiro não referenciado em uma função seja verificado por NULL primeiro - até mesmo membros de dados privados? (Nota: Para dar algum contexto, nós fazemos um dispositivo eletrônico de consumo, não um sistema de controle de tráfego aéreo ou algum outro produto 'falha-igual a pessoas-die').
EDITAR : no exemplo acima, o colaborador msgSender_ não é opcional. Se for sempre NULL , isso indica um erro. A única razão pela qual ele é passado para o construtor é, portanto, PowerManager pode ser testado com uma subclasse IMsgSender simulada.
RESUMO : Houve algumas ótimas respostas a essa pergunta, obrigado a todos. Eu aceitei o de @Aaronps principalmente devido à sua brevidade. Parece haver um acordo geral bastante amplo de que:
NULL guardas para todos os ponteiro não referenciado é um exagero, mas const e assert são uma alternativa mais esclarecida para NULL guards para verificar se as condições prévias de uma função são atendidas. Depende do 'contrato':
Se PowerManager DEVE ter um IMsgSender válido, nunca verifique se há nulo, deixe-o morrer mais cedo.
Se, por outro lado, MAY tiver IMsgSender , você precisará verificar cada vez que usar, de forma simples.
Comentário final sobre a história do programador júnior, o problema é, na verdade, a falta de procedimentos de teste.
Eu sinto que o código deve ler:
PowerManager::PowerManager(IMsgSender* msgSender)
: msgSender_(msgSender)
{
assert(msgSender);
}
void PowerManager::SignalShutdown()
{
assert(msgSender_);
msgSender_->sendMsg("shutdown()");
}
Isso é realmente melhor do que proteger o NULL, porque deixa muito claro que a função nunca deve ser chamada se msgSender_ for NULL. Também garante que você perceberá se isso acontecer.
A "sabedoria" compartilhada de seus colegas silenciosamente ignorará esse erro, com resultados imprevisíveis.
Em geral, os bugs são mais fáceis de corrigir se forem detectados mais perto de sua causa. Neste exemplo, o protetor NULL proposto levaria a uma mensagem de desligamento não sendo definida, o que pode ou não resultar em um bug perceptível. Você teria mais dificuldade em retroceder para a função SignalShutdown do que se o aplicativo inteiro morresse, produzindo um backtrace ou um dump de memória acessível apontando diretamente para SignalShutdown() .
É um pouco contra-intuitivo, mas falhar assim que algo está errado tende a tornar seu código mais robusto. Isso porque você realmente encontra os problemas, e tende a ter causas muito óbvias também.
Se msgSender nunca deve ser null , você deve colocar a null check apenas no construtor. Isto também é verdade para qualquer outra entrada na classe - coloque a verificação de integridade no ponto de entrada no 'módulo' - classe, função etc.
Minha regra básica é realizar verificações de integridade entre os limites do módulo - a classe neste caso. Além disso, uma classe deve ser pequena o suficiente para que seja possível verificar mentalmente a integridade da vida útil dos membros da classe - garantindo que erros como exclusões / designações nulas sejam evitados. A verificação nula que é executada no código em sua postagem pressupõe que qualquer uso inválido realmente atribui nulo ao ponteiro - o que nem sempre é o caso. Como o 'uso inválido' implica inerentemente que quaisquer suposições sobre o código regular não se aplicam, não podemos ter certeza de pegar todos os tipos de erros de ponteiro - por exemplo, uma exclusão, incremento, etc.
Além disso - se tiver certeza de que o argumento nunca pode ser nulo, considere o uso de referências, dependendo do uso da classe. Caso contrário, considere usar std::unique_ptr ou std::shared_ptr em vez de um ponteiro bruto.
Não, não é razoável verificar qualquer referência de ponteiro para o ponteiro sendo NULL .
As verificações de ponteiro nulo são valiosas em argumentos de função (incluindo argumentos de construtor) para garantir que as pré-condições sejam atendidas ou tomar a ação apropriada se um parâmetro opcional não for fornecido e são valiosas para verificar a invariável de uma classe depois que você expôs a classe internals. Mas se a única razão para um ponteiro ter se tornado NULL é a existência de um bug, então não faz sentido verificar. Esse bug poderia facilmente definir o ponteiro para outro valor inválido.
Se eu fosse confrontado com uma situação como a sua, faria duas perguntas:
assert(msgSender_) em vez da verificação nula? Se você acabou de colocar o check-in nulo, pode ter evitado uma falha, mas pode ter criado uma situação pior, porque o software continua na premissa de que uma operação ocorreu enquanto, na realidade, essa operação foi ignorada. Isso pode levar a que outras partes do software se tornem instáveis. Este exemplo parece ser mais sobre a vida útil do objeto do que se um parâmetro de entrada é nulo ou não †. Como você mencionou que PowerManager deve sempre ter um IMsgSender válido, passar o argumento por ponteiro (permitindo assim a possibilidade de um ponteiro nulo) me parece uma falha de design ††.
Em situações como esta, prefiro alterar a interface para que os requisitos do autor da chamada sejam aplicados pela linguagem:
PowerManager::PowerManager(const IMsgSender& msgSender)
: msgSender_(msgSender) {}
void PowerManager::SignalShutdown() {
msgSender_->sendMsg("shutdown()");
}
Reescrevê-lo desta maneira diz que PowerManager precisa manter uma referência a IMsgSender para toda a sua vida útil. Isso, por sua vez, também estabelece um requisito implícito de que IMsgSender deve viver mais do que PowerManager e nega a necessidade de qualquer verificação de ponteiro nulo ou asserções dentro de PowerManager .
Você também pode escrever a mesma coisa usando um ponteiro inteligente (via boost ou c ++ 11), para forçar explicitamente IMsgSender a viver mais do que PowerManager :
PowerManager::PowerManager(std::shared_ptr<IMsgSender> msgSender)
: msgSender_(msgSender) {}
void PowerManager::SignalShutdown() {
// Here, we own a smart pointer to IMsgSender, so even if the caller
// destroys the original pointer, we still have a valid copy
msgSender_->sendMsg("shutdown()");
}
Esse método é preferido se for possível que a vida útil de IMsgSender não possa ser maior do que PowerManager (ou seja, x = new IMsgSender(); p = new PowerManager(*x); ).
† Com relação aos ponteiros: a verificação nula excessiva torna o código mais difícil de ler e não melhora a estabilidade (melhora a aparência da estabilidade, que é muito pior).
Em algum lugar, alguém tem um endereço de memória para armazenar o IMsgSender . É responsabilidade dessa função certificar-se de que a alocação foi bem-sucedida (verificar valores de retorno de biblioteca ou manipular corretamente std::bad_alloc exceções), para não passar por ponteiros inválidos.
Como o PowerManager não possui o IMsgSender (é apenas emprestado por um tempo), ele não é responsável por alocar ou destruir essa memória. Esta é outra razão pela qual eu prefiro a referência.
†† Como você é novo nesse trabalho, espero que esteja invadindo o código existente. Então, por falha de design, quero dizer que a falha está no código com o qual você está trabalhando. Assim, as pessoas que estão sinalizando seu código porque ele não está verificando os ponteiros nulos estão realmente sinalizando para escrever código que requer ponteiros:)
Como as exceções, as condições de guarda são úteis apenas se você souber o que fazer para se recuperar do erro ou se desejar fornecer uma mensagem de exceção mais significativa.
Engolir um erro (seja detectado como uma exceção ou como uma verificação de guarda) é a coisa certa a fazer quando o erro não importa. O local mais comum para eu ver os erros sendo engolidos está no código de log de erros - você não quer travar um aplicativo porque não conseguiu registrar uma mensagem de status.
Sempre que uma função é chamada e não é um comportamento opcional, ela deve falhar alto, não silenciosamente.
Editar: ao pensar sobre sua história de programador júnior, parece que o que aconteceu foi que um membro privado foi definido como nulo quando isso nunca deveria acontecer. Eles tiveram um problema com a escrita inadequada e estão tentando consertá-lo validando a leitura. Isso é para trás. No momento em que você o identifica, o erro já aconteceu. A solução executável de revisão / compilador de código para isso não são condições de guarda, mas sim getters e setters ou membros const.
Como outros observaram, isso depende se msgSender pode ou não ser legitimamente NULL . O seguinte assume que deve nunca ser NULL.
void PowerManager::SignalShutdown()
{
if (!msgSender_)
{
throw SignalException("Shut down failed because message sender is not set.");
}
msgSender_->sendMsg("shutdown()");
}
A "correção" proposta pelos outros membros da sua equipe viola o princípio Programas inativos não dizem mentiras . Bugs são realmente difíceis de encontrar como é. Um método que muda silenciosamente seu comportamento com base em um problema anterior, não apenas dificulta encontrar o primeiro bug, mas também adiciona um segundo bug.
O júnior causou estragos por não verificar um valor nulo. E se esse pedaço de código causar estragos, continuando a rodar em um estado indefinido (o dispositivo está ligado, mas o programa "pensa" que está desligado)? Talvez outra parte do programa faça algo que só seja seguro quando o dispositivo estiver desligado.
Qualquer uma dessas abordagens evitará falhas silenciosas:
Use as afirmações sugeridas por esta resposta , mas verifique se elas estão ativadas no código de produção . Isso, é claro, poderia causar problemas se outras afirmações fossem escritas com a suposição de que elas seriam desativadas na produção.
Lance uma exceção se ela for nula.
Concordo com o trapping do nulo no construtor. Além disso, se o membro for declarado no cabeçalho como:
IMsgSender* const msgSender_;
Em seguida, o ponteiro não pode ser alterado após a inicialização, portanto, se estiver bom na construção, ele ficará bem durante a vida útil do objeto que o contém. (O objeto apontado para não será const.)
Isso é claro Perigoso!
Trabalhei com um desenvolvedor sênior em uma base de código C com os "padrões" mais shoddiest que pressionavam pela mesma coisa, para verificar cegamente todos os ponteiros para null. O desenvolvedor acabaria fazendo coisas assim:
// Pre: vertex should never be null.
void transform_vertex(Vertex* vertex, ...)
{
// Inserted by my "wise" co-worker.
if (!vertex)
return;
...
}
Uma vez, tentei remover essa verificação da condição certa uma vez nessa função e substituí-la por um assert para ver o que aconteceria.
Para meu horror, eu encontrei milhares de linhas de código no codebase que estavam passando nulos para esta função, mas onde os desenvolvedores, provavelmente confusos, trabalharam e apenas adicionaram mais código até que as coisas funcionassem.
Para meu horror, descobri que esse problema era prevalente em todos os lugares na base de código, verificando nulos. A base de código havia crescido ao longo de décadas para se basear nessas verificações para poder violar silenciosamente até mesmo as pré-condições documentadas mais explicitamente. Ao remover essas verificações mortais em favor de asserts , todos os erros humanos lógicos ao longo de décadas na base de código seriam revelados, e nos afogaríamos neles.
Levou apenas duas linhas aparentemente inocentes de código como este + tempo e uma equipe para acabar mascarando mil bugs acumulados.
Estes são os tipos de práticas que fazem com que os erros dependam de outros erros para que o software funcione. É um cenário de pesadelo . Ele também faz com que todo erro lógico relacionado a violar tais pré-condições mostre misteriosamente um milhão de linhas de código longe do site em que ocorreu o erro, já que todas essas verificações nulas escondem o bug e ocultam o bug até chegarmos a um lugar que esqueceu para esconder o bug.
Para simplesmente verificar nulos cegamente em todos os lugares onde um ponteiro nulo viola uma pré-condição é, para mim, total insanidade, a menos que seu software seja tão crítico contra falhas de asserção e quedas de produção que o potencial deste cenário seja preferível.
Is it reasonable for a coding standard to require that every single pointer dereferenced in a function be checked for NULL first—even private data members?
Então eu diria, absolutamente não. Não é nem "seguro". Pode muito bem ser o oposto e mascarar todos os tipos de erros em toda a sua base de código, o que, ao longo dos anos, pode levar aos cenários mais terríveis.
assert é o caminho a seguir aqui. Violações de pré-condições não devem passar despercebidas, ou a lei de Murphy pode facilmente entrar em cena.
Objective-C , por exemplo, trata cada chamada de método em um objeto nil como um não operacional que avalia um valor zero-ish. Existem algumas vantagens para essa decisão de projeto no Objective-C, pelas razões sugeridas na sua pergunta. O conceito teórico de proteção nula de todo chamado de método tem algum mérito se for bem publicado e consistentemente aplicado.
Dito isso, o código vive em um ecossistema, não em um vácuo. O comportamento nulo-guardado seria não-idiomático e surpreendente em C ++, e, portanto, deve ser considerado prejudicial. Em resumo, ninguém escreve código C ++ dessa maneira, então não faça isso! Como contra-exemplo, note que chamar free() ou delete em um NULL em C e C ++ é garantido como não-operacional.
Em seu exemplo, provavelmente valeria a pena colocar uma declaração no construtor que msgSender não é nula. Se o construtor chamou um método em msgSender imediatamente, então tal afirmação não seria necessária, já que ele iria falhar de qualquer maneira. No entanto, como é apenas armazenar msgSender para uso futuro, não seria óbvio, ao olhar para um rastreamento de pilha de SignalShutdown() , como o valor passou a ser NULL , portanto, uma afirmação o construtor tornaria a depuração significativamente mais fácil.
Melhor ainda, o construtor deve aceitar uma referência const IMsgSender& , que possivelmente não pode ser NULL .
O motivo pelo qual você é solicitado a evitar desreferências nulas é garantir que seu código seja robusto. Exemplos de programadores júnior há muito tempo são apenas exemplos. Qualquer um pode quebrar o código por acidente e causar uma desreferência nula - especialmente para globals e classes globais. Em C e C ++ é ainda mais possível acidentalmente, com a capacidade de gerenciamento de memória direta. Você pode se surpreender, mas esse tipo de coisa acontece com muita frequência. Mesmo por desenvolvedores muito experientes, muito experientes e muito experientes.
Você não precisa anular tudo, mas precisa se proteger contra as discrepâncias com probabilidade decente de serem nulas. Isso é comumente quando eles são alocados, usados e desreferenciados em diferentes funções. É possível que uma das outras funções seja modificada e quebre sua função. Também é possível que uma das outras funções possa ser chamada fora de ordem (como se você tivesse um desalocador que possa ser chamado separadamente do destruidor).
Eu prefiro a abordagem que seus colegas de trabalho estão lhe dizendo em combinação com o uso de assert. Crash no ambiente de teste, então é mais óbvio que há um problema para corrigir e falhar normalmente na produção.
Você também deve usar uma ferramenta robusta de correção de código, como coverity ou fortify. E você deve estar lidando com todos os avisos do compilador.
Editar: como outros já mencionaram, falhar silenciosamente, como em vários exemplos de código, geralmente também é algo errado. Se a sua função não puder se recuperar do valor que está sendo nulo, ela deverá retornar um erro (ou lançar uma exceção) ao seu chamador. O responsável pela chamada é responsável por consertar sua ordem de chamada, recuperar ou retornar um erro (ou lançar uma exceção) ao chamador, e assim por diante. Eventualmente, uma função é capaz de se recuperar e se mover normalmente, recuperar e falhar normalmente (como um banco de dados que falha em uma transação devido a um erro interno de um usuário mas não sair ativamente) ou a função determina que o estado do aplicativo está corrompido e irrecuperável e o aplicativo sai.
O uso de um ponteiro em vez de uma referência informa que msgSender é apenas uma opção e aqui a verificação nula seria esteja certo. O snippet de código é muito lento para decidir isso. Talvez existam outros elementos em PowerManager que são valiosos (ou testáveis) ...
Ao escolher entre ponteiro e referência, peso as duas opções. Se eu tiver que usar um ponteiro para um membro (mesmo para membros privados), eu tenho que aceitar o procedimento if (x) cada vez que eu desreferencia.
Depende do que você quer que aconteça.
Você escreveu que está trabalhando em "um dispositivo eletrônico para o consumidor". Se, de alguma forma, um bug é introduzido, definindo msgSender_ para NULL , você quer
SignalShutdown , mas continuando o restante da operação ou Dependendo do impacto do sinal de desligamento não ser enviado, a opção 1 pode ser uma opção viável. Se o usuário puder continuar a ouvir sua música, mas a exibição ainda mostrar o título da faixa anterior, isso pode ser preferível a uma falha completa do dispositivo.
É claro que, se você escolher a opção 1, um assert (como recomendado por outros) é vital para reduzir a probabilidade de um bug desse tipo passar despercebido durante o desenvolvimento. A proteção if null está lá apenas para mitigação de falha no uso da produção.
Tags c++ coding-standards