Cela fait quelques semaines que j'interviens de façon ponctuelle sur la maintenance d'un extranet pour un client, et que je m'énerve régulièrement sur l'incapacité de la plupart des développeurs à comprendre qu'un code doit être un minimum propre... L'application, écrite en asp.net, est un ensemble de morceaux de code réalisés, probablement, à la va-vite par des CDD si j'en juge par la "qualité" très médiocre.
Voici donc quelque conseils à retenir si vous faites du développement ASP.net et que vous souhaitez voir votre code être maintenu...
N'accèdez jamais, directement dans votre page, à des variables de sessions ou d'application : c'est probablement le B-A-BA du développement avec des langages typés... Il n'est pas normal de voir dans une page ASP.net (sauf cas exceptionnels) des accès du type :
(int) Session["MEMBRE_ID"] // non !!!!!
Pourquoi cela n'est-il pas bien ? eh bien tout simplement parce que si une autre page accède (ou encore plus catastrophique met à jour) votre valeur, il y a 1 chance sur 2 pour que celle-ci ne manipule pas le même type de données (dans l'exemple de l'application extranet, l'une des pages traitait l'identifiant client comme un decimal et l'autre comme un int...). Pour réaliser un code maintenable, il vous reste à faire une class "Helper" qui se charge de masquer l'accès à la variable de session et le typage :
public class SessionHelper
{
public int MembreId
{
get
{
object o = Session["MEMBRE_ID"];
if (o == null || !(o is int))
return 0;
return (int)o;
}
set
{
Session["MEMBRE_ID"] = value;
}
}
}
Avec ça, plus jamais de "InvalidCastException" - enfin, si je ne me suis pas trompé dans l'écriture de la classe :) - et vous risquez moins de problème qu'à tout stocker en string (je pense principalement à des soucis d'injection sql...). Sans parler du fait que vous ne chercherez plus si vous avez appelé votre variable "MEMBRE_ID", "IdMembre", "MEMBREID", ou tout autre variation...
Dans le même ordre d'idée, ne réalisez jamais les updates sans typer vos variables : à moins de réaliser une application poubelle et donc d'utiliser le RAD à 100%, vous avez probablement écrit des méthodes (si ce n'est des objets) pour l'implémentation de vos règles métiers et de vos accès aux données. Bien ! mais pitié, n'utilisez pas des signatures du type :
public static Membre GetMembre(string membreId)
{
// non !!!!!
}
lorsque vous savez que membreId est un int, cela fait désordre. Lorsque je vois ce genre de signature, cela m'effraie toujours un peu : si le développeur n'a même pas été capable de typer ses variables, il y a peu de chance qu'il ai fait des requêtes paramétrées, et c'est donc une porte ouverte à l'injection SQL...
Si vous avez des cas complexes dans vos règles de gestion, découpez le problème en différentes méthodes axées chacune sur la résolution d'un problème simple. Ce conseil la, je pensais vraiment que tout le monde l'avait en tête, mais je me trompais lourdement... Toujours sur le même exemple (oui, cela pourrait être un cas d'école cet extranet, pour le cours "programmez comme des pieds, c'est mieux !"), l'application présente une menu général dont les fonctionnalités changent en fonction de critères multiples (droits accordé à un client, type de structure dont il fait partie, etc.), voici (extrêmement simplifiée) la façon dont cela est traité :
string matricePossibilite = "";
matricePossiblite += Session["MEMBRE_TYPE"].ToString();
matricePossibilite += Session["..."].ToString();
// il y a encore 3 autres criteres comme ceci...
switch (matricePossibilite)
{
case "11012":
// une bonne cinquentaine de lignes de code
break;
case "21012":
// d'autres lignes...
break;
}
Non, non, vous ne rêvez pas, c'est bel est bien écrit comme cela, avec un minimum de commentaires, pour la plupart inutiles d'ailleurs, histoire de simplifier l'affaire... Hormis le fait que tout le code se trouve dans la même méthode, c'est aussi typiquement le cas ou un switch est une très mauvaise idée : cela rends plus difficile à lire. Admettons que l'on garde le switch pour une premiere ré-écriture, il est déjà plus qu'obligatoire de virer toutes ces lignes de codes :
string matricePossibilite = "";
matricePossibilite += Session["MEMBRE_TYPE"].ToString();
matricePossibilite += Session["..."].ToString();
// il y a encore 3 autres critères comme ceci...
switch (matricePossibilite)
{
case "11012":
NomDuCasNumero1();
break;
case "21012":
NomDuCasNumero2();
break;
}
C'est déjà un poil plus lisible... Il reste ensuite, soit à bien documenter ce que signifie chacun des cas :
case "11012":
// l'utilisateur est de type ...
// dans une structure ....
// etc
Ou même encore mieux : ne pas utiliser de variable bizarre pour déterminer les droits mais des variables avec des noms compréhensibles :
bool estAdmin = SessionHelper.MembreType == MembreType.Admin;
TypeStruct type = SessionHelper.TypeStructure;
// les autres critères selon le même principe
if(estAdmin)
{
if(...) // un autre critère
NomDuCasNumero1();
else
NomDuCasNumero2();
}
else if(!estAdmin && type==TypeStruct.Type1)
{
NomDuCasNumero3();
}
// etc.
C'est un peu plus verbeux, et cela demandera certainement un peu de réflexion pour ne pas tomber dans une liste de if/else if ou une cascade de if imbriqués - pensez de nouveau à découper en plusieurs méthodes si vous avez trop de if... - mais qu'est-ce que c'est plus simple à comprendre !
Si vous êtes adepte des procédure stockées (ce n'est pas mon cas, mais bon, tous les goûts sont dans la nature...), n'hésitez pas à les nommer proprement : mbr_s c'est bien comme nom mais MembreSuppr c'est carrément plus lisible... et tant qu'à faire, pensez à définir des méthodes-metier dont les noms sont en relation avec celui de la proc-stock.
Voila, ce sont les quelques conseils/remarques qui me sont venus à l'esprit au cours des heures passées à me battre avec cette application. Pour résumer :
- la plupart des langages actuels sont fortement typés (cela n'a pas que des avantages, mais évite certains dérapages...), assurez vous donc que votre code en soit conscient et compense automatiquement les cas où le typage est plus léger (pour asp.net, il faut comprendre par là : tout ce qui à été conservé compatible avec asp...)
- votre code doit pouvoir être compris dans son ensemble par un autre développeur sans avoir à lire 2000 pages de document Word ou sans avoir besoin de lire dans vos pensées, un peu de commentaires et surtout un code lisible qui peut se comprendre facilement est très souvent préférable à un code plus optimisé - ou du moins qui vous paraît plus optimisé.
- ah...oui... pour avoir eu aussi un problème de code source livré différents de l'application en production : utilisez un système de gestion de source : il est rageant de devoir décompiler la version de production pour retrouver des bouts de code...
Et pour thierry, si il passe sur ce billet : essayez de trouver un prestataire qui ne soit pas en carton pour la prochaine fois :)