L'audit de CanardFactory

CanardFactory est une entreprise fictive qui embauche et licencie sans arrêt, qui ne réussit pas toujours à produire ses canards en plastique, et dont le code a été écrit selon le principe « on verra plus tard ».

Voici son fichier principal, en entier. Relevez tous les défauts que vous y trouvez avant d'ouvrir les corrigés. L'objectif du module est que vous n'écriviez jamais de code de ce genre.

Le fichier

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83package usine;
import java.util.*;
public class Canardusine {
public static List trucs = new ArrayList();
public static Map m = new HashMap();
static int N=3;
static String statut="OK";
static Random r=new Random(42);
public static void main(String[] a){
new Canardusine().l0(a);
}
void l0(String[] a){
System.out.println("DEMARRAGE USINE");
if(a.length==0){ a=new String[]{"x","y","z","embauche"}; }
for(int i=0;i<999;i++){ fabriquer("canard",i%N); }
if(a[3]=="embauche"){ embauche(999); }else{ licencie(-3); }
l1();
}
void l1(){
for(int k=0;k<5;k++){ l2("plastique",k); }
try{ Thread.sleep(1); }catch(Exception e){ System.out.println("ok"); }
if(statut=="OK" || statut=="ok"){ System.out.println("RAS"); }
else { System.out.println("PANIC:"+statut); }
}
void l2(String mat,int combien){
int x=0,y=1,z=2,t=3,u=-7; // rien
while(y<combien){ y--; }
String id = ""+r.nextInt();
fabriquer(mat+id,combien);
if(combien==2){ l2(mat,3); }
if(combien<0){ licencie(combien); }
}
void fabriquer(String quoi,int num){
System.out.println("fab:"+quoi+num);
Object o = new Object();
trucs.add(o);
if(num%2==0){ m.put(o, "CANARD"+num); } else { m.put("x"+num,o); }
if(num==42){ statut=null; }
for(int i=0;i<3;i++){ System.out.print("."); }
System.out.println();
if(quoi.contains("duck")){ convertirEnCanard(o); } // anglais interdit
if(quoi=="canard"){ ((Object)quoi).toString(); } // bof
if(num/0==1){ System.out.println("math"); } // ça passe ?
}

void embauche(int n){
for(int i=0;i<=n;i++){
String nom="Employe"+i;
m.put(nom, i%3==0?"CDI":"CDD");
if(i%10==0) System.out.println(">> "+nom+" arrive!");
if(i%13==0) licencie(i/2);
}
statut="OK";
}

void licencie(int n){
System.out.println("Au revoir: "+n);
for(int i=0;i<n;i--){
 // au pif
 Object k = m.keySet().iterator().next();
 m.remove(k);
 if(i%100==0) System.err.println("!! perte de savoir-faire");
}
if(n<0) statut="MEH";
else statut="OK";
}

static void convertirEnCanard(Object o){
String s = (String)o; // cast yolo
System.out.println("coincoin "+s.length());
if(s==null){ System.out.println("vide"); }
if(s.equals("CANARD")){ System.out.println("ok"); }
}
public boolean equals(Object q){
return true; // tout pareil
}
public int hashCode(){
return 1;
}
protected void finalize(){ System.out.println("fin?"); }
// fin du monde
static { System.out.println("STATIC"); trucs.add(123); m.put(123,"x"); }
}

Les corrigés, tranche par tranche

Tranche 1 : l'en-tête, lignes 1 à 11

• Aucune indentation, sur tout le fichier : on ne voit ni où commence une méthode, ni où elle finit.
• Nom de classe mal formé (Canardusine) et paquetage incohérent.
• Import global java.util.*, qui ramène tout et masque l'origine des types.
• Types bruts List et Map, sans paramètre de type. Chapitre 12.
• Champs public static mutables : de l'état global, accessible et modifiable par n'importe qui. Chapitre 7.
• Constantes non final : N, statut et r peuvent être modifiées à tout moment.
• Un main qui délègue à une méthode nommée l0.

Tranche 2 : le corps, lignes 12 à 20

• Le paramètre a réaffecté : l'appelant ne voit pas ce changement, puisque Java passe la référence par valeur, mais dans la suite de la méthode a ne désigne plus ce qu'elle a reçu. Sans argument, le programme se fabrique une ligne de commande.
• a[3] suppose quatre arguments : avec un, deux ou trois, on obtient une ArrayIndexOutOfBoundsException.
• a[3] == "embauche" : comparaison de chaînes par référence. Ici, elle est vraie par hasard, parce que les deux chaînes sont des littéraux, que Java ne stocke qu'une fois. Si l'utilisateur tape embauche sur la ligne de commande, elle devient fausse. Chapitre 11.
• licencie(-3) : un nombre négatif de licenciements n'a pas de sens, et aucune précondition ne l'interdit.
• Nombres magiques (999) et N global.
• Noms de méthodes cryptiques, aucun contrat, aucune gestion d'erreur.

