Jérémy Jeanson's blog
chasseur de bugs depuis .net 1.0

Quand on parle de qualité de code, chacun a ses habitudes pour identifier les problèmes. Certains lanceront systématiquement Code Analysis, NDepend, SonarQube, ou autres outils. D’autres préfèreront jeter un œil au code avant de faire quoi que ce soit.

Mais, il restera toujours quelques situations où l’œil humain sera le seul recours efficace. C’est le cas des mauvais usages des méthodes Trim(), ToLower(), et ToUpper() de String.

Ces trois petites méthodes insignifiantes sont de véritables gouffres à ressources et provoquent quantité de mauvaises pratiques.

Dit comme cela, je prends bien conscience que l’on va me prendre un fou. Il y a toujours un moment où l’on peut avoir besoin de celle-ci pour formater un contenu. Oui, c’est là leur but premier. Il y a cependant des dérives qui sont source de problèmes. Tant pour les performances, la compréhension du code, que pour la fiabilité de celui-ci. Il s’agit des conditions / tests.

Le danger

À partir du moment où ces trois méthodes sont employées dans un if ou un switch, il y danger.

Exemple :

if(arguments.FirstOrDefault().ToString().Trim().ToUpper() == constante1)

// D’accord, cette ligne conjugue plusieurs mauvaises pratiques, mais qui n’a jamais vu une telle combinaison ?

Mais que se passera-t-il si notre constante1 est modifiée par un développeur qui n’a pas conscience qu’il faut à tout prix qu’elle ne contienne que des majuscules ?

Vous commencez à saisir les risques encourus par un code qui contient trop de ToLower() et ToUpper() ?

La solution :

String.Equals(), StartsWith(), Contains(), et autres méthodes de tests peuvent être utilisées avec un argument de type StringComparison pour ne pas être sensible à la casse (CurrentCultureIgnoreCase, OrdinalIgnoreCase, et mon préféré InvariantCultureIgnoreCase). Ce qui résout le problème.

La lisibilité du code

Quand Trim() est combiné à ToLower(), ou ToUpper(), et que cette approche est utilisée de chaque côté d’une égalité, le code devient rapidement illisible.

Ajouté à cela un StartsWith(), un Contains() ou un IndexOf, et le code devient imbuvable. Ce qui est bien dommage, car comme dit un peu plus tôt, ces méthodes permettent l’usage d’un argument de type StringComparison.

Les problèmes de performances

Trim(), ToLower() et ToUpper() ont un petit effet pervers. Ces méthodes instancient systématiquement une nouvelle String. Nombre de développeurs me rétorqueront que cela n’est pas grave et n’a pas d’impact visible. Je ne peux pas être en accord avec cette affirmation. Chaque application a ses impératifs. Si un développeur prend de bonnes habitudes dès le début, il sera en mesure de travailler dans un contexte où il faut impérativement réduire les instanciations inutiles.

Petit rappel : l’instanciation d’une String ou tout autre type référence va consommer de la mémoire (RAM ou disque si l’OS swap). Pour accéder à cette mémoire, le CPU va devoir être utilisé. Une instanciation consomme donc du CPU, de la RAM et en cas de malchance des accès au disque. En prime, tout type référence fait travailler le Garbage Collector (GC). En fonction de la configuration du GC, celui-ci peut être amené à travailler davantage. Ce qui va consommer un peu plus de CPU et potentiellement ralentir l’application. Mais d’un autre côté, si le GC ne travaille pas suffisamment, on peut se retrouver face à un problème de fragmentation de la mémoire.

Si on le peut, il convient donc de ne pas instancier de ressources inutiles. Cela est d’autant plus important sur des environnements de type :

• Mobile
• IOT
• Ressources cloud partagées / Containers (docker, kubernetes, …)
• Serveurs devant répondre à de grosses quantités de requêtes chaque seconde.
• Solutions à base de Citrix / RDS exécutant de très nombreuses versions de la même application sur un même serveur.

Note : Je suis toujours surpris de voir des développeurs minimiser l’impact des instanciations de String en .net alors qu’ils évitent toute instanciation quand ils sont amenés à coder en C++.

Le cas particulier du Trim()

Trim() est encore trop souvent utilisé pour comparer une chaîne avec "" ou String.Empty. Il est pourtant très simple d’utiliser la méthode String.IsNullOrWhiteSpace(). Outre le fait de ne pas instancier une nouvelle String, celle-ci permet d’éviter le test avec null (ou le ? devant le .Trim())

Trim() est parfois utilisé dans des couches très basses d’une application afin de "sécuriser" des arguments. Normalement, les entrées des utilisateurs et les ressources en base devraient avoir droit à un Trim() très tôt (dans les couches très hautes de l’application). Un Trim() ne devrait donc pas se trouver ici. Ceci démontre un gros problème de fiabilité et de confiance.

Conclusion

Attention aux méthodes Trim(), ToLower() et ToUpper(). Un trop important usage de celles-ci peut nuire à la lisibilité du code, à sa maintenabilité et aussi aux performances de l’application finale.