¿Es razonable anular la protección de cada puntero desreferenciado?

Depende del 'contrato':

Si PowerManager DEBE tener un IMsgSender válido, nunca verifique si es nulo, deje que muera antes.

Si, por otro lado, PUEDE tener un IMsgSender , entonces debes verificar cada vez que lo uses, tan simple como eso.

Comentario final sobre la historia del programador junior, el problema es en realidad la falta de procedimientos de prueba.

Siento que el código debería leerse:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender)
{
    assert(msgSender);
}

void PowerManager::SignalShutdown()
{
    assert(msgSender_);
    msgSender_->sendMsg("shutdown()");
}

Esto es realmente mejor que proteger NULL, porque deja muy claro que nunca se debe llamar a la función si msgSender_ es NULL. También se asegura de que notará si esto sucede.

La "sabiduría" compartida de sus colegas ignorará silenciosamente este error, con resultados impredecibles.

En general, los errores son más fáciles de solucionar si se detectan más cerca de su causa. En este ejemplo, la protección NULL propuesta daría lugar a que no se establezca un mensaje de apagado, lo que puede o no dar lugar a un error notable. Tendrías más dificultades para trabajar hacia atrás en la función SignalShutdown que si la aplicación completa simplemente falleciera, produciendo un retroceso práctico o dump central apuntando directamente a SignalShutdown() .

Es un poco contrario a la intuición, pero fallar tan pronto como algo está mal tiende a hacer que tu código sea más robusto. Esto se debe a que en realidad encuentra los problemas, y también tiende a tener causas muy obvias.

Si msgSender nunca debe ser null , debe poner el cheque null solo en el constructor. Esto también es válido para cualquier otra entrada en la clase: coloque la verificación de integridad en el punto de entrada en el 'módulo': clase, función, etc.

Mi regla de oro es realizar verificaciones de integridad entre los límites del módulo, la clase en este caso. Además, una clase debe ser lo suficientemente pequeña como para que sea posible verificar rápidamente mentalmente la integridad de la vida útil de los miembros de la clase, lo que garantiza que se eviten los errores como las eliminaciones incorrectas / las asignaciones nulas. La comprobación de nulos que se realiza en el código de su publicación asume que cualquier uso no válido en realidad asigna un valor nulo al puntero, lo que no siempre es así. Dado que el "uso no válido" implica de forma inherente que no se aplica ninguna suposición sobre el código normal, no podemos asegurarnos de detectar todos los tipos de errores de puntero, por ejemplo, una eliminación no válida, un incremento, etc.

Además, si está seguro de que el argumento nunca puede ser nulo, considere usar referencias, dependiendo de su uso de la clase. De lo contrario, considere usar std::unique_ptr o std::shared_ptr en lugar de un puntero en bruto.

No, no es razonable verificar todas y cada una de las desreferencias del puntero para que el puntero sea NULL .

Las comprobaciones de puntero nulo son valiosas en los argumentos de la función (incluidos los argumentos del constructor) para garantizar que se cumplan las condiciones previas o para tomar las medidas apropiadas si no se proporciona un parámetro opcional, y son valiosas para verificar el invariante de una clase después de haber expuesto la clase internos Pero si la única razón por la que un puntero se convirtió en NULO es la existencia de un error, entonces no tiene sentido verificarlo. Ese error podría fácilmente haber establecido el puntero en otro valor no válido.

Si me enfrentara a una situación como la tuya, te haría dos preguntas:

• ¿Por qué el error de ese programador junior no se detectó antes del lanzamiento? Por ejemplo, en una revisión de código o en la fase de prueba? Llevar al mercado un dispositivo electrónico de consumo defectuoso puede ser tan costoso (si toma en cuenta la participación de mercado y la buena voluntad) como liberar un dispositivo defectuoso utilizado en una aplicación de seguridad crítica, por lo que espero que la empresa tome en serio las pruebas y otras actividades de control de calidad.
• Si falla la comprobación de nulos, ¿qué gestión de errores espera que aplique o podría simplemente escribir assert(msgSender_) en lugar de la comprobación de nulos? Si acaba de poner el registro nulo, es posible que haya evitado un bloqueo, pero podría haber creado una situación peor porque el software continúa con la premisa de que se realizó una operación, mientras que en realidad esa operación se omitió. Esto podría hacer que otras partes del software se vuelvan inestables.

