Le kata Digging Estimator – Part 3 – Refactoring

  • Auteur/autrice de la publication :
  • Post category:Tests
  • Commentaires de la publication :0 commentaire

Cet article est le troisième de la série concernant le kata Digging Estimator, un kata créé par mes soins à destinations des élèves d’un module de tests.

Après avoir utilisé l’approval testing pour couvrir le code existant avec des tests, nous allons dans cette partie effectuer le refactoring pour permettre un ajout plus facile de la nouvelle fonctionnalité.

Les autres articles :

Vous pouvez trouver le code terminé, avec le détail des commits sur la branche ts-solution du github

Résumé des épisodes précédents

Dans l’article précédent, nous avons établi un filet de tests grâce à l’approval testing. Nos couvertures de code (100% sur la couverture de branche) et nos tests de mutation (score de 94%) nous mettent en confiance sur l’efficacité de nos tests.

Le but de cette étape

Il est temps maintenant de lancer le refactoring, c’est à dire la transformation du code en quelque chose de lisible. Pour cela, je vais me donner plusieurs axes de conduite :

  • Je vais faire attention aux moments où je modifie les tests. Normalement, ça ne devrait pas arriver du tout. Si ça arrive, c’est probablement que je commence à faire des modifications non rétro-compatibles, ou que j’ajoute des fonctionnalités, et ça, c’est mauvais signe.
  • Je vais tâcher au maximum d’utiliser mon IDE pour faire les refactoring, parce que lui sait faire les choses sans tout casser. Quand l’IDE ne sait pas faire je tâcherai de n’effectuer que des opérations simples que je sais pouvoir ne casser que peu de choses à la fois.
  • Si mes tests deviennent soudainement rouges, je m’encourage à revert après avoir appris pourquoi.
  • En conséquence de quoi je vais commit aussi fréquemment que possible.

Vers où aller ?

J’ai déjà un peu lu le code et noté des choses sur ma todo-list pendant que j’écrivais mes tests. Entre autres, je me suis fait les remarques suivantes :

  • L’objet TeamComposition mériterait d’être un vrai objet, pas un réceptacle à valeurs. En particulier, il mériterait d’être capable de calculer le total de nains.
  • L’objet Team souffre sûrement du même symptôme. En fait, je me dis qu’il pourrait lui même calculer sa composition, mais pour en être sûr, il va falloir que je creuse un peu plus (ah. ah. ah).

Petit tour dans le code

Pour le moment, le code ressemble à ça :

import { RockInformationInterface } from "./rock-information.interface";
import { VinRockInformationService } from "./external/vin-rock-information.service";

export class TunnelTooLongForDelayException extends Error {
}

export class InvalidFormatException extends Error {
}

export class Team {
  miners = 0;
  healers = 0;
  smithies = 0;
  lighters = 0;
  innKeepers = 0;
  guards = 0;
  guardManagers = 0;
  washers = 0;
}

export class TeamComposition {
  dayTeam: Team = new Team();
  nightTeam: Team = new Team();

  total = 0;
}

export class DiggingEstimator {
  private readonly rockInformation: RockInformationInterface;

  constructor(rockInformation?: RockInformationInterface) {
    this.rockInformation = rockInformation || new VinRockInformationService()
  }

