Lanza una excepción o deja que el código falle

Me pregunto si hay ventajas y desventajas en contra de este estilo:

private void LoadMaterial(string name)
{
    if (_Materials.ContainsKey(name))
    {
        throw new ArgumentException("The material named " + name + " has already been loaded.");
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );
}

Ese método debe, para cada uno name, ejecutarse solo una vez. _Materials.Add()lanzará una excepción si se llamará varias veces por lo mismo name. ¿Es mi guardia, como resultado, completamente redundante, o hay algunos beneficios menos obvios?

Eso es C #, Unidad, si alguien está interesado.

El beneficio es que su excepción "personalizada" tiene un mensaje de error que es significativo para cualquiera que llame a esta función sin saber cómo se implementa (¡que en el futuro podría ser usted!).

De acuerdo, en este caso probablemente podrían adivinar qué significaba la excepción "estándar", pero aún está dejando en claro que violaron su contrato, en lugar de tropezar con algún error extraño en su código.

Estoy de acuerdo con la respuesta de Ixrec . Sin embargo, es posible que desee considerar una tercera alternativa: hacer que la función sea idempotente. En otras palabras, regrese temprano en lugar de lanzar un ArgumentException. Esto a menudo es preferible si de lo contrario se vería obligado a verificar si ya se ha cargado antes de llamar LoadMaterialcada vez. Cuantas menos condiciones previas tenga, menos carga cognitiva tendrá el programador.

Sería preferible lanzar una excepción si realmente es un error del programador, donde debería ser obvio y conocido en el momento de la compilación si el material ya se ha cargado, sin tener que verificar en tiempo de ejecución antes de llamar.

La pregunta fundamental que debe hacerse es: ¿Cuál quiere que sea la interfaz para su función? En particular, ¿es la condición _Materials.ContainsKey(name)una condición previa de la función?

Si es no una condición previa, la función debe proporcionar un resultado bien definido para todos los posibles valles de name. En este caso, la excepción lanzada si nameno forma parte de _Materialsla interfaz de la función. Eso significa que debe ser parte de la documentación de la interfaz y si alguna vez decide cambiar esa excepción en el futuro, será un cambio de interfaz de última hora .

La pregunta más interesante es qué sucede si es una condición previa. En ese caso, la condición previa se convierte en parte de la interfaz de la función, pero la forma en que se comporta una función cuando se viola esta condición no es necesariamente parte de la interfaz.

En este caso, el enfoque que publicó, verificando las infracciones de precondición e informando el error, es lo que se conoce como programación defensiva . La programación defensiva es buena porque notifica al usuario temprano cuando cometió un error y llamó a la función con un argumento falso. Es malo, ya que aumenta significativamente la carga de mantenimiento, ya que el código de usuario puede depender de que la función maneje las infracciones de precondición de cierta manera. En particular, si encuentra que la verificación del tiempo de ejecución se convierte en un cuello de botella en el rendimiento en el futuro (poco probable para un caso tan simple, pero bastante común para condiciones previas más complejas), es posible que ya no pueda eliminarlo.

Resulta que esas desventajas pueden ser muy significativas, lo que le dio a la programación defensiva una mala reputación en ciertos círculos. Sin embargo, el objetivo inicial sigue siendo válido: queremos que los usuarios de nuestra función se den cuenta temprano cuando cometieron un error.

Por lo tanto, muchos desarrolladores hoy en día proponen un enfoque ligeramente diferente para este tipo de problemas. En lugar de lanzar una excepción, utilizan un mecanismo de aseveración para verificar las condiciones previas. Es decir, las condiciones previas pueden verificarse en las compilaciones de depuración para ayudar a los usuarios a detectar errores desde el principio, pero no son parte de la interfaz de funciones. La diferencia puede parecer sutil a primera vista, pero puede hacer una gran diferencia en la práctica.

Técnicamente, llamar a una función con condiciones previas violadas es un comportamiento indefinido. Pero la implementación podría decidir detectar esos casos y notificar al usuario de inmediato si eso sucede. Desafortunadamente, las excepciones no son una buena herramienta para implementar esto, ya que el código de usuario puede reaccionar ante ellas y, por lo tanto, podría comenzar a depender de su presencia.

Para obtener una explicación detallada de los problemas con el enfoque defensivo clásico, así como una posible implementación para verificaciones de precondición de estilo asertivo, consulte la charla de John Lakos Programación defensiva realizada directamente desde CppCon 2014 ( Diapositivas , video ).

Ya hay algunas respuestas aquí, pero aquí hay una respuesta que tiene en cuenta Unity3D (la respuesta es muy específica para Unity3D, haría la mayoría de esto de manera muy diferente en la mayoría de los contextos):

