Notre base de code est ancienne et les nouveaux programmeurs, comme moi, apprennent rapidement à le faire comme il le fait dans un souci d'uniformité. Pensant que nous devons commencer quelque part, j'ai décidé de refactoriser une classe de détenteurs de données en tant que telle:

• Suppression des méthodes de définition et création de tous les champs final(je prends " finalc'est bien" axiomatiquement). Les setters ont été utilisés uniquement dans le constructeur, il s'avère que cela n'a donc eu aucun effet secondaire.
• Introduit une classe Builder

La classe Builder était nécessaire parce que le constructeur (qui est à l'origine de la refactorisation) s'étend sur environ 3 lignes de code. Il a beaucoup de paramètres.

Par chance, un de mes coéquipiers travaillait sur un autre module et avait besoin des setters, car les valeurs dont il avait besoin sont devenues disponibles à différents moments du flux. Ainsi, le code ressemblait à ceci:

public void foo(Bar bar){
    //do stuff
    bar.setA(stuff);
    //do more stuff
    bar.setB(moreStuff);
}

J'ai soutenu qu'il devrait utiliser le générateur à la place, car se débarrasser des setters permet aux champs de rester immuables (ils m'ont déjà entendu parler de l'immuabilité auparavant), et aussi parce que les constructeurs permettent à la création d'objets d'être transactionnelle. J'ai esquissé le pseudocode suivant:

public void foo(Bar bar){
    try{
        bar.setA(a);
        //enter exception-throwing stuff
        bar.setB(b);
    }catch(){}
}

Si cette exception se déclenche, barles données seront corrompues, ce qui aurait été évité avec un générateur:

public Bar foo(){
        Builder builder=new Builder();
    try{
        builder.setA(a);
        //dangerous stuff;
        builder.setB(b);
        //more dangerous stuff
        builder.setC(c);
        return builder.build();
    }catch(){}
    return null;
}

Mes coéquipiers ont répliqué que l'exception en question ne se déclenchera jamais, ce qui est assez juste pour ce domaine particulier du code, mais je crois qu'il manque la forêt pour l'arbre.

Le compromis était de revenir à l'ancienne solution, à savoir utiliser un constructeur sans paramètres et tout régler avec des setters selon les besoins. La raison était que cette solution suit le principe KISS, que le mien viole.

Je suis nouveau dans cette entreprise (moins de 6 mois) et pleinement conscient que j'ai perdu celle-ci. Ma ou mes questions sont:

• Y a-t-il un autre argument pour utiliser Builders au lieu de "l'ancienne méthode"?
• Le changement que je propose en vaut-il vraiment la peine?

mais réellement,

• Avez-vous des conseils pour mieux présenter de tels arguments lorsque vous préconisez d'essayer quelque chose de nouveau?

Pensant que nous devons commencer quelque part, j'ai décidé de refactoriser une classe de détenteurs de données

C'est là que cela s'est mal passé - vous avez apporté une modification architecturale majeure à la base de code de telle sorte qu'elle est devenue très différente de ce qui était attendu. Vous avez surpris vos collègues. La surprise n'est bonne que si elle implique une fête, le principe de la moindre surprise est d'or (même si cela implique des fêtes, alors je saurais porter un slip propre!)

Maintenant, si vous pensiez que ce changement valait la peine, il aurait été préférable d'esquisser le pseudocode montrant le changement et de le présenter à vos collègues (ou au moins à celui qui a le rôle le plus proche de l'architecte) et de voir ce qu'ils pensaient avant de faire quoi que ce soit. Dans l'état actuel des choses, vous êtes devenu un voyou, pensant que toute la base de code était à vous pour jouer comme vous le vouliez. Un joueur dans l'équipe, pas un joueur d'équipe!

Je suis peut-être un peu dur avec vous, mais je me suis retrouvé à l'opposé: consultez du code et pensez que "WTF est arrivé ici?!", Seulement pour trouver le nouveau mec (c'est presque toujours le nouveau mec) qui a décidé de changer une charge de choses basée sur ce qu'il pense que la "bonne façon" devrait être. Vis avec l'historique SCM aussi, ce qui est un autre problème majeur.

Par chance, un de mes coéquipiers travaillait sur un autre module et avait besoin des setters, car les valeurs dont il avait besoin sont devenues disponibles à différents moments du flux.

Comme décrit, je ne vois pas ce qui fait que le code nécessite des setters. Je ne connais probablement pas assez le cas d'utilisation, mais pourquoi ne pas attendre que tous les paramètres soient connus avant d'instancier l'objet?

