Si las funciones tienen que hacer comprobaciones nulas antes de realizar el comportamiento deseado, ¿es este un mal diseño?

67

Entonces, no sé si este es un diseño de código bueno o malo, así que pensé que sería mejor preguntar.

Con frecuencia, creo métodos que procesan datos que involucran clases y, a menudo, realizo muchas comprobaciones en los métodos para asegurarme de que no obtengo referencias nulas u otros errores de antemano.

Para un ejemplo muy 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);
}

Entonces, como puede ver, estoy buscando nulos cada vez. ¿Pero el método no debería tener esta verificación?

Por ejemplo, si el código externo limpia los datos de antemano para que los métodos no necesiten validarse como se muestra a continuación:

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

En resumen, si los métodos deben ser "validación de datos" y luego se procesan en los datos, o debe garantizarse antes de llamar al método, y si no lo valida antes de llamar al método, debería arrojar un error (o detectar el error)?

WDUK
fuente
77
¿Qué sucede cuando alguien no puede hacer la verificación nula y SetName lanza una excepción de todos modos?
Robert Harvey
55
¿Qué sucede si realmente quiero que SomeEntity sea Null? Usted decide lo que quiere, someEntity puede ser nulo o no, y luego escribe su código en consecuencia. Si desea que nunca sea Nulo, puede reemplazar los cheques con afirmaciones.
gnasher729
55
Puede ver los límites de Gary Bernhardt hablar sobre hacer una división clara entre la desinfección de datos (es decir, la verificación de nulos, etc.) en un caparazón externo, y tener un sistema central interno que no tenga que preocuparse por esas cosas, y simplemente haga su trabajo.
mgarciaisaia
1
Con C # 8, posiblemente nunca deba preocuparse por las excepciones de referencia nula con la configuración correcta activada: blogs.msdn.microsoft.com/dotnet/2017/11/15/…
JᴀʏMᴇᴇ
3
Esta es una de las razones por las cuales el equipo de lenguaje C # quiere introducir tipos de referencia anulables (y no anulables) -> github.com/dotnet/csharplang/issues/36
Mafii

Respuestas:

168

El problema con su ejemplo básico no es la verificación nula, es la falla silenciosa .

Los errores de puntero / referencia nulos, la mayoría de las veces, son errores de programador. Los errores del programador a menudo se resuelven mejor al fallar de inmediato y en voz alta. Tiene tres formas generales de abordar el problema en esta situación:

  1. No se moleste en verificar, y solo deje que el tiempo de ejecución arroje la excepción de puntero nulo.
  2. Verifica y lanza una excepción con un mensaje mejor que el mensaje básico de NPE.

Una tercera solución es más trabajo pero es mucho más robusta:

  1. Estructura tu clase de tal manera que sea prácticamente imposible _someEntityestar en un estado no válido. Una forma de hacerlo es deshacerse de él AssignEntityy requerirlo como parámetro para la creación de instancias. Otras técnicas de inyección de dependencia también pueden ser útiles.

En algunas situaciones, tiene sentido verificar la validez de todos los argumentos de la función antes de cualquier trabajo que esté haciendo, y en otras tiene sentido decirle a la persona que llama que es responsable de garantizar que sus entradas sean válidas y no verificar. El extremo del espectro en el que se encuentre dependerá de su dominio problemático. La tercera opción tiene un beneficio significativo, ya que puede, hasta cierto punto, hacer que el compilador haga cumplir que la persona que llama hace todo correctamente.

Si la tercera opción no es una opción, entonces, en mi opinión, realmente no importa siempre que no falle silenciosamente. Si no verificar nulo hace que el programa explote instantáneamente, está bien, pero si en cambio corrompe silenciosamente los datos para causar problemas en el futuro, entonces es mejor verificarlos y tratarlos de inmediato.