En general, Unity3D no usa excepciones tradicionalmente. Si lanza una excepción en Unity3D, no se parece en nada a su aplicación .NET promedio, es decir, no detendrá el programa, la mayoría de las veces puede configurar el editor para que haga una pausa. Simplemente se registrará. Esto puede poner fácilmente el juego en un estado no válido y crear un efecto en cascada que hace que los errores sean difíciles de rastrear. Entonces, diría que en el caso de Unity, dejar Adduna excepción es una opción especialmente indeseable.

Pero examinar la velocidad de las excepciones no es un caso de optimización prematura en algunos casos debido a cómo Mono funciona en Unity en algunas plataformas. De hecho, Unity3D en iOS admite algunas optimizaciones de script avanzadas, y las excepciones deshabilitadas * son un efecto secundario de una de ellas. Esto es realmente algo a considerar porque esas optimizaciones no han demostrado ser muy valiosas para muchos usuarios, mostrando un caso realista para considerar limitar el uso de excepciones en Unity3D. (* excepciones gestionadas del motor, no su código)

Diría que en Unity es posible que desee adoptar un enfoque más especializado. Irónicamente, una respuesta muy rechazada al momento de escribir esto , muestra una forma en que podría implementar algo como esto específicamente en el contexto de Unity3D (en otros lugares algo como esto es realmente inaceptable, e incluso en Unity es bastante poco elegante).

Otro enfoque que consideraría en realidad no es indicar un error en lo que respecta a la persona que llama, sino más bien usar las Debug.LogXXfunciones. De esa forma, obtienes el mismo comportamiento que lanzar una excepción no controlada (debido a cómo Unity3D los maneja) sin arriesgarte a poner algo en un estado extraño en el futuro. También considere si esto realmente es un error (¿Tratar de cargar el mismo material dos veces necesariamente es un error en su caso? ¿O podría ser un caso en el que Debug.LogWarningsea ​​más aplicable?).

Y en lo que respecta al uso de cosas como Debug.LogXXfunciones en lugar de excepciones, aún debe considerar qué sucede cuando se produce una excepción de algo que devuelve un valor (como GetMaterial). Tiendo a abordar esto pasando nulo junto con el registro del error ( nuevamente, solo en Unity ). Luego uso verificaciones nulas en mis MonoBehaviors para asegurar que cualquier dependencia como un material no sea un valor nulo, y deshabilito el MonoBehavior si lo es. Un ejemplo de un comportamiento simple que requiere algunas dependencias es algo como esto:

    public void Awake()
    {
        _inputParameters = GetComponent<VehicleInputParameters>();
        _rigidbody = GetComponent<Rigidbody>();
        _rigidbodyTransform = _rigidbody.transform;
        _raycastStrategySelector = GetComponent<RaycastStrategySelectionBehavior>();

        _trackParameters =
            SceneManager.InstanceOf.CurrentSceneData.GetValue<TrackParameters>();

        this.DisableIfNull(() => _rigidbody);
        this.DisableIfNull(() => _raycastStrategySelector);
        this.DisableIfNull(() => _inputParameters);
        this.DisableIfNull(() => _trackParameters);
    }

SceneData.GetValue<>es similar a su ejemplo en que llama a una función en un diccionario que arroja una excepción. Pero en lugar de lanzar una excepción que usa, Debug.LogErrorque proporciona un seguimiento de la pila como lo haría una excepción normal y devuelve nulo. Las comprobaciones que siguen * deshabilitarán el comportamiento en lugar de permitir que continúe existiendo en un estado no válido.

* los cheques se ven así debido a un pequeño ayudante que uso que imprime un mensaje formateado cuando deshabilita el objeto del juego **. Verificaciones nulas simples con iftrabajo aquí (** las verificaciones de ayuda solo se compilan en compilaciones de depuración (como afirmaciones). Usar lambdas y expresiones como esas en Unity puede afectar el rendimiento)

Me gustan las dos respuestas principales, pero quiero sugerir que los nombres de sus funciones podrían mejorarse. Estoy acostumbrado a Java, así que YMMV, pero un método "agregar" no debería, IMO, no lanzar una excepción si el elemento ya está allí. Debería agregar el elemento nuevamente o, si el destino es un Conjunto, no hacer nada. Como ese no es el comportamiento de Materials.Add, se debe cambiar el nombre de TryPut o AddOnce o AddOrThrow o similar.