  tunnel(length: number, days: number, rockType: string): TeamComposition {
    const digPerRotation = this.rockInformation.get(rockType);
    const maxDigPerRotation = digPerRotation[digPerRotation.length - 1];
    const maxDigPerDay = 2 * maxDigPerRotation;

    if (Math.floor(length) !== length || Math.floor(days) !== days || length < 0 || days < 0) {
      throw new InvalidFormatException();
    }
    if (Math.floor(length / days) > maxDigPerDay) {
      throw new TunnelTooLongForDelayException();
    }
    const composition = new TeamComposition();

    // Miners
    for (let i = 0; i < digPerRotation.length - 1; ++i) {
      if (digPerRotation[i] < Math.floor(length / days)) {
        composition.dayTeam.miners++;
      }
    }
    if (Math.floor(length / days) > maxDigPerRotation) {
      for (let i = 0; i < digPerRotation.length - 1; ++i) {
        if (digPerRotation[i] + maxDigPerRotation < Math.floor(length / days)) {
          composition.nightTeam.miners++;
        }
      }
    }
    const dt = composition.dayTeam;
    const nt = composition.nightTeam;

    if (dt.miners > 0) {
      ++dt.healers;
      ++dt.smithies;
      ++dt.smithies;
    }

    if (nt.miners > 0) {
      ++nt.healers;
      ++nt.smithies;
      ++nt.smithies;
    }

    if (nt.miners > 0) {
      nt.lighters = nt.miners + 1;
    }

    if (dt.miners > 0) {
      dt.innKeepers = Math.ceil((dt.miners + dt.healers + dt.smithies) / 4) * 4;
      dt.washers = Math.ceil((dt.miners + dt.healers + dt.smithies + dt.innKeepers) / 10);
    }

    if (nt.miners > 0) {
      nt.innKeepers = Math.ceil((nt.miners + nt.healers + nt.smithies + nt.lighters) / 4) * 4;
    }

    // eslint-disable-next-line no-constant-condition
    while (true) {
      const oldWashers = nt.washers;
      const oldGuard = nt.guards;
      const oldChiefGuard = nt.guardManagers;

      nt.washers = Math.ceil((nt.miners + nt.healers + nt.smithies + nt.innKeepers + nt.lighters + nt.guards + nt.guardManagers) / 10);
      nt.guards = Math.ceil((nt.healers + nt.miners + nt.smithies + nt.lighters + nt.washers) / 3);
      nt.guardManagers = Math.ceil((nt.guards) / 3);

      if (oldWashers === nt.washers && oldGuard === nt.guards && oldChiefGuard === nt.guardManagers) {
        break;
      }
    }

    composition.total = dt.miners + dt.washers + dt.healers + dt.smithies + dt.innKeepers +
      nt.miners + nt.washers +  nt.healers  + nt.smithies  + nt.innKeepers + nt.guards + nt.guardManagers + nt.lighters;
    return composition;
  }
}

Faux départ

Ok. C’est relativement long. J’aimerais commencer par déplacer tout ce qui n’a rien à faire dans des fichiers séparés. Mon IDE me permet de naviguer facilement, et ça me permettra de focaliser. Je crée donc les fichiers suivants :

Les nouveaux fichiers de modèle

Ok. Les tests ont bougé : les chemins d’import ont changé. Première alerte : ma modification n’est pas rétrocompatible. Si quelqu’un utilise mes fichiers tels quels, ses imports vont planter. En temps normal, on aurait mis un index.ts (barrel file) vers lequel ils pointent, mais dans notre cas, il n’y est pas. Je suis embêté. Je revert. Tant pis pour mon beau rangement.

Nettoyage de surface

Petit tour rapide, je vais nettoyer ce qui me saute aux yeux. 

if (Math.floor(length / days) > maxDigPerDay) {
  throw new TunnelTooLongForDelayException();
}
const composition = new TeamComposition();

// Miners
for (let i = 0; i < digPerRotation.length - 1; ++i) {
  if (digPerRotation[i] < Math.floor(length / days)) {
    composition.dayTeam.miners++;
  }
}
if (Math.floor(length / days) > maxDigPerRotation) {
  for (let i = 0; i < digPerRotation.length - 1; ++i) {
    if (digPerRotation[i] + maxDigPerRotation < Math.floor(length / days)) {
      composition.nightTeam.miners++;
    }
  }
}

Comptez voir combien de Math.floor(length / days) vous trouvez ? Un petit extract (Shift + Alt + V, oui, je suis sur Webstorm) me répond 4 :

const distanceToDigPerDay = Math.floor(length / days);
if (distanceToDigPerDay > maxDigPerDay) {
  throw new TunnelTooLongForDelayException();
}
const composition = new TeamComposition();

