IdentifiantMot de passe
Loading...
Mot de passe oublié ?Je m'inscris ! (gratuit)
Navigation

Inscrivez-vous gratuitement
pour pouvoir participer, suivre les réponses en temps réel, voter pour les messages, poser vos propres questions et recevoir la newsletter

 C Discussion :

Bien coder en C : avis sur bout(s) de code


Sujet :

C

  1. #1
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut Bien coder en C : avis sur bout(s) de code
    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.


  2. #2
    Membre émérite Avatar de SofEvans
    Homme Profil pro
    Développeur C
    Inscrit en
    Mars 2009
    Messages
    1 076
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 35
    Localisation : France

    Informations professionnelles :
    Activité : Développeur C

    Informations forums :
    Inscription : Mars 2009
    Messages : 1 076
    Points : 2 328
    Points
    2 328
    Par défaut
    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.

  3. #3
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    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.

  4. #4
    Expert éminent sénior
    Avatar de diogene
    Homme Profil pro
    Enseignant Chercheur
    Inscrit en
    Juin 2005
    Messages
    5 761
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Essonne (Île de France)

    Informations professionnelles :
    Activité : Enseignant Chercheur
    Secteur : Enseignement

    Informations forums :
    Inscription : Juin 2005
    Messages : 5 761
    Points : 13 926
    Points
    13 926
    Par défaut
    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 :
    - 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;
    }
    Publication : Concepts en C

    Mon avatar : Glenn Gould

    --------------------------------------------------------------------------
    Une réponse vous a été utile ? Remerciez son auteur en cliquant le pouce vert !

  5. #5
    gl
    gl est déconnecté
    Rédacteur

    Homme Profil pro
    Inscrit en
    Juin 2002
    Messages
    2 165
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 45
    Localisation : France, Isère (Rhône Alpes)

    Informations forums :
    Inscription : Juin 2002
    Messages : 2 165
    Points : 4 637
    Points
    4 637
    Par défaut
    Citation Envoyé par Merillym Voir le message
    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.

  6. #6
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    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

  7. #7
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    - 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.


  8. #8
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    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.

  9. #9
    Expert éminent sénior
    Avatar de diogene
    Homme Profil pro
    Enseignant Chercheur
    Inscrit en
    Juin 2005
    Messages
    5 761
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Essonne (Île de France)

    Informations professionnelles :
    Activité : Enseignant Chercheur
    Secteur : Enseignement

    Informations forums :
    Inscription : Juin 2005
    Messages : 5 761
    Points : 13 926
    Points
    13 926
    Par défaut
    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.
    Publication : Concepts en C

    Mon avatar : Glenn Gould

    --------------------------------------------------------------------------
    Une réponse vous a été utile ? Remerciez son auteur en cliquant le pouce vert !

  10. #10
    Rédacteur
    Avatar de Vincent Rogier
    Profil pro
    Inscrit en
    Juillet 2007
    Messages
    2 373
    Détails du profil
    Informations personnelles :
    Âge : 46
    Localisation : France

    Informations forums :
    Inscription : Juillet 2007
    Messages : 2 373
    Points : 5 307
    Points
    5 307
    Par défaut
    Personnellement, l'appel à malloc() dans un if me gène un peu...
    Je préfère dissocier les affectations des tests conditionnels.
    Je pense qu'il est préférable de faire le malloc() en premier et ensuite tester la valeur du pointeur. Le code fera 2 lignes de plus mais plus clair a mon sens.
    De plus, un compilo bien réglé doit te générer un warning avec ce code (affectation d'un valeur lors d'un d'un test conditionnel).
    Vincent Rogier.

    Rubrique ORACLE : Accueil - Forum - Tutoriels - FAQ - Livres - Blog

    Vous voulez contribuer à la rubrique Oracle ? Contactez la rubrique !

    OCILIB (C Driver for Oracle)

    Librairie C Open Source multi-plateformes pour accéder et manipuler des bases de données Oracle

  11. #11
    Expert éminent sénior
    Avatar de diogene
    Homme Profil pro
    Enseignant Chercheur
    Inscrit en
    Juin 2005
    Messages
    5 761
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Essonne (Île de France)

    Informations professionnelles :
    Activité : Enseignant Chercheur
    Secteur : Enseignement

    Informations forums :
    Inscription : Juin 2005
    Messages : 5 761
    Points : 13 926
    Points
    13 926
    Par défaut
    Vincent Rogier :
    Je préfère dissocier les affectations des tests conditionnels.
    Je pense qu'il est préférable de faire le malloc() en premier et ensuite tester la valeur du pointeur. Le code fera 2 lignes de plus mais plus clair a mon sens.
    Oui, je suis assez d'accord sur la question de la lisibilité (surtout si la ligne est longue et pleine de parenthèses).
    Mais dans le cas présent, je ne suis pas certain que le code soit plus clair ici:
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
     
       t_registre->handle = malloc(len1+1)
       if(t_registre->handle != NULL)
       {
         t_registre->path   = malloc(len2+1);
         if(t_registre->path != NULL)
         {
          ...
         }
         else free(t_registre->handle);
       }
    que là :
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
     
        if (   (t_registre->handle = malloc(len1+1)) != NULL
            && (t_registre->path   = malloc(len2+1)) != NULL)
        {
          ...
        }
        else free(t_registre->handle);
    De plus, un compilo bien réglé doit te générer un warning avec ce code (affectation d'un valeur lors d'un d'un test conditionnel).
    Il ne devrait pas. Dans ce cas, il n'y a pas de confusion possible du programmeur entre = et == dans ce contexte.
    Par contre, ce doit être le cas pour
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
     
        if (   (t_registre->handle = malloc(len1+1)) 
            && (t_registre->path   = malloc(len2+1)))
    Publication : Concepts en C

    Mon avatar : Glenn Gould

    --------------------------------------------------------------------------
    Une réponse vous a été utile ? Remerciez son auteur en cliquant le pouce vert !

  12. #12
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    Merci pour ces dernières remarques. Pour ma part, je serais plutôt de l'avis de Diogene concernant le malloc, mais débutant en C, je m'abstiendrai de tout jugement. Je sollicite vos avis, c'est bien pour changer mes mauvaises habitudes et prendre en compte vos remarques, que j'apprécie beaucoup, merci.

    Toujours sur ce bout de code, je voudrai être sûr de partir sur une bonne base et j'ai quelques lacunes (encore) sur pointeurs, structures...

    J'ai repris ton exemple Diogene et je vais désormais recoder tout mon tool en utilisant les fonctions standard et en intégrant toutes vos remarques. J'ai 10.000 lignes de codes à reprendre, c'est pour ça que j'insiste beaucoup sur le début, afin de partir sur les meilleurs bases possibles. Et pour information, c'est un projet personnel que je fais en loisir, rien à voir avec mes études

    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
    39
    40
    41
    42
    43
    44
    45
    46
    #include <windows.h>
    #include <stdio.h>
    #include <string.h>
    #include "headers.h"
     
    int get_handlepath(struct s_registre *ptr_registre, const char path[])
    {
      size_t len1;
      size_t len2;
      char *pos;
     
      pos = strchr(path, '\\');
      if (pos != NULL)
        {
          len1 = pos-path;
          pos++;
          len2 = strlen(pos);
          if (   (ptr_registre->handle = malloc(len1+1)) != NULL
              && (ptr_registre->path   = malloc(len2+1)) != NULL)
          {
              strncpy(ptr_registre->handle, path, len1);
              ptr_registre->handle[len1] = '\0';
              strcpy(ptr_registre->path, pos);
          }
        }
        return (*pos);
    }
     
    int main (int argc, char *argv[])
    {
        struct      s_registre t_registre;
        struct      s_registre *ptr_registre;
        const char  *path;
        size_t n;
     
        ptr_registre = &t_registre;
        n = strlen(argv[2])*sizeof(char);
        if ((path = malloc(n+1)) == NULL)
          return (1);
     
        memcpy(path, argv[2], n);
        get_handlepath(ptr_registre, path);
        printf("path = %s\n", ptr_registre->path);
        printf("handle = %s\n", ptr_registre->handle);
        return (0);
    }
    La structure :

    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;
    };
    Le compilo me renvoie un warning sur memcpy et effectivement le résultat obtenu n'est pas correct.

    J'aimerais bien une petite explication à nouveau sur structure, pointeur de structure et pointeur de structure passé comme argument à une fonction, car je m'embrouille encore. Je sens que ça commence à venir, mais ce n'est pas encore ça.

    Dans le bout de code ci-dessus, je suis sûr que ma déclaration de structure, son initialisation, la déclaration et initialisation du pointeur et enfin l'appel de la fonction ne sont pas bons.

  13. #13
    Expert éminent sénior
    Avatar de diogene
    Homme Profil pro
    Enseignant Chercheur
    Inscrit en
    Juin 2005
    Messages
    5 761
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Essonne (Île de France)

    Informations professionnelles :
    Activité : Enseignant Chercheur
    Secteur : Enseignement

    Informations forums :
    Inscription : Juin 2005
    Messages : 5 761
    Points : 13 926
    Points
    13 926
    Par défaut
    -
    Le compilo me renvoie un warning sur memcpy et effectivement le résultat obtenu n'est pas correct.
    C'est de ta faute : tu as précisé
    soit "path est un pointeur sur des caractères constants". Donc tu ne peux pas écrire dans la zone pointée par path, ce qui indispose le memcpy.
    -
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    n = strlen(argv[2])*sizeof(char);
    sizeof(char) vaut toujours 1
    -
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    get_handlepath(ptr_registre, path);
    ptr_registre est inutile dans ton code, passe directement l'adresse de t_registre. La fin du code devient :
    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
     
    int main (int argc, char *argv[])
    {
        struct      s_registre t_registre;
        char  *path;
        size_t n;
     
        n = strlen(argv[2]);
        if ((path = malloc(n+1)) == NULL)  return (1);
        memcpy(path, argv[2], n);
        get_handlepath(&t_registre, path);
        printf("path = %s\n", t_registre.path);
        printf("handle = %s\n", t_registre.handle);
        return (0);
    }
    Publication : Concepts en C

    Mon avatar : Glenn Gould

    --------------------------------------------------------------------------
    Une réponse vous a été utile ? Remerciez son auteur en cliquant le pouce vert !

  14. #14
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    J'ai trouvé aussi une erreur sur le retour de la fonction et sa déclaration : je retourne un pointeur de type char 'pos' et non '*pos', donc j'ai modifié le prototype de la fonction en conséquence.

    soit "path est un pointeur sur des caractères constants". Donc tu ne peux pas écrire dans la zone pointée par path, ce qui indispose le memcpy.
    Compris.

    sizeof(char) vaut toujours 1
    SoftEvans m'avait conseillé de toujours laissé sizeof(char) plutôt que 1. Je ne sais pas trop quoi faire à ce sujet, quelle "norme" appliquer etc.

    ptr_registre est inutile dans ton code, passe directement l'adresse de t_registre. La fin du code devient :
    Inutile pour le moment, mais je vais avoir besoin de passer un pointeur vers la structure s_registre de type t_registre assez souvent, donc je pense qu'il est préférable de passer un pointeur plutôt qu'une structure toute entière non ?

    Car si j'enlève ptr_pointeur, je suis désormais obligé de passer la structure en paramètre et non un pointeur vers cette structure, si j'ai bien compris ?

    edit:

    Là ça marche nickel, est-ce mieux niveau syntaxe du code ?

    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
    int main (int argc, char *argv[])
    {
        struct      s_registre t_registre;
        struct      s_registre *ptr_registre;
        char        *action;                                        // == argv[1]
        char        *path;                                          // == argv[2]
        char        *option1;                                       // == argv[3]
        char        *option2;                                       // == argv[4]
        size_t      n;
        char        *pos;
     
        ptr_registre = &t_registre;
        n = strlen(argv[2])+1;
        if ((path = malloc(n * sizeof(*path))) == NULL)
          return (1);
     
        memcpy(path, argv[2], n);
        if ((pos = get_handlepath(ptr_registre, path)) == NULL)
          return (1);
     
        printf("path = %s\n", ptr_registre->path);
        printf("handle = %s\n", ptr_registre->handle);
        return (0);
    }

  15. #15
    Expert éminent sénior
    Avatar de diogene
    Homme Profil pro
    Enseignant Chercheur
    Inscrit en
    Juin 2005
    Messages
    5 761
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Essonne (Île de France)

    Informations professionnelles :
    Activité : Enseignant Chercheur
    Secteur : Enseignement

    Informations forums :
    Inscription : Juin 2005
    Messages : 5 761
    Points : 13 926
    Points
    13 926
    Par défaut
    SoftEvans m'avait conseillé de toujours laissé sizeof(char) plutôt que 1. Je ne sais pas trop quoi faire à ce sujet, quelle "norme" appliquer etc.
    C'est sans doute affaire de goût. Personnellement, je ne le met pas : je trouve que ça allonge les lignes et distrait inutilement lors de la lecture du code.

    Inutile pour le moment, mais je vais avoir besoin de passer un pointeur vers la structure s_registre de type t_registre assez souvent, donc je pense qu'il est préférable de passer un pointeur plutôt qu'une structure toute entière non ?
    Il faut voir pour la suite. En général, je trouve qu'on n'a pas besoin d'ajouter des trucs tant qu'ils ne s'avèrent pas effectivement utiles : ça alourdi le code pendant le développement et polarise le programmeur sur des options choisies parfois prématurément.

    Car si j'enlève ptr_pointeur, je suis désormais obligé de passer la structure en paramètre et non un pointeur vers cette structure, si j'ai bien compris ?
    Non, tu peux passer l'adresse de la structure plutôt que la valeur d'un pointeur qui contient l'adresse de la structure. Comme ici dans le code que j'ai posté
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
        get_handlepath(&t_registre, path);
    Publication : Concepts en C

    Mon avatar : Glenn Gould

    --------------------------------------------------------------------------
    Une réponse vous a été utile ? Remerciez son auteur en cliquant le pouce vert !

  16. #16
    Membre émérite Avatar de SofEvans
    Homme Profil pro
    Développeur C
    Inscrit en
    Mars 2009
    Messages
    1 076
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Âge : 35
    Localisation : France

    Informations professionnelles :
    Activité : Développeur C

    Informations forums :
    Inscription : Mars 2009
    Messages : 1 076
    Points : 2 328
    Points
    2 328
    Par défaut
    Citation Envoyé par diogene Voir le message
    C'est sans doute affaire de goût. Personnellement, je ne le met pas : je trouve que ça allonge les lignes et distrait inutilement lors de la lecture du code.
    [/code]
    En fait, mon prof de C m'avait dit que char pouvait avoir differente valeurs (selon le processeur je crois) et donc avait qu'il fallait toujours laisser le sizeof(char) ...

    Recemment, j'ai apris que la norme etait sizeof(char) == 1, ce que je ne savais pas. Apres, c'est effectivement une affaire de gout : ceux qui ont vu tellement de fois sizeof(char) en ont marre et raccourcice l'affaire.

    La seule norme en langage est donné par la norme. Apres, il y a plus ou moins des style de codage en vogue, mais tout le reste est une affaire de gout.

  17. #17
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    Bonsoir,

    Je vous mets une autre fonction, dont le but est similaire, c'est-à-dire qu'elle doit parser une chaîne.

    J'ai essayé de prendre en compte au maximum toutes vos remarques et j'aimerais vos avis, savoir s'il y a du progrès ou pas, quels sont les défauts, etc. Mon objectif est de finir par "coder proprement" le plus naturellement possible. Donc soyez fermes

    De même je suis intéressé pour avoir vos avis concernant l'optimisation en terme de temps d'exécution du programme : ce qu'il faut faire, ce qu'il faut éviter de faire.

    Je pense mettre des bouts de code, ou quelques fonctions assez régulièrement, toujours dans la même optique. Si jamais cela vous dérange, n'hésitez pas à me le dire

    Voilà une autre fonction ( la structure est la même que dans l'autre exemple ) :

    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
    char *get_pathKeyEnd(struct s_registre *ptr_registre)
    {
      char      *pos;
      size_t    n;
     
      pos = strstr(ptr_registre->path, "|*|");
      if (pos != NULL)
        {
          n = strlen(ptr_registre->path)+1-(pos-(ptr_registre->path))-4;
          if ((ptr_registre->pathKeyEnd = malloc(n * sizeof(ptr_registre->pathKeyEnd))) != NULL)
            {
              strcpy(ptr_registre->pathKeyEnd, pos+4);
              ptr_registre->path[pos-(ptr_registre->path)-1] = '\0';
            }
        }
        return (pos);
    }
    Concernant le fait d'utiliser un pointeur de structure ou la structure elle-même, je ne sais pas trop... en terme de vitesse d'exécution, vous préférez quoi ?

    Merci

  18. #18
    Expert éminent sénior
    Avatar de diogene
    Homme Profil pro
    Enseignant Chercheur
    Inscrit en
    Juin 2005
    Messages
    5 761
    Détails du profil
    Informations personnelles :
    Sexe : Homme
    Localisation : France, Essonne (Île de France)

    Informations professionnelles :
    Activité : Enseignant Chercheur
    Secteur : Enseignement

    Informations forums :
    Inscription : Juin 2005
    Messages : 5 761
    Points : 13 926
    Points
    13 926
    Par défaut
    C'est pas mal.
    - Mais, supprime les parenthèses inutiles qui aloudissent la lecture :
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    n = strlen(ptr_registre->path)+1-(pos-ptr_registre->path)-4;
    ...
    ptr_registre->path[(pos-ptr_registre->path)-1] = '\0';
    Par contre,
    D'accord pour laisser +1 et -4 car ils commentent bien le code. -3 aurait été peu clair et on se serait demandé d'où il vient
    D'accord pour les parenthèses autour de (pos-ptr_registre->path), qui montrent bien le calcul d'un indice

    -
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
          if ((ptr_registre->pathKeyEnd = malloc(n * sizeof(ptr_registre->pathKeyEnd))) != NULL)
    Le sizeof est faux (l'opérande est du type char*). D'ailleurs, comme une ligne plus loin on fait un strcpy, on se doute que tu veux allouer n char.
    On peut se contenter de :
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
          if ((ptr_registre->pathKeyEnd = malloc(n )) != NULL)
    -
    Code : Sélectionner tout - Visualiser dans une fenêtre à part
              ptr_registre->path[pos-(ptr_registre->path)-1] = '\0';
    Code assez obscur qui ne montre pas à l'évidence où ce 0 va être placé et qui doit être équivalent à :
    - Sur le fonctionnement (là, c'est toi qui connait les réponses) :
    Est-il correct de sauter dans la copie le caractère suivant "|*|" ?
    Est-il correct de placer le '\0' à la place du caractère qui précède "|*|" et qui donc l'écrase ?
    Est-il certain qu'il existe au moins un caractère avant la sous-chaîne "|*|" ?
    Publication : Concepts en C

    Mon avatar : Glenn Gould

    --------------------------------------------------------------------------
    Une réponse vous a été utile ? Remerciez son auteur en cliquant le pouce vert !

  19. #19
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    Code assez obscur qui ne montre pas à l'évidence où ce 0 va être placé et qui doit être équivalent à :
    *(pos-1) = '\0';
    Je ne suis pas encore bien familier avec les fonctions de la bibliothèque standard puisque jusqu'à présent je n'utilisais que les miennes

    Je commence à mieux comprendre.

    Est-il correct de sauter dans la copie le caractère suivant "|*|" ?
    Oui.

    Est-il correct de placer le '\0' à la place du caractère qui précède "|*|" et qui donc l'écrase ?
    Oui.

    Est-il certain qu'il existe au moins un caractère avant la sous-chaîne "|*|" ?
    Oui, sauf si un utilisateur s'amuse à faire n'importe quoi, mais comme le tool est destiné à des gens qui sont censés savoir utiliser un command line tool... Mais sur le fond, tu as raison : il vaut mieux par principe tout contrôler.

    Merci beaucoup de ta réponse.

    Ca m'aide vraiment beaucoup d'avoir des personnes compétentes pour me corriger au niveau lisibilité et syntaxe : c'est ce qui fait le plus défaut dans un apprentissage en autodidacte.



    Voici une autre fonction, qui compare en ignorant la casse : je ne suis pas très satisfait de mes incrémentations de i et de j, surtout d'être obligé de partir à i = -1... si vous avez mieux, je suis preneur.

    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
    int compare_ch(char str1[], char str2[])
    {
        int     i;
        int     j;
     
        i = -1;
        j = 0;
        if ((strlen(str1) == strlen(str2)) && str1 != NULL && str2 != NULL)
          {
            while (i++ < strlen(str1));
            {
                if (str1[i] != str2[j] && str1[i] != str2[j]+32 && str1[i] != str2[j]-32)
                  return(1);
                j++;
            }
            return(0);
          }
          return(1);
    }

  20. #20
    Membre du Club
    Profil pro
    Inscrit en
    Octobre 2009
    Messages
    140
    Détails du profil
    Informations personnelles :
    Localisation : France

    Informations forums :
    Inscription : Octobre 2009
    Messages : 140
    Points : 49
    Points
    49
    Par défaut
    Bonjour,

    Voilà une autre fonction :

    Code : Sélectionner tout - Visualiser dans une fenêtre à part
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    unsigned int check_handle_HKLM(struct s_registre *ptr_registre)
    {
      HKEY hKey;
      if ((compare_ch(ptr_registre->handle, "HKLM")) == 0 ||
          (compare_ch(ptr_registre->handle, "HKEY_LOCAL_MACHINE")) == 0)
        {
          if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, ptr_registre->path, 0, KEY_READ, &hKey) == ERROR_SUCCESS)
            return (hKey);
          else
            return (-1);
        }
      return (-1);
    }
    J'aimerais retourner un handle de clé, mais je ne suis pas sûr que ma fonction marche... le type d'un handle de clé est HKEY, donc je me demande si je mets unsigned int ou HKEY en type de retour. J'attends d'avoir une réponse à cette question avant de tester la fonction ci-dessus

    Je vous mets ma fonction compare_ch ( qui ignore la casse ) :

    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 compare_ch(char str1[], char str2[])
    {
        int     i;
        int     j;
     
        i = 0;
        j = 0;
        if ((strlen(str1) == strlen(str2)) && str1 != NULL && str2 != NULL)
          {
            while (i < strlen(str1))
            {
              if (str1[i] != str2[j] && str1[i] != str2[j]+32 && str1[i] != str2[j]-32)
                return(1);
              j++;
              i++;
            }
            return(0);
          }
          return(1);
    }
    Comme pour les bouts de code précédents, j'attends vos remarques/critiques/conseils.

    Merci.


Discussions similaires

  1. Votre avis sur des parties de code "triviales"
    Par bstevy dans le forum SQL
    Réponses: 2
    Dernier message: 20/05/2015, 03h35
  2. Demande d'avis sur bout de code
    Par youx21 dans le forum Langage
    Réponses: 9
    Dernier message: 23/09/2013, 15h14
  3. Votre avis sur une portion de code
    Par ninikkhuet dans le forum Langage
    Réponses: 7
    Dernier message: 29/10/2009, 13h33
  4. [POO] Avis sur la Lisibilité du code
    Par redsaint0 dans le forum Langage
    Réponses: 7
    Dernier message: 12/03/2007, 21h09
  5. [java.lang.class] Votre avis sur une portion de code
    Par be_tnt dans le forum Langage
    Réponses: 3
    Dernier message: 18/10/2006, 16h55

Partager

Partager
  • Envoyer la discussion sur Viadeo
  • Envoyer la discussion sur Twitter
  • Envoyer la discussion sur Google
  • Envoyer la discussion sur Facebook
  • Envoyer la discussion sur Digg
  • Envoyer la discussion sur Delicious
  • Envoyer la discussion sur MySpace
  • Envoyer la discussion sur Yahoo