Combien de logique dans Getters

Mes collègues me disent qu'il devrait y avoir le moins de logique possible dans les getters et les setters.

Pourtant, je suis convaincu que beaucoup de choses peuvent être cachées dans les accesseurs et les installateurs pour protéger les utilisateurs / programmeurs des détails de la mise en œuvre.

Un exemple de ce que je fais:

public List<Stuff> getStuff()
{
   if (stuff == null || cacheInvalid())
   {
       stuff = getStuffFromDatabase();
   }
   return stuff;
}

Un exemple de la façon dont le travail me dit de faire les choses (ils citent «Clean Code» de Oncle Bob):

public List<Stuff> getStuff()
{
    return stuff;
}

public void loadStuff()
{
    stuff = getStuffFromDatabase();
}

Combien de logique est appropriée dans un setter / getter? Quelle est l'utilisation de getters et de setters vides, sauf en cas de violation de la dissimulation de données?

La façon dont le travail vous dit de faire les choses est boiteux.

En règle générale, ma façon de faire les choses est la suivante: si obtenir le contenu est peu coûteux du point de vue du calcul (ou si la plupart du temps il se trouve dans le cache), votre style de getStuff () convient parfaitement. Si obtenir le matériel est connu pour être coûteux en calcul, si coûteux que la publicité de son coût est nécessaire à l'interface, je ne l'appellerais pas getStuff (), je l'appellerais CalculateStuff () ou quelque chose du genre, afin d'indiquer que il y aura du travail à faire.

Dans les deux cas, la façon dont le travail vous dit de faire les choses est boiteuse, car getStuff () explose si loadStuff () n'a pas été appelé à l'avance. Ils veulent donc essentiellement que vous compliquiez votre interface en introduisant une complexité d'ordre des opérations. à cela. L'ordre des opérations est à peu près le pire type de complexité auquel je puisse penser.

La logique des accesseurs convient parfaitement.

Mais obtenir des données d'une base de données, c'est beaucoup plus que de la "logique". Il s'agit d'une série d' opérations très coûteuses où beaucoup de problèmes peuvent se produire, de manière non déterministe. J'hésiterais à le faire implicitement dans un getter.

D'un autre côté, la plupart des ORM prennent en charge le chargement paresseux des collections, ce qui est fondamentalement exactement ce que vous faites.

Je pense que selon le "code propre" il devrait être divisé autant que possible en quelque chose comme:

public List<Stuff> getStuff() {
   if (hasStuff()) {
       return stuff;
   }
   loadStuff();
   return stuff;
}

private boolean hasStuff() {
    if (stuff == null) {
       return false;
    }
    if (cacheInvalid()) {
       return false;        
    }
    return true;
} 

private void loadStuff() {
    stuff = getStuffFromDatabase();
}

Bien sûr, c'est totalement absurde, étant donné que la belle forme que vous avez écrite fait le bon choix avec une fraction de code que tout le monde comprend en un coup d'œil:

public List<Stuff> getStuff() {
   if (stuff == null || cacheInvalid()) {
       stuff = getStuffFromDatabase();
   }
   return stuff;
}

Cela ne devrait pas être le casse-tête de l'appelant, mais surtout le fait de ne pas avoir mal à la tête de l'appelant de ne pas oublier d'appeler les choses dans un "ordre correct" arbitraire.

Ils me disent qu'il devrait y avoir le moins de logique possible dans les Getters et les Setters.