// Miners
for (let i = 0; i < digPerRotation.length - 1; ++i) {
  if (digPerRotation[i] < distanceToDigPerDay) {
    composition.dayTeam.miners++;
  }
}
if (distanceToDigPerDay > maxDigPerRotation) {
  for (let i = 0; i < digPerRotation.length - 1; ++i) {
    if (digPerRotation[i] + maxDigPerRotation < distanceToDigPerDay) {

Mieux. Dans la même veine :

const dt = composition.dayTeam;
const nt = composition.nightTeam;

Je n’aime pas les abréviations, mon cerveau doit faire le mapping en arrière plan. Hop, SHIFT + F6. 

const dayTeam = composition.dayTeam;
const nightTeam = composition.nightTeam;

En plus, TS permet de déstructurer :

const {dayTeam, nightTeam} = composition;

On continue :

if (dayTeam.miners > 0) {
  ++dayTeam.healers;
  ++dayTeam.smithies;
  ++dayTeam.smithies;
}

Vraiment ? Sachant que les variables valent 0 au début (on vient de les créer), il y a plus simple, non ?

if (dayTeam.miners > 0) {
  dayTeam.healers = 1;
  dayTeam.smithies = 2;
}

Voilà. Je fais pareil pour nightTeam juste en dessous. C’est mieux.

En descendant un peu plus bas, je remarque qu’il y a beaucoup de if impliquant dayTeam.miners > 0 et nightTeam.miners > 0. Je note dans un coin qu’on pourra réordonner ça et le rassembler, mais pour le moment je fais juste un scan de surface. Je continue donc :

const oldChiefGuard = nightTeam.guardManagers;

Pourquoi changer le nom ? guardManagers c’était bien, c’est comme ça qu’on en parle avec le métier. Donc on va éviter chiefGuard :

const oldGuardManagers = nightTeam.guardManagers;

Ok. On en reste là pour le moment. Commit : refactor: just a few surface fixes. Oh, et on lance les tests aussi. J’étais tellement confiant que j’ai commité sans vérifier. Ouf, ils sont verts. Si un jour je dois bosser longtemps sur le sujet, un petit husky pour m’empêcher de commit sans tester pourrait être bienvenu.

Les choses sérieuses commencent

Bon. Il y a un truc qui me rend fou depuis le début. C’est cette ligne.

composition.total = dayTeam.miners + dayTeam.washers + dayTeam.healers + dayTeam.smithies + dayTeam.innKeepers +
      nightTeam.miners + nightTeam.washers +  nightTeam.healers  + nightTeam.smithies  + nightTeam.innKeepers + nightTeam.guards + nightTeam.guardManagers + nightTeam.lighters;

Elle me hurle au visage qu’elle appartient à l’objet TeamComposition. Faisons le changement avec un getter :

export class TeamComposition {
  dayTeam: Team = new Team();
  nightTeam: Team = new Team();

  get total(): number {
    return this.dayTeam.miners + this.dayTeam.washers + this.dayTeam.healers + this.dayTeam.smithies + this.dayTeam.innKeepers +
      this.nightTeam.miners + this.nightTeam.washers +  this.nightTeam.healers  + this.nightTeam.smithies  + this.nightTeam.innKeepers + this.nightTeam.guards + this.nightTeam.guardManagers + this.nightTeam.lighters;
  }
}

Je lance les tests. Ils sont verts. Ils ne sont pas modifiés. Pour autant, ma modification n’est pas rétrocompatible. Je décide que c’est safe : soit on est derrière une API, et tout sera sérialisé, donc le setter n’est jamais utilisé. Soit quelqu’un utilise directement mon code, et appelle le setter, et je lui demanderai pourquoi. Je vérifierai avec le responsable fonctionnel quand même.

Commentaire à l’édition : vu comment je me suis démené pour rester 100% rétro-compatible, ce moment de faiblesse est un peu triste.

Bon, mais le code là haut me crie encore quelque chose. Que Team pourrait bénéficier d’avoir un total :

export class Team {
  // Tous les champs
  
  get total(): number {
    return this.miners + this.washers +  this.healers  + this.smithies  + this.innKeepers + this.guards + this.guardManagers + this.lighters; 
  }
}

export class TeamComposition {
  // Tous les champs

  get total(): number {
    return this.dayTeam.total + this.nightTeam.total;
  }
}

Les tests sont verts. Ma pression artérielle redescend. Je commit: refactor: move total into TeamComposition and Team

Réordonner pour extraire

J’ai toujours dans l’idée que je veux que la classe Team calcule elle-même sa composition. Mais pour savoir si c’est possible, il me faut réorganiser. Je vais prendre tous les if (dayTeam.miners >0) et les mettre ensemble :

if (dayTeam.miners > 0) {
  dayTeam.healers = 1;
  dayTeam.smithies = 2;
}

if (dayTeam.miners > 0) {
  dayTeam.innKeepers = Math.ceil((dayTeam.miners + dayTeam.healers + dayTeam.smithies) / 4) * 4;
  dayTeam.washers = Math.ceil((dayTeam.miners + dayTeam.healers + dayTeam.smithies + dayTeam.innKeepers) / 10);
}

Il n’y en a même pas tant que ça. Comme j’ai réordonné, je lance les tests. Verts. Je supprime le if désormais redondant

if (dayTeam.miners > 0) {
  dayTeam.healers = 1;
  dayTeam.smithies = 2;
  dayTeam.innKeepers = Math.ceil((dayTeam.miners + dayTeam.healers + dayTeam.smithies) / 4) * 4;
  dayTeam.washers = Math.ceil((dayTeam.miners + dayTeam.healers + dayTeam.smithies + dayTeam.innKeepers) / 10);
}

Toujours vert. Je vais pousser le vide un peu plus loin. J’ai if (nightTeam.miners >0) partout maintenant, je nettoie :

if (nightTeam.miners > 0) {
  nightTeam.healers = 1;
  nightTeam.smithies = 2;

  nightTeam.lighters = nightTeam.miners + 1;

  nightTeam.innKeepers = Math.ceil((nightTeam.miners + nightTeam.healers + nightTeam.smithies + nightTeam.lighters) / 4) * 4;
}

Tiens, la boucle while que je n’ai pas osé mentionner jusque là n’est pas dedans. Je vais la mettre dans le if voir si ça change quelque chose. C’est vert. J’ai maintenant deux blocs, un qui calcule l’équipe de jour, l’autre l’équipe de nuit.

J’ai envie de lancer un changement majeur, mais avant, commit. refactor: gather dayTeam and nightTeam computation

La course à l’héritage

Bon. Là, le système me crie à nouveau quelque chose. Team c’est pas mal, mais c’est un peu générique. Admettons qu’on ait une DayTeam et une NightTeam, elles pourraient calculer leur compo une fois qu’on leur a mis le nombre de mineurs. Allez, j’essaie. Par étapes :

export class DayTeam extends Team {
  compute() {
    if (this.miners > 0) {
      this.healers = 1;
      this.smithies = 2;
      this.innKeepers = Math.ceil((this.miners + this.healers + this.smithies) / 4) * 4;
      this.washers = Math.ceil((this.miners + this.healers + this.smithies + this.innKeepers) / 10);
    }
  }
}

Puis :

export class TeamComposition {
  dayTeam: DayTeam = new DayTeam();

Et enfin, dans la fonction tunnel :

const {dayTeam, nightTeam} = composition;

dayTeam.compute();

Vert. Les tests n’ont pas bougé. Tout est bon.

Je fais pareil pour la nuit :

export class NightTeam extends Team {
  compute() {
    if (this.miners > 0) {
      this.healers = 1;
      this.smithies = 2;

      this.lighters = this.miners + 1;

      this.innKeepers = Math.ceil((this.miners + this.healers + this.smithies + this.lighters) / 4) * 4;

      // eslint-disable-next-line no-constant-condition
      while (true) {
        const oldWashers = this.washers;
        const oldGuard = this.guards;
        const oldGuardManagers = this.guardManagers;

        this.washers = Math.ceil((this.miners + this.healers + this.smithies + this.innKeepers + this.lighters + this.guards + this.guardManagers) / 10);
        this.guards = Math.ceil((this.healers + this.miners + this.smithies + this.lighters + this.washers) / 3);
        this.guardManagers = Math.ceil((this.guards) / 3);

        if (oldWashers === this.washers && oldGuard === this.guards && oldGuardManagers === this.guardManagers) {
          break;
        }
      }
    }
  }
}

export class TeamComposition {
  dayTeam: DayTeam = new DayTeam();
  nightTeam: NightTeam = new NightTeam();

La fonction tunnel finit par :

const {dayTeam, nightTeam} = composition;

dayTeam.compute();
nightTeam.compute();

return composition;

Quel confort. Allez, je committe : refactor: extract DayTeam and NightTeam into subclasses - wip

Oui, Work In Progres. Je suis à peu près sûr que le reste va tourner autour de ça. De fait, le nombre de mineurs dépend lui aussi uniquement de la distance qui doit être creusée. Tentons déjà d’améliorer la boucle for :

// Miners
for (let i = 0; i < digPerRotation.length - 1; ++i) {
  if (digPerRotation[i] < distanceToDigPerDay) {
    composition.dayTeam.miners++;
  }
}

Pourquoi pas : 

composition.dayTeam.miners = digPerRotation.findIndex(length => length >= distanceToDigPerDay);

Tiens, tous les tests impliquant l’équipe de nuit sont rouges. C’est parce que si je ne trouve pas d’index qui matche, il me faut retourner l’équipe maximum. Or la fonction findIndex retourne -1 dans ce cas. L’idée était donc naze. Rollback. Ce n’est pas grave. Einstein lui même disait : « Une personne qui n’a jamais commis d’erreurs n’a jamais tenté d’innover »

Extraire vers compute ne m’arrange pas non plus, car je vais devoir passer trop d’infos. Je vais plutôt extraire dans des sous-fonctions et aviser ensuite : 

composition.dayTeam.miners = this.getDailyMiners(digPerRotation, distanceToDigPerDay);

 private getDailyMiners(digPerRotation: number[], distanceToDigPerDay: number) {
    let miners = 0;
    for (let i = 0; i < digPerRotation.length - 1; ++i) {
      if (digPerRotation[i] < distanceToDigPerDay) {
        miners++;
      }
    }
    return miners;
  }

J’aurais aimé virer la boucle for, mais je n’ai pas d’idée pour le moment.

Par contre, pour la boucle des mineurs de nuit, j’ai une idée. Rappel du code :

if (distanceToDigPerDay > maxDigPerRotation) {
  for (let i = 0; i < digPerRotation.length - 1; ++i) {
    if (digPerRotation[i] + maxDigPerRotation < distanceToDigPerDay) {
      composition.nightTeam.miners++;
    }
  }
}

En fait, c’est le même que celui des daily, avec un offset de maxDigPerRotation, et un if en plus. Sauf que mes tests de mutations m’ont indiqué que ce if, en fait, ne servait à rien :

Mes tests de mutations, qui me disent que le if est inutile
Remplacée par true, la condition ne casse pas les tests.

Du coup :

composition.dayTeam.miners = this.getDailyMiners(digPerRotation, distanceToDigPerDay);
composition.nightTeam.miners = this.getDailyMiners(digPerRotation, distanceToDigPerDay - maxDigPerRotation);

Ça ne coûte rien de tester. Et ça marche ! Du coup le nom de la fonction n’est plus bon : 

composition.dayTeam.miners = this.computeMiners(digPerRotation, distanceToDigPerDay);
composition.nightTeam.miners = this.computeMiners(digPerRotation, distanceToDigPerDay - maxDigPerRotation);

Là, l’horizon s’est ouvert. J’imagine déjà une TeamComposition qui prendrait en entrée le nombre de mineurs de chaque équipe et qui délèguerait tout aux Team. SAUF QUE ça tire la tronche au moment où je vais vouloir ajouter la nouvelle fonctionnalité, qui ajoute des protecteurs sous certaines conditions liées à l’API. Hé oui, je rappelle que si je fais tout ça, c’est pour insérer cette fonctionnalité sans douleur. « First make the change easy, then make the easy change », comme disait Kent Beck.

Du coup, restons en là à une petite retouche près :

const composition = new TeamComposition();
const {dayTeam, nightTeam} = composition;

dayTeam.miners = this.computeMiners(digPerRotation, distanceToDigPerDay);
nightTeam.miners = this.computeMiners(digPerRotation, distanceToDigPerDay - maxDigPerRotation);

dayTeam.computeOtherDwarves();
nightTeam.computeOtherDwarves();

J’ai remonté dayTeam et nightTeam. C’est plus clair. Quelque chose m’embête encore mais je tourne en rond pour l’instant. Je reviendrai dessus après. Pour l’instant commit, et on va voir une autre zone du code : refactor: clean up how miners are computed

Mon cerveau a chauffé en essayant d’entrevoir une solution pure, en vain. Je vais probablement trop loin. Le mieux est l’ennemi du bien. Du coup, je vais retourner sur des choses simples.

Validation et variables

Quelque chose de simple : 

const digPerRotation = this.rockInformation.get(rockType);
const maxDigPerRotation = digPerRotation[digPerRotation.length - 1];
const maxDigPerDay = 2 * maxDigPerRotation;

if (Math.floor(length) !== length || Math.floor(days) !== days || length < 0 || days < 0) {
  throw new InvalidFormatException();
}

devient assez facilement :

this.checkInputs(length, days);

const digPerRotation = this.rockInformation.get(rockType);
const maxDigPerRotation = digPerRotation[digPerRotation.length - 1];
const maxDigPerDay = 2 * maxDigPerRotation;

Il y a une meilleure syntaxe pour maxDigPerRotation :

const maxDigPerRotation = digPerRotation.at(-1)  || 0;

Étonnament eslint râle que la valeur retournée peut désormais être nulle. Comme si la syntaxe précédente ne présentait pas ce risque… tant pis, je deviens défensif.

Quant à maxDigPerDay, elle n’est là que pour s’assurer d’une exception. Je vais l’inliner

if (distanceToDigPerDay > NB_ROTATIONS_PER_DAY * maxDigPerRotation) {
  throw new TunnelTooLongForDelayException();
}

Il est 23h45 et quelque chose me chiffone toujours. Je crois que le plus sage, c’est d’aller dormir. Je suis probablement en plein effet tunnel et je n’entrevois pas de solution qui me satisfasse. La nuit porte conseil.

Vers un code plus lisible ?

À la réflexion, un petit objet me permettant de transporter toutes les données du problème serait sûrement le bienvenu. Mais je vais commiter avant, au cas où : refactor: data validation

Ce qui m’embêtait jusque là, c’était de transporter les paramètres, je m’en rends compte maintenant. Extrayons ça :

export class DiggingInfo {
  readonly distanceToDigPerDay;
  readonly maxDiggingDistancePerRotation;
  constructor(readonly lengthToDig: number, readonly daysAvailable: number, digPerDwarfPerRotation: number[]) {
    this.distanceToDigPerDay = Math.floor(lengthToDig / daysAvailable);
    this.maxDiggingDistancePerRotation = digPerDwarfPerRotation.at(-1) || 0;
  }
}

Tunnel commence désormais par :

 tunnel(length: number, days: number, rockType: string): TeamComposition {
    this.checkInputs(length, days);
    const diggingInfo = new DiggingInfo(length, days, this.rockInformation.get(rockType);)

Il ne me reste plus qu’à remplacer chaque appel de variable par un appel à diggingInfo pour enlever les variables qu’on avait jusque là.

tunnel(length: number, days: number, rockType: string): TeamComposition {
  this.checkInputs(length, days);
  const diggingInfo = new DiggingInfo(length, days, this.rockInformation.get(rockType))

  if (diggingInfo.distanceToDigPerDay > NB_ROTATIONS_PER_DAY * diggingInfo.maxDiggingDistancePerRotation) {
    throw new TunnelTooLongForDelayException();
  }

  const composition = new TeamComposition();
  const {dayTeam, nightTeam} = composition;

  dayTeam.miners = this.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay);
  nightTeam.miners = this.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay - diggingInfo.maxDiggingDistancePerRotation);

  dayTeam.computeOtherDwarves();
  nightTeam.computeOtherDwarves();

  return composition;
}

Ok. Bon, là, je suis capable de générer mes équipes en prenant un seul objet en entrée, que je pourrais étendre plus tard. Maintenant, c’est l’heure de faire de l’objet.

TeamComposition au pouvoir

J’ai eu peur de casser ma rétro-compatibilité depuis le début, mais le fait est que les objets que j’expose sont là pour être lus, pas créés. Je vais donc reprendre la main sur leur création. C’est TeamComposition qui va orchestrer.

export class TeamComposition {
  constructor(readonly dayTeam = new DayTeam(), readonly nightTeam = new NightTeam()) {
  }

  get total(): number {
    return this.dayTeam.total + this.nightTeam.total;
  }
}

Jusque là, je n’ai rien cassé. Mon plan, c’est de rendre disponible une méthode statique permettant de tout créer depuis les DiggingInfo. Essayons voir :

static fromDiggingInfo(diggingInfo: DiggingInfo): TeamComposition {
  const dayTeam = DayTeam.fromDiggingInfo(diggingInfo);
  const nightTeam = NightTeam.fromDiggingInfo(diggingInfo);

  return new TeamComposition(dayTeam, nightTeam);
}

Évidemment, fromDiggingInfo n’est pas définie dans les Team. Faisons ça :

export class DayTeam extends Team {
  static fromDiggingInfo(diggingInfo: DiggingInfo): DayTeam {
    const team = new DayTeam()
    team.computeMiners(diggingInfo.distanceToDigPerDay);
    team.computeOtherDwarves();
    return team;
  }

  computeOtherDwarves() {
    if (this.miners > 0) {
      this.healers = 1;
      this.smithies = 2;
      this.innKeepers = Math.ceil((this.miners + this.healers + this.smithies) / 4) * 4;
      this.washers = Math.ceil((this.miners + this.healers + this.smithies + this.innKeepers) / 10);
    }
  }
}

Je l’ai déjà quelque part computeMiners ! Je n’ai qu’à la remonter dans Team et le tour est joué ! Pour ne rien casser pour le moment, je vais juste la dupliquer.

export class Team {
  // Rest of code
  // ----
  
  protected computeMiners(digPerRotation: number[], distanceToDigPerDay: number) {
    let miners = 0;
    for (let i = 0; i < digPerRotation.length - 1; ++i) {
      if (digPerRotation[i] < distanceToDigPerDay) {
        miners++;
      }
    }
    return miners;
  }
}

Du coup pour la nuit, c’est pareil :

static fromDiggingInfo(diggingInfo: DiggingInfo): NightTeam {
  const team = new NightTeam()
  team.miners = team.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay - diggingInfo.maxDiggingDistancePerRotation);
  team.computeOtherDwarves();
  return team;
}

Et du coup, le bloc de code dans ma méthode tunnel :

const composition = new TeamComposition();
const {dayTeam, nightTeam} = composition;

dayTeam.miners = this.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay);
nightTeam.miners = this.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay - diggingInfo.maxDiggingDistancePerRotation);

dayTeam.computeOtherDwarves();
nightTeam.computeOtherDwarves();

return composition;

Devient :

return TeamComposition.fromDiggingInfo(diggingInfo);

Je lance les tests… c’est vert ! Un peu de ménage sur les fonctions non utilisées. Et commit : refactor: move logic to TeamComposition and Teams

Le dernier cleanup

On va reprendre morceau par morceau pour juger. D’abord, mon DiggingEstimator :

export class DiggingEstimator {
  private readonly rockInformation: RockInformationInterface;

  constructor(rockInformation?: RockInformationInterface) {
    this.rockInformation = rockInformation || new VinRockInformationService()
  }

  tunnel(length: number, days: number, rockType: string): TeamComposition {
    this.checkInputs(length, days);

    const diggingInfo = new DiggingInfo(length, days, this.rockInformation.get(rockType))

    if (diggingInfo.distanceToDigPerDay > NB_ROTATIONS_PER_DAY * diggingInfo.maxDiggingDistancePerRotation) {
      throw new TunnelTooLongForDelayException();
    }

    return TeamComposition.fromDiggingInfo(diggingInfo);
  }

  private checkInputs(length: number, days: number) {
    if (Math.floor(length) !== length || Math.floor(days) !== days || length < 0 || days < 0) {
      throw new InvalidFormatException();
    }
  }
}

J’ai une différence de niveau d’abstraction à la ligne 13. J’ajuste en testant l’exception séparément.

tunnel(length: number, days: number, rockType: string): TeamComposition {
  this.checkInputs(length, days);

  const diggingInfo = new DiggingInfo(length, days, this.rockInformation.get(rockType))

  this.checkTunnelCanBeDugInRequestedTime(diggingInfo);

  return TeamComposition.fromDiggingInfo(diggingInfo);
}


private checkTunnelCanBeDugInRequestedTime(diggingInfo: DiggingInfo) {
  if (diggingInfo.distanceToDigPerDay > NB_ROTATIONS_PER_DAY * diggingInfo.maxDiggingDistancePerRotation) {
    throw new TunnelTooLongForDelayException();
  }
}

Ça me convient bien. Regardons TeamComposition :

export class TeamComposition {
  constructor(readonly dayTeam = new DayTeam(), readonly nightTeam = new NightTeam()) {
  }

  get total(): number {
    return this.dayTeam.total + this.nightTeam.total;
  }

  static fromDiggingInfo(diggingInfo: DiggingInfo): TeamComposition {
    const dayTeam = DayTeam.fromDiggingInfo(diggingInfo);
    const nightTeam = NightTeam.fromDiggingInfo(diggingInfo);

    return new TeamComposition(dayTeam, nightTeam);
  }
}

Je me demande si je peux changer le type de dayTeam et nightTeam, pour ne pas exposer de nouvel objet.

export class TeamComposition {
  constructor(readonly dayTeam: Team = new DayTeam(), readonly nightTeam: Team = new NightTeam()) {
  }

Le compilo est d’accord. Les tests aussi. Parfait. Du coup Team ressemble à :

export class Team {
  miners = 0;
  healers = 0;
  smithies = 0;
  lighters = 0;
  innKeepers = 0;
  guards = 0;
  guardManagers = 0;
  washers = 0;

  get total(): number {
    return this.miners + this.washers +  this.healers  + this.smithies  + this.innKeepers + this.guards + this.guardManagers + this.lighters;
  }

  protected computeMiners(digPerRotation: number[], distanceToDigPerDay: number) {
    let miners = 0;
    for (let i = 0; i < digPerRotation.length - 1; ++i) {
      if (digPerRotation[i] < distanceToDigPerDay) {
        miners++;
      }
    }
    return miners;
  }

}

Suffisamment clair. DayTeam : 

export class DayTeam extends Team {
  static fromDiggingInfo(diggingInfo: DiggingInfo): DayTeam {
    const team = new DayTeam()
    team.miners = team.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay);
    team.computeOtherDwarves();
    return team;
  }

  computeOtherDwarves() {
    if (this.miners > 0) {
      this.healers = 1;
      this.smithies = 2;
      this.innKeepers = Math.ceil((this.miners + this.healers + this.smithies) / 4) * 4;
      this.washers = Math.ceil((this.miners + this.healers + this.smithies + this.innKeepers) / 10);
    }
  }
}

Toutes les règles sont là, au bon endroit. Quant à NightTeam :

export class NightTeam extends Team {
  static fromDiggingInfo(diggingInfo: DiggingInfo): NightTeam {
    const team = new NightTeam()
    team.miners = team.computeMiners(diggingInfo.digPerDwarfPerRotation, diggingInfo.distanceToDigPerDay - diggingInfo.maxDiggingDistancePerRotation);
    team.computeOtherDwarves();
    return team;
  }
  
  computeOtherDwarves() {
    if (this.miners > 0) {
      this.healers = 1;
      this.smithies = 2;

      this.lighters = this.miners + 1;

      this.innKeepers = Math.ceil((this.miners + this.healers + this.smithies + this.lighters) / 4) * 4;

      // eslint-disable-next-line no-constant-condition
      while (true) {
        const oldWashers = this.washers;
        const oldGuard = this.guards;
        const oldGuardManagers = this.guardManagers;

        this.washers = Math.ceil((this.miners + this.healers + this.smithies + this.innKeepers + this.lighters + this.guards + this.guardManagers) / 10);
        this.guards = Math.ceil((this.healers + this.miners + this.smithies + this.lighters + this.washers) / 3);
        this.guardManagers = Math.ceil((this.guards) / 3);

        if (oldWashers === this.washers && oldGuard === this.guards && oldGuardManagers === this.guardManagers) {
          break;
        }
      }
    }
  }
}

J’ai une duplication de code sur les soigneurs et les forgerons, mais elle rend l’intention explicite, je garde comme ça. Par contre je ne me suis pas attaqué au bloc du bas. Les spécs le montrent, le nombre de gardes impacte le nombre de chefs des gardes, ce qui impacte le nombre de laveurs de vaisselles, ce qui à leur tour impacte le nombre de gardes… d’où ce bout de code qui pique un peu les yeux.

Mathématiquement, ça semble converger au bout de deux itérations quoiqu’il arrive. Si le nombre de washers doit augmenter parce qu’on ajoute des gardes et des managers, il n’augmentera jamais le coup suivant (il faudrait rajouter 10 nains). Je n’ai pas écrit la preuve formelle sur papier, mais j’ai mieux, j’ai des tests que je sais exhaustifs :

private computeDependantDwarves() {
  let iterationsNeededToStabilizeCount = 2;
  while (iterationsNeededToStabilizeCount-- > 0) {
    this.washers = Math.ceil((this.miners + this.healers + this.smithies + this.innKeepers + this.lighters + this.guards + this.guardManagers) / 10);
    this.guards = Math.ceil((this.healers + this.miners + this.smithies + this.lighters + this.washers) / 3);
    this.guardManagers = Math.ceil((this.guards) / 3);
  }
}

Il y avait sûrement des moyens plus court de le faire (enlever la boucle while et appeler deux fois computeDependantDwarves), mais si je reprends le code dans trois mois, je perdrai l’expressivité. Je préfère cette formulation.

Pour finir, je n’ai pas osé bouger TeamComposition et Team, par contre je vais bouger toutes les autres classes dans leur fichier, vu qu’elles sont privées. Ah, non, je ne peux pas, ça crée une dépendance circulaire (le DiggingEstimator a besoin de Dayteam, mais Dayteam hérite de Team, qui est au même endroit que DiggingEstimator). Je vais me contenter d’enlever les export pour que personne ne se branche dessus. Mais je devrais vraiment vérifier qui sont mes clients.

Commit : refactor: cleaning up few things, and make NightTeam easier to understand.

Allez, un tout dernier…

J’en peux vraiment plus de toutes ces classes dans le même fichier. Je vais essayer un truc. J’exporte tout le monde dans son fichier, dans le dossier model. Évidemment, les tests sont impactés :

import { TeamComposition } from "./model/team-composition";

Mais dans digging-estimator.ts, je peux ajouter :

export { Team } from "./model/team";
export { TeamComposition } from "./model/team-composition";

Je rollback les tests qui ont été impactés par mon refacto, et… ça marche ! Vite, commit: refactor: split into files

Conclusion

Ce refacto était long, et j’ai avancé à tâtons. J’avais une idée globale de l’endroit où je voulais aller, mais la rétro-compatibilité m’a posé quelques soucis. La solution n’est pas parfaite (j’aurais aimé pouvoir rendre les constructeurs private), mais suffisante pour accueillir le changement à venir. C’est l’heure de passer à l’étape suivante : l’ajout de fonctionnalité.

Étiquettes: , ,

Laisser un commentaire