estructura con valor predeterminado sin sentido

12

En mi sistema que con frecuencia operan con códigos de aeropuertos ( "YYZ", "LAX", "SFO", etc.), que siempre están en el mismo formato exacto (de 3 letras, representada en mayúsculas). El sistema generalmente maneja entre 25 y 50 de estos (diferentes) códigos por solicitud de API, con más de mil asignaciones en total, se pasan a través de muchas capas de nuestra aplicación y se comparan por la igualdad con bastante frecuencia.

Comenzamos simplemente pasando cadenas, lo que funcionó bien por un tiempo, pero rápidamente notamos muchos errores de programación al pasar un código incorrecto en algún lugar donde se esperaba el código de 3 dígitos. También nos encontramos con problemas en los que se suponía que debíamos hacer una comparación entre mayúsculas y minúsculas y, en cambio, no lo hicimos, lo que resultó en errores.

A partir de esto, decidí dejar de pasar cadenas y crear una Airportclase, que tiene un solo constructor que toma y valida el código del aeropuerto.

public sealed class Airport
{
    public Airport(string code)
    {
        if (code == null)
        {
            throw new ArgumentNullException(nameof(code));
        }

        if (code.Length != 3 || !char.IsLetter(code[0]) 
        || !char.IsLetter(code[1]) || !char.IsLetter(code[2]))
        {
            throw new ArgumentException(
                "Must be a 3 letter airport code.", 
                nameof(code));
        }

        Code = code.ToUpperInvariant();
    }

    public string Code { get; }

    public override string ToString()
    {
        return Code;
    }

    private bool Equals(Airport other)
    {
        return string.Equals(Code, other.Code);
    }

    public override bool Equals(object obj)
    {
        return obj is Airport airport && Equals(airport);
    }

    public override int GetHashCode()
    {
        return Code?.GetHashCode() ?? 0;
    }

    public static bool operator ==(Airport left, Airport right)
    {
        return Equals(left, right);
    }

    public static bool operator !=(Airport left, Airport right)
    {
        return !Equals(left, right);
    }
}

Esto hizo que nuestro código fuera mucho más fácil de entender y simplificamos nuestros controles de igualdad, uso de diccionario / conjunto. Ahora sabemos que si nuestros métodos aceptan una Airportinstancia que se comportará de la manera que esperamos, ha simplificado nuestras verificaciones de métodos a una verificación de referencia nula.

Sin embargo, lo que sí noté fue que la recolección de basura se ejecutaba con mucha más frecuencia, lo que rastreé hasta muchos casos de recolección Airport.

Mi solución a esto fue convertir el classa struct. Principalmente fue solo un cambio de palabra clave, con la excepción de GetHashCodey ToString:

public override string ToString()
{
    return Code ?? string.Empty;
}

public override int GetHashCode()
{
    return Code?.GetHashCode() ?? 0;
}

Para manejar el caso donde default(Airport)se usa.

