Discussion :

[Merillym]

Bonjour,

Je mettrai ici de temps en temps des bouts de code issus du programme que je suis en train de faire dans le but d'avoir des retours critiques sur la "propreté" de mon code.

Est-ce bien codé ? Tant sur la forme que sur le fond. Le but est de prendre les bons réflexes et d'éviter les mauvaises habitudes.

Je commence par cette fonction :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

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
int get_handlepath(char path[])
{
  int     i_path;
  int     i_handle;
  int     i_found;
  int     i_subpath;
 
  for (i_path = 0; i_path < compter_ch(path); i_path++)
    {
      if (path[i_path] == '\\')
        {
           i_found = i_path;
           i_path = 0;
           if ((t_registre.handle = (char*)malloc((i_found+1)*sizeof(char)))
               && (t_registre.path = (char*)malloc((compter_ch(path)-i_found)*sizeof(char))) != NULL)
             {
               for (i_handle = 0; i_handle < i_found; i_handle++)
                 {
                   t_registre.handle[i_handle] = path[i_path];
                   i_path++;
                 }
 
                t_registre.handle[i_found] = '\0';
                i_subpath = 0;
                for (i_found = i_found+1; i_found < compter_ch(path); i_found++)
                  {
                    t_registre.path[i_subpath] = path[i_found];
                    i_subpath++;
                  }
 
                t_registre.path[i_subpath] = '\0';
                return (0);
                break;
             }
        }
    }
    return (1);
}

Merci par avance.

[SofEvans]

La premiere des chose et la plus flagrante : absence de commentaire.
Il faudrait mettre un minimum de commentaire, sur ce que fais la fonction globalement, ce qu'elle renvoie, a quoi correspond ce renvoie ...

Bref, expliquer comment tu va proceder pour que quelqu'un puisse comprendre dans les grandes lignes ce que tu va faire. Evite surtout de tomber dans l'exces inverse, c'est a dire de trop commenté et/ou de paraphraser :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
/* Alloue dynamiquement un tableau de trois int */
Tableau = (int*) malloc (3*sizeof(int));

Ce genre de commentaire est néfaste, stupide et inutile.

[Merillym]

La premiere des chose et la plus flagrante : absence de commentaire.

Certaines "normes" déconseillent les commentaires au sein des fonctions : ils doivent être juste au-dessus et signalés d'une certaine façon.

J'aurais tendance à aller dans ce sens, puisque je trouve que les commentaires nuisent à l'aspect visuel du code.

Par contre, tout à fait d'accord pour faire une entête qui stipule ce que fait la fonctions, ses arguments, ce qu'elle renvoie etc.

[diogene]

Puisque tu demandes des critiques :

- i_path semble gérer par le for(ipath =0 ;... ; ipath++). Or on trouve dans la boucle des instructions ipath = 0 et ipath++ ce qui ne facilite pas la lecture du code.

- Tu utilises une variable globale, t_registre, ce qui est toujours à éviter

- Dans ce code, le break ne sert à rien :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 return (0);
 break;

- L'écriture du if est incohérente

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
  if ((t_registre.handle = ...)  // pourquoi ici pas de comparaison
       && (t_registre.path =...) != NULL) // et ici on en fait une

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
  if ((t_registre.handle = ...)!= NULL
       && (t_registre.path =...) != NULL)

Si je comprend ton code, il s'agit d'analyser une chaine pour chercher \. Ce qui le précède est placé dans une chaine pointée par t_registre.handle et ce qui le suit dans une autre pointée par t_registre.path.
Ton code me semble bien compliqué si c'est le cas. pourquoi n'utilises-tu pas les fonctions de la bibliothèque ? Je crois que ceci fait la même chose :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
int get_handlepath( struct x *t_registre, char path[])
{
  size_t len1, len2;
  char * p = strchr(path,'\\');
  if(p != NULL)
  {
    len1 = p-path ;
    p++;
    len2 = strlen(p);
    if (   (t_registre->handle = malloc(len1+1)) != NULL
        && (t_registre->path   = malloc(len2+1)) != NULL)
    {
        strncpy(t_registre->handle,path,len1);
        t_registre->handle[len1] = '\0';
        strcpy(t_registre->path,p);
    }
    else free(t_registre->handle);
  }
  return p == NULL;
}

[gl]

Est-ce bien codé ? Tant sur la forme que sur le fond. Le but est de prendre les bons réflexes et d'éviter les mauvaises habitudes.

Tout d'abord, ça manque de commentaire. Au minimum un commentaire décrivant le rôle de cette fonction, ses paramètres et ses valeurs de retour.
Là je ne sais même pas ce que fait cette fonction sans rentrer dedans en détail.