Comme alternative, si la situation nécessite des setters: offrez un moyen de figer votre code pour que les setters lancent une erreur d'assertion (alias erreur de programmeur) si vous essayez de modifier des champs une fois que l'objet est prêt.

Je pense que supprimer les setters et utiliser final est la façon "correcte" de le faire, ainsi que d'imposer l'immuabilité, mais est-ce vraiment avec quoi vous devriez passer votre temps?

Conseils généraux

Ancien = mauvais, nouveau = bon, à ma façon, à votre façon ... ce genre de discussion n'est pas constructif.

Actuellement, la façon dont votre objet est utilisé suit une règle implicite. Cela fait partie de la spécification non écrite de votre code et votre équipe peut penser qu'il n'y a pas beaucoup de risques pour les autres parties du code de l'utiliser de manière abusive. Ce que vous voulez faire ici est de rendre la règle plus explicite avec un générateur et des vérifications au moment de la compilation.

D'une certaine manière, le refactoring du code est lié à la théorie de la fenêtre brisée : vous pensez qu'il est juste de résoudre tout problème tel que vous le voyez (ou de prendre note d'une réunion ultérieure "d'amélioration de la qualité") afin que le code soit toujours agréable à travailler, est sûr et propre. Cependant, vous et votre équipe n'avez pas la même idée de ce qui est "assez bien". Ce que vous voyez comme une opportunité de contribuer et d'améliorer le code, de bonne foi, est probablement considéré différemment de l'équipe existante.

Soit dit en passant, il ne faut pas supposer que votre équipe souffre nécessairement d'inertie, de mauvaises habitudes, d'un manque d'éducation, ... le pire que vous puissiez faire est de projeter l'image d'un savoir-faire qui est là pour leur montrer comment coder. Par exemple, l'immuabilité n'est pas quelque chose de nouveau , vos pairs l'ont probablement lu mais juste parce que ce sujet devient populaire dans les blogs ne suffit pas pour les convaincre de passer du temps à changer leur code.

Après tout, il existe d'innombrables façons d'améliorer une base de code existante, mais cela coûte du temps et de l'argent. Je pourrais regarder votre code, l'appeler "ancien" et trouver des choses à propos. Par exemple: pas de spécification formelle, pas assez d'affirmations, peut-être pas assez de commentaires ou de tests, pas assez de couverture de code, algorithme non optimal, trop d'utilisation de la mémoire, n'utilisant pas le "meilleur" modèle de conception possible dans chaque cas, etc. ad nauseam . Mais pour la plupart des points que je soulèverais, on pourrait penser: ça va, mon code fonctionne, il n'y a pas besoin de passer plus de temps dessus.

Peut-être que votre équipe pense que ce que vous proposez a dépassé le point des rendements décroissants. Même si vous aviez trouvé une instance réelle d'un bogue en raison du type d'exemple que vous montrez (à l'exception), ils le corrigeraient probablement une seule fois et ne voudraient pas refactoriser le code.

La question est donc de savoir comment dépenser votre temps et votre énergie, étant donné qu'ils sont limités ? Il y a des changements continus apportés au logiciel mais généralement votre idée commence par un score négatif jusqu'à ce que vous puissiez lui fournir de bons arguments. Même si vous avez raison sur l'avantage, ce n'est pas un argument suffisant en soi, car il finira dans le panier " Nice to have, one day ... ".

Lorsque vous présentez vos arguments, vous devez faire des vérifications de "santé mentale": essayez-vous de vendre l'idée ou vous-même? Et si quelqu'un d'autre le présentait à l'équipe? Trouvez-vous des défauts dans leur raisonnement? Essayez-vous d'appliquer certaines bonnes pratiques générales ou avez-vous pris en compte les spécificités de votre code?

Ensuite, assurez-vous de ne pas apparaître comme si vous tentiez de corriger les "erreurs" passées de votre équipe. Cela aide à montrer que vous n'êtes pas votre idée, mais que vous pouvez raisonnablement accepter les commentaires. Plus vous faites cela, plus vos suggestions auront l'occasion d'être suivies.

Comment puis-je promouvoir l'utilisation du modèle Builder dans mon équipe?

Non.

Si votre constructeur a 3 lignes de paramètres, c'est trop gros et trop complexe. Aucun modèle de conception ne résoudra cela.

Si vous avez une classe dont les données sont disponibles à des moments différents, il doit s'agir de deux objets différents, ou votre consommateur doit agréger les composants, puis construire l'objet. Ils n'ont pas besoin d'un constructeur pour le faire.

Vous semblez plus concentré sur le fait d'avoir raison que de bien travailler avec les autres. Pire encore, vous reconnaissez que ce que vous faites est une lutte de pouvoir et pourtant vous n'arrivez pas à réfléchir à la façon dont les choses sont susceptibles de se sentir pour les personnes dont vous contestez l'expérience et l'autorité.

Inversez cela.

Concentrez-vous sur le travail avec les autres. Formulez vos pensées sous forme de suggestions et d'idées. Demandez-leur de parler de LEURS idées avant de leur poser des questions pour les faire réfléchir aux vôtres. Évitez les confrontations directes et soyez ouvertement disposé à accepter que faire les choses à la manière du groupe (pour l'instant) est plus productif que de mener une bataille difficile pour changer le groupe. (Mais ramenez les choses.) Faites de votre travail une joie.

Faites-le de manière cohérente et vous devriez créer moins de conflits et trouver plus de vos idées adoptées par d'autres. Nous souffrons tous de l'illusion que l'argument logique parfait va épater les autres et gagner la victoire. Et si cela ne fonctionne pas de cette façon, nous pensons que cela devrait .

Mais cela est en conflit direct avec la réalité. La plupart des arguments sont perdus avant le premier point logique. Même parmi les personnes prétendument logiques, la psychologie l'emporte sur la raison. Convaincre les gens, c'est dépasser les barrières psychologiques pour être bien entendu et non pas avoir une logique parfaite.

Y a-t-il un autre argument pour utiliser Builders au lieu de "l'ancienne méthode"?

"Quand vous avez un nouveau marteau, tout ressemblera à un clou." - c'est ce que je pensais en lisant votre question.

Si j'étais votre collègue, je vous demanderais "pourquoi ne pas initialiser l'objet entier dans le constructeur?" C'est ce que c'est que la fonctionnalité après tout. Pourquoi utiliser le modèle constructeur (ou tout autre modèle)?

Le changement que je propose en vaut-il vraiment la peine?

Il est difficile de distinguer ce petit extrait de code. C'est peut-être le cas - vous et vos collègues devez l'évaluer.

Avez-vous des conseils pour mieux présenter de tels arguments lorsque vous préconisez d'essayer quelque chose de nouveau?

Oui, apprenez-en plus sur le sujet avant d'en discuter. Armez-vous de bons arguments et justifications avant d'entrer dans la discussion.

J'ai été dans la situation où j'ai été recruté pour mes compétences. Quand j'ai pu voir le code, eh bien ... c'était plus qu'horrible. Lorsqu'on essaie ensuite de le nettoyer, quelqu'un qui est là depuis plus longtemps supprime toujours les changements, car il ne voit pas les avantages. Cela ressemble à quelque chose que vous décrivez. J'ai fini par quitter ce poste et je peux maintenant évoluer beaucoup mieux dans une entreprise qui a une meilleure pratique.

Je ne pense pas que vous devriez simplement jouer un rôle avec les autres membres de l'équipe, car ils sont là depuis plus longtemps. Si vous voyez que les membres de votre équipe utilisent de mauvaises pratiques dans les projets dans lesquels vous travaillez, c'est une responsabilité que vous devez signaler imo, comme vous l'avez fait. Sinon, vous n'êtes pas une ressource très précieuse. Si vous pointez ces choses encore et encore, mais que personne n'écoute, demandez à la direction un remplacement ou changez d'emploi. Il est très important que vous ne vous autorisiez pas à vous y adapter.

À la vraie question

Pourquoi utiliser des objets immuables? Parce que vous savez à quoi vous avez affaire.

Supposons que vous disposiez d'une connexion db qui vous permet de changer d'hôte via un setter, alors vous ne savez pas de quelle connexion il s'agit. Être immuable, alors vous savez.

Supposons cependant que vous ayez une structure de données qui peut être extraite de la persistance puis ré-persistée avec de nouvelles données, alors il est logique que cette structure ne soit pas immuable. C'est la même entité, mais les valeurs peuvent changer. Utilisez plutôt des objets de valeur immuables comme arguments.

Ce sur quoi vous concentrez la question est cependant un modèle de générateur pour se débarrasser des dépendances dans le constructeur. Je dis un gros NON à cela. L'instance est évidemment déjà trop complexe. N'essayez pas de le cacher, réparez-le!

Tl; dr

La suppression des setters peut être correcte, selon la classe. Le modèle de générateur ne résout pas le problème, il le cache simplement. (La saleté sous le tapis existe toujours même si vous ne pouvez pas la voir)

Bien que toutes les autres réponses soient très utiles (plus utiles que la mienne), il existe une propriété de constructeurs que vous n'avez pas encore mentionnée: en transformant la création d'un objet en processus, vous pouvez appliquer des contraintes logiques complexes.

Vous pouvez par exemple exiger que certains champs soient définis ensemble ou dans un ordre spécifique. Vous pouvez même renvoyer une implémentation différente de l'objet cible en fonction de ces décisions.

// business objects

interface CharRange {
   public char getCharacter();
   public int getFrom();
   public int getTo();
}

class MultiCharRange implements CharRange {
   final char ch;
   final int from;
   final int to;

   public char getCharacter() { return ch; }
   public int getFrom() { return from; }
   public int getTo() { return to; }
}

class SingleCharRange implements CharRange {
   final char ch;
   final int position;

   public char getCharacter() { return ch; }
   public int getFrom() { return position; }
   public int getTo() { return position; }
}

// builders

class Builder1 {
   public Builder2 character(char ch) {
      return new Builder2(ch);
   }
}

class Builder2 {
   final char ch;
   int from;
   int to;

   public Builder2 range(int from, int to) {
      if (from > to) throw new IllegalArgumentException("from > to");
      this.from = from;
      this.to = to;
      return this;
   }

   public Builder2 zeroStartRange(int to) {
      this.from = 0;
      this.to = to;
      return this;
   }

   public CharRange build() {
      if (from == to)
         return new SingleCharRange(ch,from);
      else 
         return new MultiCharRange(ch,from,to);
   }
}

Il s'agit d'un exemple noddy qui n'a pas beaucoup de sens mais qui démontre les trois propriétés: le caractère est toujours défini en premier, les limites de plage sont toujours définies et validées ensemble, et vous choisissez votre implémentation au moment de la construction.

Il est facile de voir comment cela peut conduire à un code utilisateur plus lisible et plus fiable, mais comme vous pouvez également le voir, il y a un inconvénient: beaucoup, beaucoup de code constructeur (et j'ai même laissé de côté les constructeurs pour raccourcir les extraits). Vous essayez donc de calculer le compromis, en posant des questions comme:

• Instancions-nous seulement l'objet d'un ou deux endroits? Cela est-il susceptible de changer?
• Peut-être devrions-nous imposer ces contraintes en refactorisant l'objet d'origine dans une composition d'objets plus petits?
• Aurions-nous fini par passer le constructeur de méthode en méthode?
• Pourrions-nous simplement utiliser une méthode d'usine statique à la place?
• Est-ce que cela fait partie d'une API externe, qui doit être aussi infaillible et lisse que possible?
• Environ combien de notre arriéré actuel de bugs aurait pu être évité si nous avions des constructeurs?
• Combien d'écriture de documentation supplémentaire économiserions-nous?

Vos collègues ont simplement décidé que le compromis ne serait pas avantageux. Vous devriez discuter des raisons ouvertement et honnêtement, de préférence sans recourir à des principes abstraits, mais en vous concentrant sur les implications pratiques de telle ou telle solution.

Il semble que cette classe soit en fait plus d'une classe, regroupée dans la même définition de fichier / classe. S'il s'agit d'un DTO, il devrait être possible de l'instancier en une seule fois et qu'il soit immuable. Si vous n'utilisez pas tous ses champs / propriétés dans une même transaction, cela brise presque certainement le principe de responsabilité unique. Il pourrait être utile de le diviser en classes DTO plus petites, plus cohérentes et immuables. Pensez à lire Clean Code si vous ne l'avez pas déjà fait afin de mieux articuler les problèmes et les avantages d'apporter un changement.

Je ne pense pas que vous puissiez concevoir du code à ce niveau par un comité, à moins que vous ne programmiez littéralement une foule (cela ne ressemble pas à ce que vous faites partie de ce type d'équipe), donc je pense que c'est correct de faire un changement en fonction de votre meilleur jugement, puis prenez-le sur le menton s'il s'avère que vous l'avez mal jugé.

Cependant, tant que vous êtes en mesure d'articuler les problèmes potentiels avec le code tel qu'il est, vous pouvez toujours débattre de la nécessité d'une solution , même si la vôtre n'est pas celle acceptée. Les gens sont alors libres de proposer des alternatives.

" Des opinions fortes, faiblement tenues " est un bon principe - vous allez de l'avant en toute confiance en fonction de ce que vous savez, mais si vous trouvez de nouvelles informations qui contrent, soyez modestes et démissionnez et entamez une discussion sur ce que la bonne solution devrait être. La seule mauvaise réponse est de laisser la situation empirer.