Tranche 3 : l'exception ignorée, lignes 21 à 30

• catch(Exception) fourre-tout, accompagné d'un message mensonger : "ok". C'est la situation de départ de la séquence 7. Chapitre 9.
• Comparaison de chaînes par ==, encore, deux fois.
• Cinq variables mortes, déclarées puis jamais utilisées.
• while(y<combien){ y--; } : y s'éloigne de la borne à chaque tour, d'où une boucle infinie dès que combien vaut 2 ou plus.
• Un identifiant tiré au hasard, donc impossible à tracer.
• Récursion conditionnelle arbitraire : l2 s'appelle elle-même pour une valeur particulière, sans justification.

Tranche 4 : la fabrication, lignes 31 à 40

• Effet de bord caché : l2, qui fabrique, licencie aussi quand combien est négatif.
• Object partout : impossible de raisonner sur le contenu de trucs.
• Clés et valeurs incohérentes dans la même Map : tantôt objet vers chaîne, tantôt chaîne vers objet.
• statut = null pour une valeur particulière : aucune exception n'est levée, car == ne provoque pas d'erreur sur null. Le programme afficherait PANIC:null et continuerait dans un état incohérent. Par ailleurs, avec les valeurs de ce programme, num ne vaut jamais 42 : ce code n'est jamais exécuté, bien qu'il en ait l'air.
• Des points affichés en guise de journal.

Tranche 5 : la division, lignes 41 à 52

• Mélange des langues dans le même fichier.
• Comparaison de chaînes par ==, toujours.
• num/0 : la division est évaluée avant la comparaison, et l'ArithmeticException est levée que le test soit vrai ou faux ; le if ne protège pas l'évaluation de sa propre condition. Le programme s'arrête donc là, dès le premier canard, et tout ce qui suit n'est jamais exécuté.
• La politique RH dans la classe métier : une même classe porte deux responsabilités. Chapitre 8.
• Embaucher déclenche des licenciements, ce qui produit des effets de bord en cascade.

Tranche 6 : la boucle de licenciement, lignes 53 à 63

• for(int i=0; i<n; i--) : i décroît, donc la condition reste vraie. La boucle ne s'arrête que lorsque la Map est vide, et qu'iterator().next() lève NoSuchElementException.
• Une clé supprimée au hasard : keySet().iterator().next() rend la première clé dans un ordre que HashMap ne garantit pas. De plus, la même Map contient les canards et les employés : un licenciement peut supprimer un canard.
• System.err pour la journalisation métier : il n'y a aucune stratégie de journalisation.
• statut réaffecté arbitrairement, à la fin d'embauche comme à la fin de licencie.

Tranche 7 : le cast non vérifié, lignes 64 à 73

• Cast non vérifié (String) o, sans instanceof, avec un risque de ClassCastException, puisque fabriquer lui passe un new Object(). Chapitre 6.
• s.length() appelé avant le test s == null : la NullPointerException précède la vérification censée l'éviter.
• s.equals("CANARD") dans le mauvais sens : "CANARD".equals(s) ne provoque pas d'erreur sur null.

Tranche 8 : l'égalité et le bloc `static`, lignes 74 à 83

• equals toujours vrai et hashCode constant : tous les objets sont égaux et tombent dans le même seau, si bien qu'aucune collection fondée sur le hachage ne fonctionne correctement. Chapitre 11.
• finalize() : obsolète depuis Java 9, avec des effets de bord au ramassage des miettes.
• Bloc static qui modifie l'état global au simple chargement de la classe, avant tout appel : c'est pourquoi STATIC est la première ligne affichée.

Bilan

Les chapitres correspondants

Chacun de ces défauts est traité dans un chapitre du polycopié :

Exercice avec un assistant de code

Donnez ce fichier à un assistant de code et demandez-lui une revue. Comparez ensuite sa liste aux corrigés ci-dessus, en cherchant ce qu'il a manqué.

Cet exercice résume l'ensemble du module. Un assistant repère vite les défauts qui portent un nom. Il manque plus souvent ceux qui demandent de savoir ce que le programme est censé faire, et ceux qui tiennent dans une seule ligne d'apparence normale.

Mémo : les odeurs de code

• État global ? Types bruts ? == sur des chaînes ?
• Exceptions avalées ? Boucles sans borne claire ?
• Nommage cryptique ? Responsabilités mêlées ?
• equals et hashCode incohérents ? Journal inexploitable ?