Mis preguntas:

  1. ¿Crear una Airportclase o estructura era una buena solución en general, o estoy resolviendo el problema incorrecto / resolviéndolo de manera incorrecta creando el tipo? Si no es una buena solución, ¿cuál es una mejor solución?

  2. ¿Cómo debe manejar mi aplicación las instancias donde default(Airport)se usa? Un tipo de no default(Airport)tiene sentido para mi aplicación, por lo que he estado haciendo if (airport == default(Airport) { throw ... }en lugares donde obtener una instancia de Airport(y su Codepropiedad) es fundamental para la operación.

Notas: Revisé las preguntas C # / VB struct: ¿cómo evitar el caso con valores predeterminados cero, que se considera inválido para la estructura dada? , y use struct o no antes de hacer mi pregunta, sin embargo, creo que mis preguntas son lo suficientemente diferentes como para justificar su propia publicación.

Mateo
fuente
77
¿La recolección de basura tiene un impacto material en el rendimiento de su aplicación? En otras palabras, ¿importa?
Robert Harvey
De todos modos, sí, la solución de clase fue "buena". La forma en que lo sabes es que resolvió tu problema sin crear ninguno nuevo.
Robert Harvey
2
Una forma de resolver el default(Airport)problema es simplemente deshabilitando las instancias predeterminadas. Puede hacerlo escribiendo un constructor sin parámetros y lanzándolo InvalidOperationExceptiono NotImplementedExceptionen él.
Robert Harvey
3
En una nota al margen, en lugar de confirmar que su cadena de inicialización es de hecho 3 caracteres alfabéticos, ¿por qué no solo compararla con la lista finita de todos los códigos de aeropuerto (por ejemplo, github.com/datasets/airport-codes o similar)?
Dan Pichelman
2
Estoy dispuesto a apostar varias cervezas a que esta no es la raíz de un problema de rendimiento. Una computadora portátil normal puede asignar en el orden de 10 millones de objetos / segundos.
Esben Skov Pedersen

Respuestas:

6

Actualización: reescribí mi respuesta para abordar algunas suposiciones incorrectas sobre las estructuras de C #, así como el OP que nos informa en los comentarios que se están utilizando cadenas internados.


Si puede controlar los datos que ingresan a su sistema, use una clase tal como la publicó en su pregunta. Si alguien corre default(Airport)obtendrá un nullvalor de vuelta. Asegúrese de escribir su Equalsmétodo privado para devolver falso cada vez que compare objetos nulos del aeropuerto, y luego deje NullReferenceExceptionvolar el otro en el código.

Sin embargo, si está ingresando datos al sistema desde fuentes que no controla, no es necesario que bloquee todo el hilo. En este caso, una estructura es ideal porque el simple hecho default(Airport)le dará algo más que un nullpuntero. Cree un valor obvio para representar "sin valor" o el "valor predeterminado" para que tenga algo que imprimir en la pantalla o en un archivo de registro (como "---" por ejemplo). De hecho, simplemente mantendría lo codeprivado y no expondría una Codepropiedad en absoluto, solo me centraría en el comportamiento aquí.

public struct Airport
{
    private string code;

    public Airport(string code)
    {
        // Check `code` for validity, throw exceptions if not valid

        this.code = code;
    }

    public override string ToString()
    {
        return code ?? (code = "---");
    }

    // int GetHashcode()

    // bool Equals(...)

    // bool operator ==(...)

    // bool operator !=(...)

    private bool Equals(Airport other)
    {
        if (other == null)
            // Even if this method is private, guard against null pointers
            return false;

        if (ToString() == "---" || other.ToString() == "---")
            // "Default" values should never match anything, even themselves
            return false;

        // Do a case insensitive comparison to enforce logic that airport
        // codes are not case sensitive
        return string.Equals(
            ToString(),
            other.ToString(),
            StringComparison.InvariantCultureIgnoreCase);
    }
}

En el peor de los casos, la conversión default(Airport)a una cadena se imprime "---"y devuelve falso en comparación con otros códigos de aeropuerto válidos. Cualquier código de aeropuerto "predeterminado" no coincide con nada, incluidos otros códigos de aeropuerto predeterminados.

Sí, las estructuras están destinadas a ser valores asignados en la pila, y cualquier puntero a la memoria del montón básicamente niega las ventajas de rendimiento de las estructuras, pero en este caso el valor predeterminado de una estructura tiene significado y proporciona alguna resistencia adicional al resto de la bala. solicitud.

Doblaría un poco las reglas aquí, por eso.


Respuesta original (con algunos errores de hecho)

Si puede controlar los datos que ingresan a su sistema, haría lo que Robert Harvey sugirió en los comentarios: crear un constructor sin parámetros y lanzar una excepción cuando se llame. Esto evita que datos no válidos ingresen al sistema a través de default(Airport).

public Airport()
{
    throw new InvalidOperationException("...");
}

Sin embargo, si está ingresando datos al sistema desde fuentes que no controla, no es necesario que bloquee todo el hilo. En este caso, puede crear un código de aeropuerto que no sea válido, pero que parezca un error obvio. Esto implicaría crear un constructor sin parámetros y establecer Codealgo como "---":

public Airport()
{
    Code = "---";
}

Como está utilizando a stringcomo el Código, no tiene sentido utilizar una estructura. La estructura se asigna en la pila, solo para que se Codeasigne como un puntero a una cadena en la memoria del montón, por lo que no hay diferencia aquí entre la clase y la estructura.

Si cambia el código del aeropuerto a una matriz de 3 elementos de caracteres, una estructura se asignaría completamente en la pila. Incluso entonces, el volumen de datos no es tan grande como para marcar la diferencia.

Greg Burghardt
fuente
Si mi aplicación usara cadenas internas para la Codepropiedad, ¿cambiaría eso su justificación con respecto a su punto de la cadena que está en la memoria del montón?
Matthew
@Matthew: ¿Usar una clase te da un problema de rendimiento? Si no, entonces lanza una moneda para decidir cuál usar.
Greg Burghardt
44
@Matthew: Realmente lo importante es centralizar la problemática lógica de normalizar los códigos y las comparaciones. Después de eso, "clase versus estructura" es solo una discusión académica, hasta que se mida un impacto lo suficientemente grande en el rendimiento para justificar el tiempo extra del desarrollador para tener una discusión académica.
Greg Burghardt
1
Es cierto, no me importa tener una discusión académica de vez en cuando si me ayuda a crear soluciones mejor informadas en el futuro.
Matthew
@Matthew: Sí, tienes toda la razón. Dicen que "hablar es barato". Ciertamente es más barato que no hablar y construir algo mal. :)
Greg Burghardt
13

Usa el patrón Flyweight

