struct avec une valeur par défaut absurde

12

Dans mon système je fonctionne souvent avec des codes d'aéroport ( "YYZ", "LAX", "SFO", etc.), ils sont toujours dans le même format exact (3 lettres, représentée en majuscules). Le système traite généralement 25 à 50 de ces codes (différents) par demande d'API, avec plus de mille allocations au total, ils sont transmis à travers de nombreuses couches de notre application et sont comparés assez souvent pour l'égalité.

Nous avons commencé par simplement passer des chaînes, ce qui a bien fonctionné un peu, mais nous avons rapidement remarqué beaucoup d'erreurs de programmation en passant un mauvais code quelque part où le code à 3 chiffres était attendu. Nous avons également rencontré des problèmes où nous étions censés faire une comparaison insensible à la casse et qui ne l'ont pas fait, ce qui a entraîné des bogues.

À partir de cela, j'ai décidé d'arrêter de passer des chaînes et de créer une Airportclasse, qui a un seul constructeur qui prend et valide le code de l'aéroport.

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

Cela a rendu notre code beaucoup plus facile à comprendre et nous avons simplifié nos vérifications d'égalité, nos utilisations de dictionnaire / ensemble. Nous savons maintenant que si nos méthodes acceptent une Airportinstance qui se comportera comme nous l'attendons, cela a simplifié nos vérifications de méthodes en une vérification de référence nulle.

La chose que j'ai remarquée, cependant, était que la collecte des ordures fonctionnait beaucoup plus souvent, ce que j'ai retrouvé dans de nombreux cas de Airportcollecte.

Ma solution à cela a été de convertir le fichier classen a struct. Il s'agissait principalement d'un changement de mot clé, à l'exception de GetHashCodeet ToString:

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

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

Pour gérer le cas où default(Airport)est utilisé.

