Question sur l'exemple de Robert C. Martin dans _Clean Code_.

Question

Il s'agit d'une question sur le concept d'une fonction qui ne fait qu'une seule chose. Elle n'aura pas de sens sans quelques passages pertinents pour le contexte, je vais donc les citer ici. Ils apparaissent aux pages 37-38 :

Pour le dire autrement, nous voulons pouvoir lire le programme comme s'il s'agissait d'un ensemble de paragraphes TO, chacun d'entre eux décrivant le niveau d'abstraction actuel et faisant référence aux paragraphes TO suivants au niveau inférieur.

Pour inclure les mises en place et les démontages, nous incluons les mises en place, puis nous incluons le contenu de la page de test, et enfin nous incluons les démontages. Pour inclure les configurations, nous incluons la configuration de la suite s'il s'agit d'une suite, puis nous incluons la configuration normale.

Il s'avère très difficile pour les programmeurs d'apprendre à suivre cette règle et d'écrire fonctions qui restent à un seul niveau d'abstraction. Mais apprendre cette astuce est aussi très important. C'est la clé pour que les fonctions soient courtes et qu'elles fassent "une seule chose". Faire en sorte que le code se lise comme un ensemble descendant de paragraphes TO est une technique efficace pour pour maintenir un niveau d'abstraction cohérent.

Il donne ensuite l'exemple suivant de code médiocre :

public Money calculatePay(Employee e) 
throws InvalidEmployeeType {
switch (e.type) {
  case COMMISSIONED:
    return calculateCommissionedPay(e);
  case HOURLY:
    return calculateHourlyPay(e);
  case SALARIED:
    return calculateSalariedPay(e);
  default:
    throw new InvalidEmployeeType(e.type);
}
}

et explique les problèmes qu'elle pose comme suit :

Cette fonction pose plusieurs problèmes. Tout d'abord, elle est volumineuse, et lorsque de nouvelles types d'employés sont ajoutés, elle augmentera. Deuxièmement, elle fait très clairement plus d'une chose. Troisièmement, elle viole le principe de la responsabilité unique7 (SRP) car il y a plus d'une raison pour laquelle elle raison pour laquelle il doit changer. Quatrièmement, il viole le principe d'ouverture et de fermeture8 (OCP) parce qu'il doit changer chaque fois que de nouveaux types sont ajoutés.

Maintenant mes questions.

Pour commencer, il est clair pour moi qu'il viole l'OCP, et il est clair pour moi que cela seul en fait une mauvaise conception. Cependant, j'essaie de comprendre chaque principe, et je ne vois pas très bien comment l'ASR s'applique. Plus précisément, la seule raison que je puisse imaginer pour que cette méthode change est l'ajout de nouveaux types d'employés. Il n'y a qu'un seul "axe de changement". Si les détails du calcul devaient changer, cela n'affecterait que les sous-méthodes telles que "calculateHourlyPay()"

De plus, si, dans un sens, il fait manifestement trois choses, ces trois choses se situent toutes au même niveau d'abstraction et peuvent toutes être regroupées dans un paragraphe d'OT identique à celui de l'exemple : Pour calculer la rémunération d'un employé, on calcule la rémunération à la commission si l'employé est à la commission, la rémunération horaire s'il est à l'heure, etc. . Ainsi, à part sa violation de l'OCP, ce code semble être conforme aux autres exigences de Martin en matière de code propre, même s'il soutient le contraire.

Quelqu'un peut-il m'expliquer ce que je rate ?

Merci.

Answer 1

Il semble qu'il y ait deux raisons pour que calculatePay change :

1. Changements dans les types d'employés
2. Modification du calcul de la rémunération

Deux axes de changement différents. Cependant, la responsabilité de la méthode calculatePay est le calcul du salaire. Elle ne devrait changer que si la formule de calcul de la rémunération est modifiée. Je pense que c'est la raison pour laquelle l'auteur déclare que la méthode viole l'ASR.

Dans le livre, l'auteur propose une solution où il définit des classes pour chaque type d'employé dérivées d'une classe de base abstraite commune Employé. Il déplace la méthode calculatePay vers la classe de base Employee et définit une classe factory Employee qui crée les objets employee appropriés à partir d'un type d'employé. De cette façon, chaque calcul de salaire est encapsulé dans une classe de type d'employé spécifique et n'est donc pas affecté par les changements de types d'employés. De plus, dans cette solution simple, la classe de fabrique d'employés n'est affectée que par les changements de types d'employés. La nouvelle solution rend donc l'ASR heureux.

Dans la nouvelle solution, vous demandez à un employé de calculer son salaire, ce qui ne me plaît pas car cela ne reflète pas la réalité. On peut même dire que cela aussi est une violation de l'ASR. Ce calcul est de la responsabilité du service des salaires. J'aime que le modèle du logiciel représente autant que possible le domaine du monde réel, mais nous devons généralement faire des compromis. Dans ce cas, je dirais que les changements de types d'employés ne vont pas se produire régulièrement. En fait, ils ne se produiront probablement que très rarement. Je garderais donc les choses là où elles ont un sens du point de vue du domaine d'activité, par exemple en demandant à l'objet de paie de calculer la rémunération des employés. Dans le même temps, j'aurais et conserverais un ensemble complet de tests unitaires, comme il se doit, pour m'assurer que lorsqu'un changement de type d'employé se produit, tout continue à fonctionner comme prévu.

Answer 2

Je prends un risque ici car je n'ai pas assez de contexte. Cette méthode pourrait changer pour DEUX raisons (d'ailleurs, le code viole également le principe d'encapsulation de base).

1. Lorsqu'un nouveau type d'employé est ajouté mais que la stratégie de calcul de la rémunération correspond aux stratégies existantes.
2. Lorsqu'un nouveau type d'employé est ajouté ET que cela nécessite une nouvelle stratégie de calcul de la rémunération.

Dans ces deux cas, l'abstraction qui doit être modifiée est le nouveau type d'employé ajouté, et non le client/utilisateur de la classe Employé. Je veux dire que la méthode calculatePay() (le calcul de la rémunération devrait être encapsulé) appartient à l'abstraction Employee, quelque chose de semblable à ce qui suit

interface SalariedEmployee
{
BigDecimal calculatePay();
}

class HourlyEmployee implements SalariedEmployee
{
}

class CommissionedEmployee implements SalariedEmployee
{
}

Answer 3

J'essayais aussi de comprendre et je pense avoir trouvé quelque chose qui pourrait vous convaincre si ce n'est pas le cas.

Il y a en fait 2 raisons de changer cette méthode :

1. calcul de la rémunération : parce que vous pouvez décider, pour n'importe quelle raison, qu'un employé payé à l'heure sera désormais payé en utilisant la stratégie mensuelle. Vous ne modifiez pas la formule de calcul proprement dite, mais la manière dont vous payez ce type d'employé, ce qui constitue un changement au même niveau d'abstraction.
2. modification des types d'employés : vous pouvez ajouter un autre type d'employé (freelance) ou même renommer le type en quelque chose d'autre.

Ce message semble un peu ancien mais j'espère qu'il aidera d'autres personnes à comprendre.