Il faut qu'il y ait autant de logique que nécessaire pour répondre aux besoins de la classe. Personnellement, je préfère le moins possible, mais lors de la maintenance du code, vous devez généralement laisser l'interface d'origine avec les getters / setters existants, mais mettez-y beaucoup de logique pour corriger une nouvelle logique métier (par exemple, un "getter dans un environnement post-911 doit respecter les règles " connaître son client "et les réglementations OFAC , associées à une politique d'entreprise interdisant la comparution de clients de certains pays [tels que Cuba ou l'Iran]).

Dans votre exemple, je préfère le vôtre et n'aime pas l'exemple "oncle bob" car la version "oncle bob" oblige les utilisateurs / responsables à se rappeler d'appeler loadStuff()avant d'appeler getStuff()- il s'agit d'une recette pour un désastre si l'un de vos responsables oublie (ou pire, je ne savais jamais). La plupart des endroits où j'ai travaillé au cours de la dernière décennie utilisent encore du code datant de plus de 10 ans. La facilité de maintenance est donc un facteur essentiel à prendre en compte.

Vous avez raison, vos collègues ont tort.

Oubliez les règles empiriques de chacun sur ce qu'une méthode get devrait ou ne devrait pas faire. Une classe devrait présenter une abstraction de quelque chose. Votre classe est lisible stuff. En Java, il est classique d'utiliser des méthodes 'get' pour lire les propriétés. Des milliards de lignes de cadres ont été écrits s'attendant à lire stuffpar appel getStuff. Si vous nommez votre fonction fetchStuffou autre chose que getStuff, votre classe sera incompatible avec tous ces frameworks.

Vous pouvez les diriger vers Hibernate, où 'getStuff ()' peut faire des choses très compliquées et lève une exception RuntimeException en cas d'échec.

Cela pourrait être un peu un débat puriste par rapport à une application qui pourrait être affecté par la façon dont vous préférez contrôler les noms de fonction. Du point de vue appliqué, je préférerais de loin voir:

List<String> names = clientRoster.getNames();
List<String> emails = clientRoster.getEmails();

Par opposition à:

myObject.load();
List<String> names = clientRoster.getNames();
List<String> emails = clientRoster.getEmails();

Ou pire encore:

myObject.loadNames();
List<String> names = clientRoster.getNames();
myOjbect.loadEmails();
List<String> emails = clientRoster.getEmails();

Ce qui tend à rendre les autres codes beaucoup plus redondants et plus difficiles à lire car vous devez commencer à parcourir tous les appels similaires. En outre, l’appel de fonctions de chargeur ou de fonctions similaires annule l’objectif même de l’utilisation de la programmation orientée objet, dans la mesure où vous ne faites plus abstraction des détails d’implémentation de l’objet avec lequel vous travaillez. Si vous avez un clientRosterobjet, vous ne devriez pas avoir à vous soucier de la façon dont getNamesfonctionne, comme vous le feriez si vous deviez appeler un loadNames, vous devez juste savoir que cela getNamesvous donne un List<String>avec le nom des clients.

Il semble donc que le problème concerne davantage la sémantique et le meilleur nom pour la fonction permettant d’obtenir les données. Si la société (et d'autres) a un problème avec le préfixe getet set, pourquoi ne pas appeler la fonction à sa retrieveNamesplace? Il dit ce qui se passe mais n'implique pas que l'opération serait instantanée comme on pourrait s'y attendre d'une getméthode.

En termes de logique dans une méthode d'accès, limitez-la au minimum car elles sont généralement impliquées comme instantanées, avec uniquement une interaction nominale se produisant avec la variable. Cependant, cela ne concerne généralement que les types simples, les types de données complexes (c'est-à-dire List). Je trouve plus difficile d'encapsuler correctement dans une propriété et d'utiliser généralement d'autres méthodes pour interagir avec elles, par opposition à un mutateur strict et à un accesseur.

L'appel d'un getter doit présenter le même comportement que la lecture d'un champ:

• Récupérer la valeur devrait être bon marché
• Si vous définissez une valeur avec le setter et que vous la lisez ensuite avec le getter, la valeur doit être la même.
• Obtenir la valeur ne devrait avoir aucun effet secondaire
• Il ne faut pas jeter une exception

Un getter qui appelle d'autres propriétés et méthodes afin de calculer sa propre valeur implique également une dépendance. Par exemple, si votre propriété doit pouvoir se calculer elle-même et qu'un autre membre doit être défini, vous devez vous inquiéter des références nulles accidentelles si vous accédez à votre propriété dans le code d'initialisation où tous les membres ne sont pas nécessairement définis.

Cela ne signifie pas «n'accédez jamais à un autre membre que le champ de sauvegarde des propriétés dans le getter», cela signifie simplement que vous devez prêter attention à ce que vous sous-entendez concernant l'état requis de l'objet et si cela correspond au contexte que vous attendez. cette propriété est accessible à.

Cependant, dans les deux exemples concrets que vous avez donnés, la raison pour laquelle je choisirais l'un plutôt que l'autre est totalement différente. Votre getter est initialisé lors du premier accès, par exemple, initialisation différée . Le deuxième exemple est supposé être initialisé à un moment donné, par exemple, Initialisation explicite .

Lorsque l’initialisation se produit exactement, cela peut être important ou non.

Par exemple, il peut être très lent et doit être effectué au cours d’une étape de chargement où l’utilisateur s’attend à un délai, plutôt qu’à un ralentissement inattendu des performances lorsque l’utilisateur déclenche un accès (c.-à-d. Un clic droit, un menu contextuel apparaît, a déjà fait un clic droit de nouveau).

De plus, parfois, il y a un point évident dans l'exécution où tout ce qui peut affecter / salir la valeur de la propriété en cache se produit. Vous pouvez même vérifier qu'aucune des dépendances ne change et renvoyer des exceptions ultérieurement. Dans cette situation, il est judicieux de mettre également en cache la valeur à ce stade, même si le calcul n'est pas particulièrement coûteux, afin d'éviter de rendre l'exécution du code plus complexe et plus difficile à suivre mentalement.

Cela dit, l’initialisation différée a beaucoup de sens dans bien d’autres situations. Ainsi, comme il arrive souvent dans la programmation, il est difficile de résumer en une règle, cela revient au code concret.

Faites-le comme @MikeNakis l'a dit ... Si vous avez juste le truc, c'est bien ... Si vous faites autre chose, créez une nouvelle fonction qui fait le travail et le rend public.

Si votre propriété / fonction ne fait que ce que son nom l'indique, il ne reste plus beaucoup de place à la complication. La cohésion est la clé de l'OMI

Personnellement, j'exposerais l'exigence de Stuff via un paramètre dans le constructeur et laisserais à la classe quelle que soit la classe qui instancie une substance d'effectuer le travail de détermination de l'origine de celle-ci. Si stuff est null, il devrait renvoyer null. Je préfère ne pas essayer de solutions aussi astucieuses que l'original de l'OP, car il s'agit d'un moyen facile de masquer des bugs au sein de votre implémentation, où il n'est pas du tout évident de voir ce qui pourrait mal tourner en cas de problème.

Il y a des questions plus importantes que la "pertinence" ici et vous devez baser votre décision sur celles-ci . La principale décision à prendre ici est de savoir si vous voulez permettre aux gens de contourner ou non la mémoire cache.

1. Premièrement, réfléchissez s'il existe un moyen de réorganiser votre code afin que tous les appels de charge et la gestion du cache nécessaires soient effectués dans le constructeur / l'initialiseur. Si cela est possible, vous pouvez créer une classe dont l'invariant vous permet d'accéder au simple getter de la partie 2 avec la sécurité du getter complexe de la partie 1. (Un scénario gagnant-gagnant)

2. Si vous ne pouvez pas créer une telle classe, décidez si vous avez un compromis et devez décider si vous souhaitez autoriser le consommateur à ignorer ou non le code de vérification du cache.

   1. S'il est important que le consommateur ne passe jamais le contrôle du cache et que les pénalités de performances ne vous dérangent pas, couplez le contrôle à l'intérieur du getter et empêchez le consommateur de faire le mauvais choix.

   2. Si vous pouvez ignorer la vérification du cache ou s'il est très important que vous soyez assuré de la performance O (1) dans le getter, utilisez des appels séparés.

Comme vous l'avez peut-être déjà noté, je ne suis pas un grand fan de la philosophie du "code propre", "tout scinder en petites fonctions". Si vous avez un tas de fonctions orthogonales qui peuvent être appelées dans n'importe quel ordre, leur division vous donnera plus de pouvoir expressif à peu de frais. Cependant, si vos fonctions ont des dépendances d'ordre (ou ne sont vraiment utiles que dans un ordre particulier), leur division n'augmente que le nombre de façons dont vous pouvez faire des choses mal, tout en apportant peu d'avantages.

À mon avis, les Getters ne devraient pas contenir beaucoup de logique. Ils ne devraient pas avoir d'effets secondaires et vous ne devriez jamais obtenir une exception. À moins bien sûr, vous savez ce que vous faites. La plupart de mes accesseurs n'ont aucune logique en eux et vont simplement dans un champ. La seule exception à cette règle est une API publique qui devait être aussi simple que possible à utiliser. J'ai donc eu un getter qui échouerait si un autre getter n'avait pas été appelé. La solution? Une ligne de code comme var throwaway=MyGetter;dans le getter qui en dépendait. Je n'en suis pas fier, mais je ne vois toujours pas de façon plus propre de le faire.

Cela ressemble à une lecture de cache avec chargement paresseux. Comme d'autres l'ont noté, la vérification et le chargement peuvent appartenir à d'autres méthodes. Il se peut que le chargement doive être synchronisé afin d'éviter de charger vingt threads en même temps.

Il peut être approprié d’utiliser un nom getCachedStuff()pour le getter car il n’a pas de temps d’exécution cohérent.

Selon le fonctionnement de la cacheInvalid()routine, la vérification de la valeur NULL peut ne pas être nécessaire. Je ne m'attendrais pas à ce que le cache soit valide sauf s'il stuffa été rempli à partir de la base de données.

La logique principale que je m'attendrais à voir dans les getters qui renvoient une liste est la logique permettant de s'assurer que la liste est non modifiable. Dans l'état actuel des choses, votre exemple casserait potentiellement l'encapsulation.

quelque chose comme:

public List<Stuff> getStuff()
{
    return Collections.unmodifiableList(stuff);
}

En ce qui concerne la mise en cache dans le getter, je pense que ce serait OK, mais je pourrais être tenté de sortir de la logique du cache si la construction du cache prenait beaucoup de temps. c'est à dire cela dépend.

Selon le cas d'utilisation exact, j'aime séparer ma mise en cache dans une classe distincte. Créez une StuffGetterinterface, implémentez une StuffComputerqui effectue les calculs et enveloppez-la dans un objet StuffCacherqui est responsable de l’accès au cache ou du transfert des appels vers StuffComputercelui qu’il encapsule.

interface StuffGetter {
     public List<Stuff> getStuff();
}

class StuffComputer implements StuffGetter {
     public List<Stuff> getStuff() {
         getStuffFromDatabase()
     }
}

class StuffCacher implements StuffGetter {
     private stuffComputer; // DI this
     private Cache<List<Stuff>> cache = new Cache<>();

     public List<Stuff> getStuff() {
         if cache.hasStuff() {
             return cache.getStuff();
         }

         List<Stuffs> stuffs = stuffComputer.getStuff();
         cache.store(stuffs);
         return stuffs;
     }
}

Cette conception vous permet d'ajouter facilement du cache, de supprimer le cache, de changer la logique de dérivation sous-jacente (par exemple, accéder à une base de données ou de renvoyer des données fictives), etc.

IMHO c'est très simple si vous utilisez un design par contrat. Décidez ce que votre getter doit fournir et codez-le en conséquence (code simple ou logique complexe pouvant être impliquée ou déléguée quelque part).