Mes questions:

  1. La création d'une Airportclasse ou d'une structure était-elle une bonne solution en général, ou suis-je en train de résoudre le mauvais problème / le résoudre de la mauvaise façon en créant le type? Si ce n'est pas une bonne solution, quelle est la meilleure solution?

  2. Comment mon application doit-elle gérer les instances où le default(Airport)est utilisé? Un type de default(Airport)n'a pas de sens pour mon application, donc je l'ai fait if (airport == default(Airport) { throw ... }dans des endroits où l'obtention d'une instance de Airport(et de ses Codepropriétés) est essentielle à l'opération.

Notes: J'ai passé en revue les questions C # / VB struct - comment éviter la casse avec des valeurs par défaut nulles, qui est considérée comme invalide pour une structure donnée? et utilisez struct ou non avant de poser ma question, mais je pense que mes questions sont suffisamment différentes pour justifier son propre message.

Matthieu
la source
7
La récupération de place a-t-elle un impact matériel sur la performance de votre application? En d'autres termes, est-ce important?
Robert Harvey
Quoi qu'il en soit, oui, la solution de classe était «bonne». La façon dont vous le savez est qu'il a résolu votre problème sans en créer de nouveaux.
Robert Harvey
2
Vous pouvez résoudre le default(Airport)problème en interdisant simplement les instances par défaut. Vous pouvez le faire en écrivant un constructeur et de jeter parameterless InvalidOperationExceptionou NotImplementedExceptionen elle.
Robert Harvey
3
Sur une note latérale, au lieu de confirmer que votre chaîne d'initialisation est en fait 3 caractères alpha, pourquoi ne pas simplement la comparer à la liste finie de tous les codes d'aéroport (par exemple, github.com/datasets/airport-codes ou similaire)?
Dan Pichelman
2
Je suis prêt à parier plusieurs bières que ce n'est pas la racine d'un problème de performance. Un ordinateur portable normal peut allouer dans l'ordre 10 millions d'objets / secondes.
Esben Skov Pedersen

Réponses:

6

Mise à jour: j'ai réécrit ma réponse pour corriger certaines hypothèses incorrectes sur les structures C #, ainsi que l'OP nous informant dans les commentaires que des chaînes internes sont utilisées.


Si vous pouvez contrôler les données entrant dans votre système, utilisez une classe telle que vous l'avez publiée dans votre question. Si quelqu'un court, default(Airport)il récupérera une nullvaleur. Assurez-vous d'écrire votre Equalsméthode privée pour renvoyer false chaque fois que vous comparez des objets Airport nuls, puis laissez-le NullReferenceExceptionvoler ailleurs dans le code.

Cependant, si vous importez des données dans le système à partir de sources que vous ne contrôlez pas, vous ne voulez pas nécessairement planter tout le thread. Dans ce cas, une structure est idéale car le simple fait default(Airport)vous donnera autre chose qu'un nullpointeur. Composez une valeur évidente pour représenter «aucune valeur» ou la «valeur par défaut» afin d'avoir quelque chose à imprimer à l'écran ou dans un fichier journal (comme «---» par exemple). En fait, je voudrais simplement garder le codeprivé et ne pas exposer du tout une Codepropriété - me concentrer uniquement sur le comportement ici.

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

Dans le pire des cas, la conversion default(Airport)en une chaîne s'imprime "---"et renvoie faux par rapport à d'autres codes d'aéroport valides. Tout code d'aéroport "par défaut" ne correspond à rien, y compris les autres codes d'aéroport par défaut.

Oui, les structures sont censées être des valeurs allouées sur la pile, et tout pointeur vers la mémoire de tas annule fondamentalement les avantages de performance des structures, mais dans ce cas, la valeur par défaut d'une structure a un sens et fournit une résistance supplémentaire aux balles pour le reste de la application.

Je plierais un peu les règles ici, à cause de cela.


Réponse originale (avec quelques erreurs factuelles)

Si vous pouvez contrôler les données entrant dans votre système, je ferais ce que Robert Harvey a suggéré dans les commentaires: créer un constructeur sans paramètre et lever une exception lors de son appel. Cela empêche les données non valides d'entrer dans le système via default(Airport).

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

Cependant, si vous importez des données dans le système à partir de sources que vous ne contrôlez pas, vous ne voulez pas nécessairement planter tout le thread. Dans ce cas, vous pouvez créer un code d'aéroport non valide, mais le faire apparaître comme une erreur évidente. Cela impliquerait la création d'un constructeur sans paramètre et la définition Codede quelque chose comme "---":

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

Puisque vous utilisez a stringcomme code, il est inutile d'utiliser une structure. La structure est allouée sur la pile, uniquement pour avoir l' Codeallocation comme pointeur vers une chaîne dans la mémoire du tas, donc aucune différence ici entre la classe et la structure.

Si vous avez changé le code d'aéroport en un tableau de 3 éléments de caractères, une structure serait entièrement allouée sur la pile. Même alors, le volume de données n'est pas si important pour faire la différence.

Greg Burghardt
la source
Si mon application utilisait des chaînes internes pour la Codepropriété, cela changerait-il votre justification concernant votre point de la chaîne étant en mémoire de tas?
Matthew
@Matthew: L'utilisation d'une classe vous pose-t-elle un problème de performances? Sinon, lancez une pièce pour décider laquelle utiliser.
Greg Burghardt
4
@Matthew: La chose vraiment importante est que vous avez centralisé la logique gênante de normalisation des codes et des comparaisons. Après cela, "la classe contre la structure" n'est plus qu'une discussion académique, jusqu'à ce que vous mesuriez un impact suffisamment important sur les performances pour justifier le temps supplémentaire du développeur pour avoir une discussion académique.
Greg Burghardt
1
C'est vrai, cela ne me dérange pas d'avoir une discussion académique de temps en temps si cela m'aide à créer des solutions mieux informées à l'avenir.
Matthew
@Matthew: Oui, vous avez absolument raison. Ils disent que "parler n'est pas cher". C'est certainement moins cher que de ne pas parler et de construire quelque chose de mal. :)
Greg Burghardt
13

Utilisez le modèle Flyweight

Étant donné que Airport est, à juste titre, immuable, il n'est pas nécessaire de créer plus d'une instance d'une instance particulière, disons, SFO. Utilisez une table de hachage ou similaire (notez que je suis un gars Java, pas C # donc les détails exacts peuvent varier) pour mettre en cache les aéroports lorsqu'ils sont créés. Avant d'en créer un nouveau, archivez le Hashtable. Vous ne libérez jamais d'aéroports, donc GC n'a pas besoin de les libérer.

Un autre avantage mineur (au moins en Java, vous n'êtes pas sûr de C #) est que vous n'avez pas besoin d'écrire une equals()méthode, un simple ==fera l'affaire. Pareil pour hashcode().

user949300
la source
3
Excellente utilisation du motif masselotte.
Neil
2
En supposant que OP continue d'utiliser une structure et non une classe, l' internement de chaîne ne gère-t-il pas déjà les valeurs de chaîne réutilisables? Les structures vivent déjà sur la pile, les chaînes sont déjà réutilisées afin d'éviter les valeurs en double en mémoire. Quel avantage supplémentaire serait tiré du modèle de poids de mouche?
Flater
Quelque chose à surveiller. Si un aéroport est ajouté ou supprimé, vous souhaiterez créer un moyen d'actualiser cette liste statique sans redémarrer l'application ni la redéployer. Les aéroports ne sont pas ajoutés ou supprimés souvent, mais les propriétaires d'entreprises ont tendance à être un peu contrariés lorsqu'un simple changement devient aussi compliqué. "Je ne peux pas simplement l'ajouter quelque part?! Pourquoi devons-nous planifier un redémarrage de version / application et incommoder nos clients?" Mais je pensais aussi à utiliser une sorte de cache statique au début.
Greg Burghardt
@Flater Point raisonnable. Je dirais que les programmeurs juniors ont moins besoin de raisonner pile / tas. De plus, voyez mon ajout - pas besoin d'écrire equals ().
user949300
1
@Greg Burghardt Si le getAirportOrCreate()code est correctement synchronisé, il n'y a aucune raison technique pour laquelle vous ne pouvez pas créer de nouveaux aéroports au besoin pendant l'exécution. Il peut y avoir des raisons commerciales.
user949300
3

Je ne suis pas un programmeur particulièrement avancé, mais ne serait-ce pas une utilisation parfaite pour un Enum?

Il existe différentes façons de construire des classes enum à partir de listes ou de chaînes. En voici un que j'ai vu dans le passé, je ne sais pas si c'est la meilleure façon.

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

Adam B
la source
2
Lorsqu'il existe potentiellement des milliers de valeurs différentes (comme c'est le cas avec les codes d'aéroport), une énumération n'est tout simplement pas pratique.
Ben Cottrell
Oui, mais le lien que j'ai publié est de savoir comment charger des chaînes en tant qu'énumérations. Voici un autre lien pour charger une table de recherche en tant qu'énumération. Cela pourrait être un peu de travail, mais profiterait du pouvoir des énumérations. exceptionnotfound.net/…
Adam B
1
Ou une liste de codes valides pourrait être chargée à partir d'une base de données ou d'un fichier. Ensuite, un code d'aéroport est juste vérifié pour faire partie de cette liste. C'est ce que vous faites généralement lorsque vous ne voulez plus coder en dur les valeurs et / ou que la liste devient trop longue à gérer.
Neil
@BenCottrell, c'est précisément à cela que servent les modèles de code gen et T4.
RubberDuck
3

L'une des raisons pour lesquelles vous voyez plus d'activité GC est parce que vous créez maintenant une deuxième chaîne - la .ToUpperInvariant()version de la chaîne d'origine. La chaîne d'origine est éligible pour GC juste après l'exécution du constructeur et la seconde est éligible en même temps que l' Airportobjet. Vous pourrez peut-être le réduire d'une manière différente (notez le troisième paramètre 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
la source
Cela ne donne-t-il pas des codes de hachage différents pour des aéroports égaux (mais différemment capitalisés)?
Hero Wanders
Ouais, j'imagine. Dangit.
Jesse C. Slicer
C'est un très bon point, je n'y ai jamais pensé, je vais essayer de faire ces changements.
Matthew
1
En ce qui concerne GetHashCode, devrait simplement utiliser StringComparer.OrdinalIgnoreCase.GetHashCode(Code)ou similaire
Matthew