Este ejemplo parece tener más que ver con la vida útil del objeto que si un parámetro de entrada es nulo o no. Como mencionó que PowerManager debe siempre tener un IMsgSender válido, pasar el argumento por puntero (lo que permite la posibilidad de un puntero nulo) me parece un defecto de diseño ††.

En situaciones como esta, preferiría cambiar la interfaz para que los requisitos de la persona que llama se cumplan con el idioma:

PowerManager::PowerManager(const IMsgSender& msgSender)
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    msgSender_->sendMsg("shutdown()");
}

Al reescribirlo de esta manera, se dice que PowerManager necesita mantener una referencia a IMsgSender durante toda su vida. Esto, a su vez, también establece un requisito implícito de que IMsgSender debe vivir más que PowerManager , y niega la necesidad de cualquier comprobación o afirmación de puntero nulo dentro de PowerManager .

También puede escribir lo mismo usando un puntero inteligente (mediante boost o c ++ 11), para forzar explícitamente a IMsgSender a vivir más 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()");
}

Se prefiere este método si es posible que no se pueda garantizar que la vida útil de IMsgSender sea más larga que la de PowerManager (es decir, x = new IMsgSender(); p = new PowerManager(*x); ).

† Con respecto a los punteros: la comprobación nula desenfrenada dificulta la lectura del código y no mejora la estabilidad (mejora la apariencia de la estabilidad, que es mucho peor).

En algún lugar, alguien obtuvo una dirección para que la memoria contenga el IMsgSender . Es responsabilidad de esa función asegurarse de que la asignación se haya realizado correctamente (verificando los valores de retorno de la biblioteca o manejando adecuadamente las excepciones std::bad_alloc ), para no pasar los punteros no válidos.

Dado que PowerManager no posee el IMsgSender (solo lo toma prestado por un tiempo), no es responsable de asignar o destruir esa memoria. Esta es otra razón por la que prefiero la referencia.

†† Como eres nuevo en este trabajo, espero que estés pirateando el código existente. Entonces, por defecto de diseño, quiero decir que el defecto está en el código con el que estás trabajando. Por lo tanto, las personas que están marcando su código porque no está comprobando los punteros nulos realmente se están marcando para escribir código que requiere punteros :)

Al igual que las excepciones, las condiciones de protección solo son útiles si sabe qué hacer para recuperarse del error o si desea dar un mensaje de excepción más significativo.

Tragar un error (ya sea que se detecte como una excepción o como un control de vigilancia), solo es lo correcto cuando el error no importa. El lugar más común en el que puedo ver los errores que se tragan es el código de registro de errores: no desea bloquear una aplicación porque no pudo registrar un mensaje de estado.

Cada vez que se llama a una función, y no es un comportamiento opcional, debería fallar en voz alta, no en silencio.

Editar: al pensar en la historia de su programador junior, parece que lo que sucedió fue que un miembro privado se estableció en nulo cuando eso nunca se suponía que debía suceder. Tuvieron un problema con la escritura inapropiada y están intentando solucionarlo validando al leer. Esto es al revés. En el momento en que usted lo identifica, el error ya ha ocurrido. La solución exigible de revisión de código / compilador para eso no son condiciones de protección, sino que son buscadores y definidores o miembros const.

Como han señalado otros, esto depende de si o no msgSender puede ser legítimamente NULL . Lo siguiente asume que nunca debe ser NULL.

void PowerManager::SignalShutdown()
{
    if (!msgSender_)
    {
       throw SignalException("Shut down failed because message sender is not set.");
    }

    msgSender_->sendMsg("shutdown()");
}

El "arreglo" propuesto por los demás miembros de su equipo viola el principio de Programas Dead Dead Tell No Lies . Los insectos son muy difíciles de encontrar tal como son. Un método que cambia su comportamiento de forma silenciosa en función de un problema anterior, no solo dificulta la búsqueda del primer error, sino que también agrega un segundo error.

El junior causó estragos al no comprobar si hay un nulo. ¿Qué sucede si este fragmento de código causa estragos al continuar ejecutándose en un estado indefinido (el dispositivo está encendido pero el programa "piensa" que está apagado)? Quizás otra parte del programa haga algo que solo sea seguro cuando el dispositivo está apagado.