whatsisname
fuente
10
@aroth: cualquier diseño de cualquier cosa, incluido el software, tendrá limitaciones. El acto mismo de diseñar es seleccionar a dónde van esas restricciones, y no es mi responsabilidad ni debería ser que acepto ninguna aportación y se espera que haga algo útil con ella. Sé si nulles incorrecto o no válido porque en mi hipotética biblioteca he definido los tipos de datos y he definido qué es válido y qué no. Lo llaman "suposiciones forzosas", pero adivinen qué: eso es lo que hace cada biblioteca de software existente. Hay momentos en que nulo es un valor válido, pero eso no es "siempre".
cuál es el
44
"que su código está roto porque no se maneja nullcorrectamente" - Esto es un malentendido de lo que significa un manejo adecuado. Para la mayoría de los programadores de Java, significa lanzar una excepción para cualquier entrada no válida. Utilizando @ParametersAreNonnullByDefault, significa también para todos null, a menos que el argumento esté marcado como @Nullable. Hacer cualquier otra cosa significa alargar el camino hasta que finalmente sople en otro lugar y, por lo tanto, también alargar el tiempo perdido en la depuración. Bueno, al ignorar los problemas, puede lograr que nunca explote, pero el comportamiento incorrecto está bastante garantizado.
maaartinus
44
@aroth Querido Señor, odiaría trabajar con cualquier código que escribas. Las explosiones son infinitamente mejores que hacer algo sin sentido, que es la única otra opción para la entrada no válida. Las excepciones me obligan, la persona que llama, a decidir qué es lo correcto en esos casos, cuando el método no puede adivinar lo que pretendía. Las excepciones marcadas y la extensión innecesaria Exceptionson debates completamente ajenos. (Estoy de acuerdo en que ambos son problemáticos). Para este ejemplo específico, nullobviamente no tiene sentido aquí si el propósito de la clase es invocar métodos en el objeto.
jpmc26
3
"su código no debería estar forzando suposiciones al mundo de la persona que llama" - Es imposible no forzar suposiciones a la persona que llama. Es por eso que debemos hacer que estos supuestos sean lo más explícitos posible.
Diogo Kollross el
3
@aroth su código está roto porque pasa mi código como nulo cuando debería pasar algo que tiene Nombre como propiedad. Cualquier comportamiento que no sea la explosión es algún tipo de versión extraña de back-world del tipo dinámico en mi humilde opinión
mbrig
56

Como _someEntityse puede modificar en cualquier etapa, entonces tiene sentido probarlo cada vez que SetNamese llama. Después de todo, podría haber cambiado desde la última vez que se llamó a ese método. Pero tenga en cuenta que el código SetNameno es seguro para subprocesos, por lo que puede realizar esa verificación en un subproceso, _someEntityestablecerlo en nullotro y luego el código se abrirá de NullReferenceExceptiontodos modos.

Por lo tanto, un enfoque para este problema es ponerse aún más a la defensiva y realizar una de las siguientes acciones:

  1. hacer una copia local _someEntityy hacer referencia a esa copia a través del método,
  2. use un candado _someEntitypara asegurarse de que no cambie a medida que se ejecuta el método,
  3. use ay try/catchrealice la misma acción en cualquiera NullReferenceExceptionde los que ocurran dentro del método como lo haría en la verificación nula inicial (en este caso, una nopacción).

Pero lo que realmente debe hacer es detenerse y hacerse la pregunta: ¿realmente necesita permitir _someEntityque se sobrescriba? ¿Por qué no configurarlo una vez, a través del constructor? De esa manera, solo necesita hacer esa comprobación nula una 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);
}

Si es posible, este es el enfoque que recomendaría adoptar en tales situaciones. Si no puede tener en cuenta la seguridad de los hilos al elegir cómo manejar posibles nulos.

Como comentario adicional, siento que su código es extraño y podría mejorarse. Todo:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

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

se puede reemplazar con:

public Entity SomeEntity { get; set; }

Que tiene la misma funcionalidad, salvo por poder establecer el campo a través del configurador de propiedades, en lugar de un método con un nombre diferente.