Como el aeropuerto es, correctamente, inmutable, no hay necesidad de crear más de una instancia de una en particular, por ejemplo, SFO. Use un Hashtable o similar (tenga en cuenta que soy un tipo Java, no C #, por lo que los detalles exactos pueden variar) para almacenar en caché los aeropuertos cuando se crean. Antes de crear uno nuevo, verifique la tabla de hash. Nunca está liberando aeropuertos, por lo que GC no tiene necesidad de liberarlos.

Una ventaja menor adicional (al menos en Java, no estoy seguro acerca de C #) es que no necesita escribir un equals()método, lo ==hará de manera simple . Lo mismo para hashcode().

user949300
fuente
3
Excelente uso del patrón de peso mosca.
Neil
2
Suponiendo que OP sigue usando una estructura y no una clase, ¿el internado de cadenas ya no maneja los valores de cadena reutilizables? Las estructuras ya viven en la pila, las cadenas ya se están reutilizando para evitar valores duplicados en la memoria. ¿Qué beneficio adicional se obtendría del patrón de peso mosca?
Flater
Algo a tener en cuenta. Si se agrega o elimina un aeropuerto, querrá crear una forma de actualizar esta lista estática sin reiniciar la aplicación o volver a implementarla. Los aeropuertos no se agregan o eliminan con frecuencia, pero los dueños de negocios tienden a molestarse un poco cuando un simple cambio se vuelve tan complicado. "¿No puedo agregarlo en algún lugar? ¿Por qué tenemos que programar un lanzamiento / reinicio de la aplicación y molestar a nuestros clientes?" Pero al principio también estaba pensando en usar algún tipo de caché estática.
Greg Burghardt
@Flater Punto razonable. Diría que hay menos necesidad de que los programadores junior razonen sobre stack vs. Además, vea mi adición: no es necesario escribir equals ().
user949300
1
@Greg Burghardt Si el getAirportOrCreate()código se sincroniza correctamente, no hay ninguna razón técnica para que no pueda crear nuevos aeropuertos según sea necesario durante el tiempo de ejecución. Puede haber razones comerciales.
user949300
3

No soy un programador particularmente avanzado, pero ¿no sería un uso perfecto para un Enum?

Hay diferentes formas de construir clases enum a partir de listas o cadenas. Aquí hay uno que he visto en el pasado, aunque no estoy seguro de si es la mejor manera.

https://blog.kloud.com.au/2016/06/17/converting-webconfig-values-into-enum-or-list/

Adam B
fuente
2
Cuando hay potencialmente miles de valores diferentes (como es el caso de los códigos de aeropuerto), una enumeración simplemente no es práctica.
Ben Cottrell
Sí, pero el enlace que publiqué es cómo cargar cadenas como enumeraciones. Aquí hay otro enlace para cargar una tabla de búsqueda como una enumeración. Puede ser un poco de trabajo, pero aprovecharía el poder de las enumeraciones. exceptionnotfound.net/…
Adam B
1
O se puede cargar una lista de códigos válidos desde una base de datos o archivo. Luego se verifica un código de aeropuerto para estar entre esa lista. Esto es lo que normalmente hace cuando ya no desea codificar los valores y / o la lista se vuelve demasiado larga para administrar.
Neil
@BenCottrell que es precisamente para qué sirven las plantillas gen gen y T4.
RubberDuck
3

Una de las razones por las que está viendo más actividad de GC es porque ahora está creando una segunda cadena: la .ToUpperInvariant()versión de la cadena original. La cadena original es elegible para GC justo después de que se ejecuta el constructor y la segunda es elegible al mismo tiempo que el Airportobjeto. Es posible que pueda minimizarlo de una manera diferente (tenga en cuenta el tercer parámetro para string.Equals()):

public sealed class Airport : IEquatable<Airport>
{
    public Airport(string code)
    {
        if (code == null)
        {
            throw new ArgumentNullException(nameof(code));
        }

        if (code.Length != 3 || !char.IsLetter(code[0])
                             || !char.IsLetter(code[1]) || !char.IsLetter(code[2]))
        {
            throw new ArgumentException(
                "Must be a 3 letter airport code.",
                nameof(code));
        }

        Code = code;
    }

    public string Code { get; }

    public override string ToString()
    {
        return Code; // TODO: Upper-case it here if you really need to for display.
    }

    public bool Equals(Airport other)
    {
        return string.Equals(Code, other?.Code, StringComparison.InvariantCultureIgnoreCase);
    }

    public override bool Equals(object obj)
    {
        return obj is Airport airport && Equals(airport);
    }

    public override int GetHashCode()
    {
        return Code.GetHashCode();
    }

    public static bool operator ==(Airport left, Airport right)
    {
        return Equals(left, right);
    }

    public static bool operator !=(Airport left, Airport right)
    {
        return !Equals(left, right);
    }
}
Jesse C. Slicer
fuente
¿No produce esto códigos hash diferentes para aeropuertos iguales (pero con mayúsculas diferentes)?
Hero Wanders
Sí, me lo imagino. Dangit
Jesse C. Slicer
Este es un muy buen punto, nunca lo pensé, miraré hacer estos cambios.
Matthew
1
En lo que respecta a GetHashCode, solo debe usar StringComparer.OrdinalIgnoreCase.GetHashCode(Code)o similar
Matthew