Cualquiera de estos enfoques evitará fallas silenciosas:

1. Use las afirmaciones sugeridas por esta respuesta , pero asegúrese de que estén activadas en el código de producción . Esto, por supuesto, podría causar problemas si se escribieran otras afirmaciones con el supuesto de que estarían fuera de producción.

2. Lanzar una excepción si es nula.

Estoy de acuerdo con atrapar el nulo en el constructor. Además, si el miembro se declara en el encabezado como:

IMsgSender* const msgSender_;

Entonces, el puntero no se puede cambiar después de la inicialización, por lo que si estuvo bien en la construcción, estará bien durante toda la vida útil del objeto que lo contiene. (El objeto apuntado a no será constante)

Esto es completamente Peligroso!

Trabajé bajo un desarrollador senior en una base de código en C con los "estándares" más malos que presionaron para obtener lo mismo, para verificar ciegamente todos los punteros en busca de valores nulos. El desarrollador terminaría haciendo cosas como esta:

// Pre: vertex should never be null.
void transform_vertex(Vertex* vertex, ...)
{
    // Inserted by my "wise" co-worker.
    if (!vertex)
        return;
    ...
}

Una vez traté de eliminar tal comprobación de la condición previa una vez en dicha función y reemplazarla con un assert para ver qué sucedería.

Para mi horror, encontré miles de líneas de código en la base de código que pasaban nulos a esta función, pero donde los desarrolladores, probablemente confundidos, trabajaron y simplemente agregaron más código hasta que las cosas funcionaron.

Para mi mayor horror, encontré que este problema prevalecía en todo tipo de lugares en el código base que verificaba nulos. La base de código había crecido durante décadas para confiar en estas verificaciones con el fin de poder violar en silencio incluso las condiciones previas más explícitamente documentadas. Al eliminar estos controles mortales a favor de asserts , se revelarían todos los errores humanos lógicos durante décadas en el código base, y nos ahogaríamos en ellos.

Solo se necesitaron dos líneas de código aparentemente inocentes como este + tiempo y un equipo para terminar ocultando mil errores acumulados.

Estos son los tipos de prácticas que hacen que los errores dependan de otros errores para que el software funcione. Es un escenario de pesadilla . También hace que cada error lógico relacionado con la violación de tales condiciones previas muestre misteriosamente un millón de líneas de código del sitio real en el que ocurrió el error, ya que todas estas comprobaciones nulas simplemente ocultan el error y lo ocultan hasta que alcanzamos un lugar que olvidó. para ocultar el error.

Para simplemente verificar los nulos a ciegas en todos los lugares donde un puntero nulo infringe una condición previa, para mí, es una absoluta locura, a menos que su software sea tan crítico en cuanto a fallos de afirmación y bloqueos de producción que el potencial de este escenario sea preferible.

  

¿Es razonable que un estándar de codificación requiera que cada   el puntero de referencia en una función se debe comprobar primero para NULL, incluso   miembros de datos privados?

Por lo que diría, absolutamente no. Ni siquiera es "seguro". Puede muy bien ser lo contrario y enmascarar todo tipo de errores a lo largo de su base de código que, a lo largo de los años, puede llevar a los escenarios más terribles.

assert es el camino a seguir aquí. No debe permitirse que las violaciones de las condiciones previas pasen inadvertidas, de lo contrario, la ley de Murphy puede fácilmente comenzar.

Objective-C , por ejemplo, trata cada llamada de método en un objeto nil como una no-op que evalúa a un valor cero-ish. Hay algunas ventajas de esa decisión de diseño en Objective-C, por las razones sugeridas en su pregunta. El concepto teórico de la protección nula de todas las llamadas de métodos tiene algún mérito si se divulga de manera adecuada y se aplica de manera consistente.

Dicho esto, el código vive en un ecosistema, no en un vacío. El comportamiento protegido nulo sería no idiomático y sorprendente en C ++ y, por lo tanto, debería considerarse dañino. En resumen, nadie escribe el código C ++ de esa manera, así que no lo haga. Sin embargo, como ejemplo contrario, tenga en cuenta que llamar a free() o delete en un NULL en C y C ++ está garantizado como no operado.