Del mismo modo, su LoadMaterial debe renombrarse LoadIfAbsent o Put o TryLoad o LoadOrThrow (dependiendo de si usa la respuesta # 1 o # 2) o algo así.

Siga las convenciones de nomenclatura de C # Unity, sean cuales sean.

Esto será especialmente útil si tiene otras funciones AddFoo y LoadBar donde se puede cargar lo mismo dos veces. Sin nombres claros, los desarrolladores se frustrarán.

Todas las respuestas agregan ideas valiosas, me gustaría combinarlas:

Decida la semántica prevista y esperada de la LoadMaterial()operación. Al menos existen las siguientes opciones:

• Una condición previa para name∉ LoadedMaterials : →

  Cuando se viola la condición previa, el efecto de LoadMaterial()no se especifica (como en la respuesta de ComicSansMS ). Esto permite la mayor libertad en la implementación y cambios futuros de LoadMaterial(). O,

• efectos de llamar LoadMaterial(name)con name∈ LoadedMaterials están especificados; ya sea:

  • la especificación establece que se produce una excepción; o

  • la especificación establece que el resultado es idempotente (como en la respuesta de Karl Bielefeldt ),

    • posiblemente devolver un booleano "ha cambiado la colección" (según lo propuesto por User949300 y propuesto (en algunas interpretaciones) por Mrlveck .

Cuando haya decidido la semántica, debe elegir una implementación. Las siguientes opciones y consideraciones fueron propuestas:

• Lanzar una excepción personalizada (como sugerido por Ixrec ) →

  El beneficio es que su excepción "personalizada" tiene un mensaje de error que es significativo para cualquiera que llame a esta función: Ixrec y User16547

  • Para evitar el costo de verificar repetidamente name∉ LoadedMaterials , puede seguir los consejos de Marc van Leeuwen :

    ... En cambio, considere sumergirse en _Materiales. Agregue incondicionalmente, luego detecte un error potencial y en el controlador arroje uno diferente.

• Deje Dictionay.Add lanzar la excepción →

  El código de lanzamiento de excepción es redundante. - Jon Raynor

  Aunque, la mayoría de los votantes está más de acuerdo con Ixrec

  Razones adicionales para elegir esta implementación son:

  • que las personas que llaman ya pueden manejar ArgumentExceptionexcepciones, y
  • para evitar perder información de la pila.

  Pero si estas dos razones son importantes, también puede derivar su excepción personalizada ArgumentExceptiony utilizar el original como una excepción encadenada.

• Hacer LoadMaterial()idempotente como anwser por Karl Bielefeldt y upvoted con mayor frecuencia (75 veces).

  • posiblemente devolver un booleano "ha cambiado la colección" (según lo propuesto por User949300 y propuesto (en algunas interpretaciones) por Mrlveck .

  Las opciones de implementación para este comportamiento:

  • Consulte con Dictionary.ContainsKey ()

  • Siempre llame a Dictionary.Add () capturarlo ArgumentExceptioncuando la clave para insertar ya existe e ignore esa excepción. Documente que ignorar la excepción es lo que intenta hacer y por qué.

    • Cuando LoadMaterials()(casi) siempre se llama una vez por cada uno name, esto evita el costo de verificar repetidamente name∉ LoadedMaterials cf. Marc van Leeuwen . Sin embargo,
    • cuando a LoadedMaterials()menudo se le llama varias veces por lo mismo name, esto incurre en el costo (costoso) de lanzar ArgumentExceptiony desenrollar la pila.

  • Pensé que existía un TryAddmétodo análogo a TryGet () que le habría permitido evitar la costosa excepción de lanzar y apilar las llamadas fallidas a Dictionary.Add.

    Pero este TryAddmétodo parece no existir.

El código de lanzamiento de excepción es redundante.

Si llamas:

_Materiales.Agregar (nombre, Recursos.Carga (string.Format ("Materiales / {0}", nombre)) como Material

con la misma tecla arrojará un

System.ArgumentException

El mensaje será: "Ya se ha agregado un elemento con la misma clave".

La ContainsKeyverificación es redundante ya que esencialmente se logra el mismo comportamiento sin ella. El único elemento que es diferente es el mensaje real en la excepción. Si hay un beneficio real de tener un mensaje de error personalizado, entonces el código de protección tiene algún mérito.

De lo contrario, probablemente refactorizaría la guardia en este caso.

Creo que esta es más una pregunta sobre el diseño de API que una pregunta de convención de codificación.

¿Cuál es el resultado esperado (el contrato) de llamar:

LoadMaterial("wood");

Si la persona que llama puede esperar / se garantiza que después de llamar a este método, se carga el material "madera", entonces no veo ninguna razón para lanzar una excepción cuando ese material ya está cargado.

Si se produce un error al cargar el material, por ejemplo, no se pudo abrir la conexión de la base de datos o no hay material "madera" en el repositorio, entonces es correcto lanzar una excepción para notificar a la persona que llama sobre ese problema.

¿Por qué no cambiar el método para que devuelva 'verdadero' si se agrega el material y 'falso' si no fuera así?

private bool LoadMaterial(string name)
{
   if (_Materials.ContainsKey(name))

    {

        return false; //already present
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );

    return true;

}