Je vais aujourd’hui profiter de cet article pour vous faire un retour d’expérience sur des coding horrors que j’ai pu rencontrer lors de ma carrière. Pour les amateurs du genre, vous avez le site the Daily WTF notamment.
Débloque les + belles offres tech en 10 mins
Nommage des tables en base de données
Une des applications sur lesquelles j’ai été amené à travailler avait décidé de nommer ses tables sur deux ou trois lettres. Sauf que, avec un tel système, on s’était retrouvé notamment avec une table nommée for et une autre nommée int. Alors ça avait marché au moment de la création de l’application car c’était sur MySQL 4 qui ne supportait pas les procédures stockées, mais un jour ils ont voulu passer le tout en 5. Curieusement ça n’a pas posé de problème directement.
Et alors, pourquoi c’est mal ?
Quiconque a programmé un minimum et notamment des procédures SQL aura tout de suite vu que bon, for et int sont des éléments du langage SQL, bref j’évite tout commentaire là-dessus. 😀 Et puis bon pour les tables il vaut mieux rendre les noms assez explicites, même si des fois on est limité en nombre de caractères, ça rend l’application plus lisible.
Aller délibérément contre une convention de codage majeure de Java
Certaines entreprises décident d’aller délibérément contre une ou plusieurs conventions de codage majeures de Java. Je ne parle pas ici de l’ordre conventionnel entre les mot-clefs static et final, même s’il existe, mais de trucs bien plus bas niveau que ça. Ainsi j’ai travaillé dans une entreprise où ils avait décidé que tous les noms de leur méthodes devaient commencer par des majuscules, comme en C#.
Et alors, pourquoi c’est mal ?
Bon déjà ça crée des soucis vis à vis des nouveaux arrivants dans l’entreprise qui en sont pas habitués à de telles conventions. Mais pire, des outils d’analyse de code comme l’excellent FindBugs ou Sonar partent du principe que vous avez respecté ces conventions et génèreront des dizaines de warnings si ce n’est pas le cas. Alors certes on peut les masquer mais bon, des fois ces filtres sont un peu trop agressifs et masquent de vraies erreurs. Et s’il n’y a pas de masquage, ces pratiques génèrent du « bruit » dans ces outils ce qui n’aide pas à retrouver les vrais problèmes.
Débloque les + belles offres tech en 10 mins
Certains cas de break dans des boucles for
Il m’est arrivé au cours de ma carrière de tomber sur un bout de code comme ça :
label:
for (int i = 0; i < 10; i++) {
// bla bla
for (int j = 0; j < 100; j++) {
// bla bla
if (condition) {
break label;
}
// bla bla
}
}
Pour ceux qui ne connaîtraient pas ce type de construction c’est expliqué ici, mais pour dire simplement si on rentre dans le break ici on sort directement de la boucle for désignée par label.
Et alors, pourquoi c’est mal ?
Déjà pour celui qui n’a jamais vu ça la syntaxe est déroutante et fait penser à une boucle infinie où on retournerait directement à label pour reprendre la boucle for (int i = 0; i < 10; i++). Mais bon ensuite le souci est qu’on peut vite arriver à faire du code spaghetti avec des labels imbriqués et des for correspondants, un peu comme avec le sinistre goto du langage BASIC. D’ailleurs pour la petite histoire le mot-clef goto existe en Java mais n’a jamais été implémenté, précisément pour cette raison.
Utilisation incorrecte de la généricité
Dans une entreprise dans laquelle j’ai eu l’occasion de travailler, à un moment on avait deux classes à gérer, BasicSegment et InternalSegment. Il se trouve que la deuxième héritait de la première. On avait aussi le code de la classe SegmentGroup qui était le suivant :
public class SegmentGroup<E extends BasicSegment> {
private List<E> segments;
// bla bla
public void addSegment(E segment) {
// bla bla
}
public void checkSegment(final InternalSegment segment) {
for (E seg: segments) {
if (seg instanceof InternalSegment) {
// bla bla
}
}
}
}
Et alors, pourquoi c’est mal ?
L’intérêt de la généricité, c’est justement de pouvoir typer un conteneur d’objets en fonction d’un paramètre. Là il se trouve qu’on peut typer une instance de SegmentGroup mais on se rend compte que celle-ci gère des traitements particuliers en fonction du type d’instance de BasicSegment contenu, bref on perd tout l’intérêt de la généricité.
Débloque les + belles offres tech en 10 mins
Utilisation incorrecte de classes anonymes
Dans certaines applications, on retrouve un code de ce type à des endroits très souvent appelés :
List l = new ArrayList();
// bla bla
CollectionUtils.filter(l, new Predicate() {
public boolean evaluate(Object o) {
String s = (String) o;
return s.startsWith("hello");
}
});
// bla bla
Je l’avoue, ici c’est déjà plus subtil, et le point est que le bout de code mentionné ci-dessus se retrouve à être très souvent appelé. Vous ne voyez toujours pas ? Allez, je vous aide, regardez donc comment fonctionne un GC.
Et alors, pourquoi c’est mal ?
La réponse est qu’à chaque fois qu’on exécutera ce code, on va instancier un nouveau Predicate, lequel ne dépend absolument pas de l’état d’une variable déterminée plus haut dans le code. On se retrouve donc à instancier un objet inutilement, celui-ci pouvant parfaitement être statique ou en singleton. Et quand on instancie des objets inutilement dans une application, ça accélère le « vieillissement » des autres objets au niveau du GC, et à terme on augmente le beson de l’application en full GC, ce qui n’est pas bon du tout. Et ce d’autant plus si le code en question est appelé souvent. Alors, convaincu ? 😉
Ne pas utiliser de vrai constructeur dans les objets
La norme JavaBean explicite que les POJOs doivent avoir un constructeur par défaut, et des getters/setters sur leurs valeurs pouvant être lues et positionnées. Le problème est qu’après on peut avoir du code comme ceci :
MyObj monObj = new MyObj();
monObj.setPropertyA(...);
monObj.setPropertyB(...);
monObj.setPropertyC(...);
// Bla bla bla
Et alors, pourquoi c’est mal ?
Déjà si un objet a besoin qu’un certain nombre de champs soit initialisés lors de son instanciation, ici toute personne qui veut l’instancier se retrouve à invoquer pas mal de setters, et d’autre part il n’y a pas moyen de valider que les champs devant être initialisés le sont réellement. D’autre part, cela complique les tests unitaires de l’objet, car il faudra aussi écrire toutes ces lignes d’initialisation dans les tests. Enfin, on casse l’encapsulation de ces objets. De manière générale les setters des POJO en particulier sont faits pour être appelés par les librairies du type Hibernate ou JAXB. Pour le développeur, il convient d’utiliser les constructeurs « normaux » en faisant comme si ces setters n’existaient pas. Je ne dis pas que de temps en temps ils ne sont pas nécessaires, juste que bon, initialiser un objet comme dans l’exemple ça va en C (et encore…) mais certainement pas dans un langage objet comme Java.
Des méthodes très très longues, trop longues
Dans une application en particulier sur laquelle j’ai été amené à travailler, on avait un paquet de méthodes qui dépassaient allègrement le millier de lignes de codes, dont certaines utilisaient la réflexivité intensivement. La complexité cyclomatique de chacune d’entre elles atteignait facilement les 50. Non je vous rassure, je n’ai rien codé de tel dans ma vie. 😉
Et alors, pourquoi c’est mal ?
Coder de telles méthodes est un bon plan pour le gars qui veut verrouiller son poste. Plus sérieusement, maintenir de tels trucs est totalement impossible, sauf peut-être pour son auteur, et encore…
Débloque les + belles offres tech en 10 mins
equals() et hashcode()
En parcourant GitHub, je suis tombé sur ceci (j’ai anonymisé le code) :
public class BadObject {
private String name;
public BadObject(String name) {
this.name = name;
}
@Override
public boolean equals(Object obj) {
boolean equality = false;
if (obj instanceof BadObject) {
BadObject other = (BadObject) obj;
if ((name != null) && name.equals(other.name)) {
equality = true;
}
}
return equality;
}
/**
* Getter for the field {@code name}
* @return the name
*/
public String getName() {
return name;
}
/**
* Setter for the field {@code name}
* @param name the name to set
*/
public void setName(String name) {
this.name = name;
}
}
Et alors, pourquoi c’est mal ?
Le equals est très mal codé ici, on a en effet new BadObject(null).equals(new BadObject(null)) qui retourne false. Mais surtout le pire est que le hashCode() manque ici, or si le equals est redéfini il faut toujours redéfinir le hashCode(), cf. l’explication ici et le fait que c’est tout simplement défini dans le contrat de la méthode hashCode() et utilisé dans les collections de type HashMap ou HashSet.
Quant au bout de code, il s’agit d’un exemple bien réel, j’ai juste renommé la classe.
Débloque les + belles offres tech en 10 mins
Cet article vous a plu ? Vous aimerez sûrement aussi :
Julien
Moi c’est Julien, ingénieur en informatique avec quelques années d’expérience. Je suis tombé dans la marmite étant petit, mon père avait acheté un Apple – avant même ma naissance (oui ça date !). Et maintenant je me passionne essentiellement pour tout ce qui est du monde Java et du système, les OS open source en particulier.
Au quotidien, je suis devops, bref je fais du dév, je discute avec les opérationnels, et je fais du conseil auprès des clients.