David Arno
fuente
55
¿Por qué responde esto a la pregunta? La pregunta aborda la comprobación nula y esto es más o menos una revisión de código.
IEatBagels
17
@TopinFrassi explica que el enfoque actual de verificación nula no es seguro para subprocesos. Luego toca otras técnicas de programación defensiva para evitar eso. Luego concluye con la sugerencia de usar la inmutabilidad para evitar la necesidad de tener esa programación defensiva en primer lugar. Y como una ventaja adicional, también ofrece consejos sobre cómo estructurar mejor el código.
David Arno
23

¿Pero el método no debería tener esta verificación?

Esta es tu elección.

Al crear un publicmétodo, está ofreciendo al público la oportunidad de llamarlo. Esto siempre viene junto con un contrato implícito sobre cómo llamar a ese método y qué esperar al hacerlo. Este contrato puede (o no) incluir " sí, uhhm, si lo pasa nullcomo un valor de parámetro, explotará en su cara ". Otras partes de ese contrato podrían ser " oh, por cierto: cada vez que dos hilos ejecutan este método al mismo tiempo, un gatito muere ".

El tipo de contrato que desea diseñar depende de usted. Lo importante es

  • crear una API que sea útil y consistente y

  • documentarlo bien, especialmente para esos casos extremos.

¿Cuáles son los casos probables de cómo se llama su método?

nulo
fuente
Bueno, la seguridad de subprocesos es la excepción, no la regla, cuando hay acceso concurrente. Por lo tanto, el uso del estado oculto / global está documentado, así como cualquier caso, la concurrencia es segura de todos modos.
Deduplicador
14

Las otras respuestas son buenas; Me gustaría ampliarlos haciendo algunos comentarios sobre los modificadores de accesibilidad. Primero, qué hacer si nullnunca es válido:

  • Los métodos públicos y protegidos son aquellos en los que no controla a la persona que llama. Deberían tirar cuando pasen nulo. De esa forma entrenas a tus llamantes para que nunca te pasen un valor nulo, porque les gustaría que su programa no se bloquee.

  • internas y privadas métodos son aquellos en los que no controlas la persona que llama. Deben afirmarse cuando pasan nulo; probablemente se estrellarán más tarde, pero al menos primero obtuviste la afirmación y la oportunidad de entrar en el depurador. Una vez más, desea capacitar a las personas que llaman para que lo llamen correctamente haciendo que duela cuando no lo hacen.

Ahora, ¿qué haces si podría ser válido para que se pase un valor nulo? Evitaría esta situación con el patrón de objeto nulo . Es decir: haga una instancia especial del tipo y requiera que se use cada vez que se necesite un objeto no válido . Ahora está de vuelta en el agradable mundo de lanzar / afirmar sobre cada uso de nulo.

Por ejemplo, no quieres estar en esta situación:

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

y en el sitio de la llamada:

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

Debido a que (1) está entrenando a las personas que llaman que nulo es un buen valor, y (2) no está claro cuáles son las semánticas de ese valor. Más bien, haz esto:

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 ...

Y ahora el sitio de la llamada se ve así:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

y ahora el lector del código sabe lo que significa.