En dehors de ce point, voici quelques autres détails (je pars du principe que tout le code autour - appelant, inclusion des en-tête, etc. - est correct) :

• Le paramètre path n'étant pas modifié par la fonction il devrait être constant.
• Si le rôle de la ligne if (path[i_path] == '\\') est bien d'identifier les séparateur dans un path, je testerais également avec le caractère '/'.
• D'où vient t_registre ? Est-ce une variable globale ? Dans ce cas et sans plus de précision, c'est probablement une mauvaise idée. D'une manière générale, les variables globales sont à éviter.
• Les deux styles de test (l'un explicitant la valeur NULL et l'autre pas) pour le retour du malloc ne sont pas du meilleur effet. Sans rentrer dans la discussion de quelle pratique est la meilleure, il est largement préférable de se tenir à un seul style dans un programme.
• Au passage en cas d'erreur sur le second malloc, tu as une jolie fuite mémoire. Je suppose qu'en cas de réussite, c'est l'appelant qui finit par tout libérer, dans le cas contraire, tu as une autre fuite mémoire.
• Le cast du retour de malloc() n'est pas utile en C.
• Plutôt que d'utiliser sizeof(char) (qui par définition vaut 1), j'utiliserais sizeof(*t_registre.handle) et sizeof(*t_registre.path) dans les malloc.
• Pourquoi avoir sortie l'initialisation et l'incrément de certain compteur du for ?
• J'ignore ce que fait en détail la fonction compter_ch, mais si comme son nom le laisse penser il s'agit d'un clone de strlen(), il serait plus judicieux (puisque path n'est pas modifié) de l'appeler une seule fois et de stocker le résultat plutôt que de l'appeler à chaque tour de boucle.

Au passage, pourquoi ne pas utiliser les fonctions de la bibliothèque standard ? Sauf contrainte particulière c'est une très mauvaise idée.

[Merillym]

Bonsoir,

Merci à tous pour vos avis. Je vais les lire soigneusement et les prendre en compte.

Au passage, pourquoi ne pas utiliser les fonctions de la bibliothèque standard ? Sauf contrainte particulière c'est une très mauvaise idée.

Je m'étais "amusé" à recoder certaines fonctions pour m'entraîner et j'ai pris la fâcheuse habitude d'utiliser mes fonctions... je vais aussi rectifier ça.

Merci

[Merillym]

- Tu utilises une variable globale, t_registre, ce qui est toujours à éviter

Oki, je pense qu'elle pourrait se justifier ici, la voici :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
struct      s_registre
{
    char    *handle;
    char    *path;
    char    *pathKeyEnd;
    char    *option;
    char    *value;
    char    *type;
    char    *data;
    char    *fichier;
};

Mais en y réfléchissant, rien ne justifie vraiment d'en faire une variable globale.

- L'écriture du if est incohérente

Une erreur d'inattention, je suis tout à fait d'accord, merci d'avoir pointé l'erreur.

Si je comprend ton code, il s'agit d'analyser une chaine pour chercher \. Ce qui le précède est placé dans une chaine pointée par t_registre.handle et ce qui le suit dans une autre pointée par t_registre.path.
Ton code me semble bien compliqué si c'est le cas. pourquoi n'utilises-tu pas les fonctions de la bibliothèque ? Je crois que ceci fait la même chose :

Tu as tout compris, je vais arrêter d'utiliser mes fonctions et apprendre à utiliser celles issues de la bibliothèque standard.

Si le rôle de la ligne if (path[i_path] == '\\') est bien d'identifier les séparateur dans un path, je testerais également avec le caractère '/'.

Ici non, puisqu'il s'agit de parser une clé de registre windows.

J'ignore ce que fait en détail la fonction compter_ch, mais si comme son nom le laisse penser il s'agit d'un clone de strlen(), il serait plus judicieux (puisque path n'est pas modifié) de l'appeler une seule fois et de stocker le résultat plutôt que de l'appeler à chaque tour de boucle.

En effet.

Merci pour tous ces commentaires détaillés.

[Merillym]

J'ai pris le temps de bien tout reprendre et effectivement ton code fait la même chose Diogene

Je posterai un autre bout de code d'ici quelques jours ( une autre fonction ) et vous pourrez me dire si c'est mieux.

Merci.

[diogene]

Pour une gestion correcte des erreurs d'allocation de la mémoire, dans le code que j'ai posté, il faut modifier un peu :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
    if (   (t_registre->handle = malloc(len1+1)) != NULL
        && (t_registre->path   = malloc(len2+1)) != NULL)
    {
        strncpy(t_registre->handle,path,len1);
        t_registre->handle[len1] = '\0';
        strcpy(t_registre->path,p);
    }
    else free(t_registre->handle);

PS : je modifie en conséquence le code du post précédent.