En su ejemplo, probablemente valdría la pena colocar una aserción en el constructor de que msgSender no es nulo. Si el constructor llamó a un método en msgSender de inmediato, entonces no sería necesaria tal afirmación, ya que de todos modos se estrellaría allí. Sin embargo, como se trata simplemente de almacenar msgSender para uso futuro, no sería obvio si observamos un seguimiento de pila de SignalShutdown() cómo el valor llegó a ser NULL , por lo que una afirmación en el constructor haría la depuración significativamente más fácil.

Aún mejor, el constructor debería aceptar una referencia const IMsgSender& , que posiblemente no puede ser NULL .

La razón por la que se le pide que evite las referencias erróneas es para asegurarse de que su código sea sólido. Ejemplos de programadores junior hace mucho tiempo son solo ejemplos. Cualquiera puede romper el código por accidente y provocar una anulación nula, especialmente para los globales y los globales de clase. En C y C ++ es incluso más posible accidentalmente, con la capacidad de administración de memoria directa. Es posible que se sorprenda, pero este tipo de cosas sucede muy a menudo. Incluso por desarrolladores muy experimentados, muy experimentados y muy avanzados.

No es necesario que anule la comprobación de todo, pero sí necesita protegerse contra las desreferencias que tienen una probabilidad decente de ser nulo. Esto es comúnmente cuando se asignan, se usan y se eliminan de referencias en diferentes funciones. Es posible que una de las otras funciones se modifique y rompa su función. También es posible que una de las otras funciones esté fuera de servicio (como si tuviera un desasignador que se puede llamar por separado desde el destructor).

Prefiero el enfoque que sus compañeros de trabajo le están diciendo en combinación con el uso de assert. Bloquee en el entorno de prueba, por lo que es más obvio que hay un problema para solucionar y fallar con gracia en la producción.

También deberías usar una herramienta de corrección de código robusta como coverity o fortify. Y deberías estar dirigiendo todas las advertencias del compilador.

Editar: como han mencionado otros, fallar en silencio, como en varios de los ejemplos de código, generalmente también es lo incorrecto. Si su función no puede recuperarse del valor que es nulo, debería devolver un error (o lanzar una excepción) a su interlocutor. El interlocutor es responsable de corregir su orden de llamada, recuperar o devolver un error (o lanzar una excepción) a su interlocutor, y así sucesivamente. Finalmente, una función puede recuperarse y seguir adelante con gracia, recuperarse y fallar con gracia (como una base de datos que falla en una transacción debido a un error interno para un usuario pero no está saliendo), o la función determina que el estado de la aplicación está dañado e irrecuperable y la aplicación se cierra.

El uso de un puntero en lugar de una referencia me dirá que msgSender es solo una opción y aquí la comprobación nula tener razón. El fragmento de código es demasiado lento para decidir eso. Tal vez hay otros elementos en PowerManager que son valiosos (o verificables) ...

Al elegir entre puntero y referencia, soplo a fondo ambas opciones. Si tengo que usar un puntero para un miembro (incluso para miembros privados), tengo que aceptar el procedimiento if (x) cada vez que lo desreferenciado.

Depende de lo que quieras que suceda.

Usted escribió que está trabajando en "un dispositivo de electrónica de consumo". Si, de alguna manera, se introduce un error configurando msgSender_ a NULL , ¿quieres

• el dispositivo para continuar, omitiendo SignalShutdown pero continuando el resto de su operación, o
• ¿el dispositivo se bloquea, obligando al usuario a reiniciarlo?

Dependiendo del impacto de la señal de apagado que no se envía, la opción 1 podría ser una opción viable. Si el usuario puede seguir escuchando su música, pero la pantalla aún muestra el título de la pista anterior, podría ser preferible a un bloqueo completo del dispositivo.

Por supuesto, si elige la opción 1, un assert (según lo recomendado por otros) es vital para reducir la probabilidad de que un error de este tipo pase inadvertido durante el desarrollo. El if null guard está justo ahí para mitigar fallos en el uso de producción.

Personalmente, también prefiero el enfoque de "bloqueo anticipado" para las compilaciones de producción, pero estoy desarrollando software empresarial que se puede corregir y actualizar fácilmente en caso de un error. Para los dispositivos electrónicos de consumo, esto podría no ser tan fácil.