Eric Lippert
fuente
Mejor aún, no tenga una cadena de ifs para los objetos nulos, sino que use el polimorfismo para que se comporten correctamente.
Konrad Rudolph el
@KonradRudolph: De hecho, a menudo es una buena técnica. Me gusta usar eso para estructuras de datos, donde hago un EmptyQueuetipo que arroja cuando se retira, y así sucesivamente.
Eric Lippert
@EricLippert - Perdóname porque todavía estoy aprendiendo aquí, pero supongamos que si la Personclase fuera inmutable y no contuviera un constructor de argumento cero, ¿cómo construirías tu ApproverNotRequiredo tus ApproverUnknowninstancias? En otras palabras, ¿qué valores pasarías al constructor?
2
Me encanta toparme con tus respuestas / comentarios a través de SE, siempre son perspicaces. Tan pronto como vi la afirmación de métodos internos / privados, me desplacé hacia abajo esperando que fuera una respuesta de Eric Lippert, no me decepcionó :)
bob esponja
44
Ustedes están pensando demasiado en el ejemplo específico que les di. Tenía la intención de transmitir rápidamente la idea del patrón de objeto nulo. No pretendía ser un ejemplo de buen diseño en un sistema de automatización de papeles y cheques de pago. En un sistema real, hacer que el "aprobador" tenga el tipo "persona" es probablemente una mala idea porque no coincide con el proceso comercial; Puede que no haya un aprobador, puede que necesite varios, y así sucesivamente. Como a menudo noto al responder preguntas en ese dominio, lo que realmente queremos es un objeto de política . No te obsesiones con el ejemplo.
Eric Lippert
8

Las otras respuestas señalan que su código se puede limpiar para que no necesite una verificación nula donde lo tenga, sin embargo, para obtener una respuesta general sobre lo que puede ser una verificación nula útil para considerar el siguiente código de muestra:

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;
    }
}

Esto le da una ventaja para el mantenimiento. Si alguna vez ocurre un defecto en su código donde algo pasa un objeto nulo al método, obtendrá información útil del método sobre qué parámetro era nulo. Sin las comprobaciones, obtendrá una NullReferenceException en la línea:

a.Something = b.Something;

Y (dado que la propiedad Something es un tipo de valor) a o b podría ser nulo y no tendrá forma de saber cuál de la traza de la pila. Con las comprobaciones, sabrá exactamente qué objeto era nulo, lo que puede ser enormemente útil al intentar eliminar un defecto.

Me gustaría señalar que el patrón en su código original:

if (x == null) return;

Puede tener sus usos en ciertas circunstancias, también puede (posiblemente más a menudo) darle una muy buena forma de enmascarar los problemas subyacentes en su base de código.

Arrozal
fuente
4

No, para nada, pero depende.

Solo realiza una verificación de referencia nula si desea reaccionar al respecto.

Si realiza una verificación de referencia nula y arroja una excepción marcada , obliga al usuario del método a reaccionar y le permite recuperarse. El programa aún funciona como se esperaba.

Si no realiza una verificación de referencia nula (o arroja una excepción no verificada), podría arrojar una verificación no verificada NullReferenceException, que generalmente no es manejada por el usuario del método e incluso podría cerrar la aplicación. Esto significa que la función es obviamente completamente incomprendida y, por lo tanto, la aplicación es defectuosa.

Herr Derb
fuente
4

Algo así como una extensión de la respuesta de @ null, pero en mi experiencia, hacer comprobaciones nulas sin importar qué.

Una buena programación defensiva es la actitud de codificar la rutina actual de forma aislada, bajo el supuesto de que todo el resto del programa está tratando de bloquearla. Cuando escribe código de misión crítica donde el fracaso no es una opción, esta es la única forma de hacerlo.

Ahora, cómo manejas realmente un nullvalor, eso depende en gran medida de tu caso de uso.

Si nulles un valor aceptable, por ejemplo, cualquiera de los parámetros segundo a quinto de la rutina MSVC _splitpath (), entonces el comportamiento correcto es tratarlo en silencio.

Si nullno debería suceder nunca al llamar a la rutina en cuestión, es decir, en el caso del OP, el contrato de API requiere AssignEntity()que se haya llamado con una validez y Entityluego lanzar una excepción, o de lo contrario no garantizará que haga mucho ruido en el proceso.

Nunca se preocupe por el costo de un cheque nulo, son muy baratos si suceden, y con los compiladores modernos, el análisis de código estático puede eliminarlos por completo si el compilador determina que no va a suceder.

dgnuff
fuente
O evítelo por completo (48 minutos 29 segundos: "Nunca use ni permita en ningún lugar cerca de su programa un puntero nulo" ).
Peter Mortensen