The CanardFactory audit

CanardFactory is a fictional company that hires and fires constantly, does not always manage to produce its rubber ducks, and whose code was written on the principle “we'll see later”.

Here is its main file, in full. Note every flaw you find in it before opening the answers. The goal of the module is that you never write code of this kind.

The file

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; // nothing
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); } // English forbidden
if(quoi=="canard"){ ((Object)quoi).toString(); } // meh
if(num/0==1){ System.out.println("math"); } // does it work?
}

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--){
 // whatever
 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; // yolo cast
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; // all the same
}
public int hashCode(){
return 1;
}
protected void finalize(){ System.out.println("fin?"); }
// end of the world
static { System.out.println("STATIC"); trucs.add(123); m.put(123,"x"); }
}

The answers, slice by slice

Slice 1: the header, lines 1 to 11

• No indentation, in the whole file: you can see neither where a method starts nor where it ends.
• A badly formed class name (Canardusine) and an inconsistent package.
• Wildcard import java.util.*, which brings in everything and hides where the types come from.
• Raw types List and Map, with no type parameter. Chapter 12.
• Mutable public static fields: global state, accessible and modifiable by anyone. Chapter 7.
• Non-final constants: N, statut and r can be modified at any time.
• A main that delegates to a method named l0.

Slice 2: the body, lines 12 to 20

• The parameter a reassigned: the caller does not see this change, since Java passes the reference by value, but in the rest of the method a no longer refers to what it received. With no argument, the program makes up its own command line.
• a[3] assumes four arguments: with one, two or three, the result is an ArrayIndexOutOfBoundsException.
• a[3] == "embauche": string comparison by reference. Here, it is true by chance, because both strings are literals, which Java stores only once. If the user types embauche on the command line, it becomes false. Chapter 11.
• licencie(-3): a negative number of dismissals makes no sense, and no precondition forbids it.
• Magic numbers (999) and a global N.
• Cryptic method names, no contract, no error handling.

Slice 3: the ignored exception, lines 21 to 30

• Catch-all catch(Exception), together with a lying message: "ok". This is the starting situation of sequence 7. Chapter 9.
• String comparison with ==, again, twice.
• Five dead variables, declared and never used.
• while(y<combien){ y--; }: y moves further from the bound at each iteration, hence an infinite loop as soon as combien is 2 or more.
• A randomly drawn identifier, and therefore impossible to trace.
• Arbitrary conditional recursion: l2 calls itself for one particular value, with no justification.

Slice 4: the manufacturing, lines 31 to 40

• Hidden side effect: l2, which manufactures, also fires people when combien is negative.
• Object everywhere: impossible to reason about the contents of trucs.
• Inconsistent keys and values in the same Map: sometimes object to string, sometimes string to object.
• statut = null for one particular value: no exception is thrown, because == causes no error on null. The program would print PANIC:null and carry on in an inconsistent state. Moreover, with this program's values, num is never 42: this code is never executed, although it looks as if it were.
• Dots printed by way of a log.

Slice 5: the division, lines 41 to 52

• Mixed languages in the same file.
• String comparison with ==, still.
• num/0: the division is evaluated before the comparison, and the ArithmeticException is thrown whether the test is true or false; the if does not protect the evaluation of its own condition. The program therefore stops there, at the first duck, and everything after it is never executed.
• HR policy in the business class: a single class carries two responsibilities. Chapter 8.
• Hiring triggers dismissals, which produces a cascade of side effects.

Slice 6: the dismissal loop, lines 53 to 63

• for(int i=0; i<n; i--): i decreases, so the condition stays true. The loop only stops when the Map is empty and iterator().next() throws NoSuchElementException.
• A key removed at random: keySet().iterator().next() returns the first key in an order that HashMap does not guarantee. Moreover, the same Map contains both ducks and employees: a dismissal can delete a duck.
• System.err for business logging: there is no logging strategy.
• statut reassigned arbitrarily, at the end of embauche as at the end of licencie.

Slice 7: the unchecked cast, lines 64 to 73

• Unchecked cast (String) o, without instanceof, with a risk of ClassCastException, since fabriquer passes it a new Object(). Chapter 6.
• s.length() called before the s == null test: the NullPointerException comes before the check meant to avoid it.
• s.equals("CANARD") the wrong way round: "CANARD".equals(s) causes no error on null.

Slice 8: equality and the `static` block, lines 74 to 83

• equals always true and constant hashCode: all objects are equal and fall into the same bucket, so no hash-based collection works correctly. Chapter 11.
• finalize(): deprecated since Java 9, with side effects during garbage collection.
• static block that modifies the global state as soon as the class is loaded, before any call: that is why STATIC is the first line printed.

Summary

The corresponding chapters

Each of these flaws is dealt with in a chapter of the lecture notes:

Exercise with a coding assistant

Give this file to a coding assistant and ask it for a review. Then compare its list with the answers above, looking for what it missed.

This exercise sums up the whole module. An assistant quickly spots the flaws that have a name. It more often misses those that require knowing what the program is supposed to do, and those that fit in a single, normal-looking line.

Memo: code smells

• Global state? Raw types? == on strings?
• Swallowed exceptions? Loops without a clear bound?
• Cryptic naming? Mixed responsibilities?
• Inconsistent equals and hashCode? Unusable log?