Demande de critiques constructives - Code C

**Reverse_** · 09/01/2015, 18h37

Bonsoir tout le monde,

J'aimerai avoir des avis/critiques constructifs sur mon début de code en C.
Toute remarque est bonne à prendre.

La partie de code que je vais vous présenter s'occupe d'aller chercher des informations concernant une raquette. ( dans un pong 2D ). Tel que sa couleur, sa position au départ, ...

/!\ ATTENTION /!\

Ce bout de code est fait pour un jeu de pong 2d pour avec la SDL2. Mais au moment ou j'ai codé cette partie, je ne l'avais pas installé, et d'ailleurs je ne l'ai toujours pas... Certaines partie du code vont changer ou partir. Notamment les variables red, green, blue, seront remplacé bien entendu par une variable du type SDL_Color, ect...

Les fonctions sont de types void, mais là aussi quand j'implémenterai la SDL2, je modifierai pour qu'elles puissent renvoyer un pointeur sur la structure qui va bien. Ex : static void get_color(...); deviendra static SDL_Color *get_color(...);

Pareil pour la fonction qui s'occupe de récupérer les touches pour contrôler la raquette, n'ayant pas encore les SDLK_z ect... Les conditions restent vide.

Je vous pries donc de m'excuser pour ce petit désagrément. Et j'espère avoir un retour, positif, ou négatif sur mon code !

/!\ ATTENTION /!\

main.c :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
 
#include <stdio.h>
#include <string.h>
 
#include "options_racquet.h"
 
int main(void)
{
    get_racquet_s_options(PATH_OPTIONS_RACQUET_ONE);
 
    return 0;
}

options_racquet.h :

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
 
#ifndef OPTIONS_RACQUET_H_INCLUDED
#define OPTIONS_RACQUET_H_INCLUDED
 
#define PATH_OPTIONS_RACQUET_ONE "racquet_one.opt"
 
#define DEC 10
 
#define RED "red="
#define GREEN "green="
#define BLUE "blue="
#define OPACITY "opacity="
 
#define X "x="
#define Y "y="
 
#define CTRL_ONE "ctrl_one="
#define CTRL_TWO "ctrl_two="
 
void get_racquet_s_options(const char options_file[]);
 
#endif // OPTIONS_RACQUET_H_INCLUDED

options_racquet.c :

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
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
 
#include "options_racquet.h"
 
#include <stdio.h>
#include <string.h>
 
static void get_color(FILE *options);
static void get_pos(FILE *options);
static void get_command(FILE *options);
 
void get_racquet_s_options(const char options_file[])
{
    char buffer[50];
 
    FILE *options = NULL;
    options = fopen(options_file, "r");
 
    if(options == NULL)
    {
        fprintf(stderr, "Erreur d'ouverteur du fichier : %s", options_file);
    }
 
    else
    {
        do
        {
            fgets(buffer, sizeof(buffer), options);
 
            if(strstr(buffer, "#COLOR") != NULL)
            {
                get_color(options);
            }
 
            if(strstr(buffer, "#POSITION") != NULL)
            {
                get_pos(options);
            }
 
            if(strstr(buffer, "#COMMAND") != NULL)
            {
                get_command(options);
            }
 
        }while(strstr(buffer, "#END") == NULL);
    }
 
    fclose(options);
}
 
static void get_color(FILE *options)
{
    char buffer[50];
    char *pBuffer = NULL;
 
    long int red = 0;
    long int green = 0;
    long int blue = 0;
    long int opacity = 0;
 
    do
    {
        fgets(buffer, sizeof(buffer), options);
 
        if((pBuffer = strstr(buffer, RED)) != NULL)
        {
            red = strtol(pBuffer + strlen(RED), NULL, DEC);
        }
 
        if((pBuffer = strstr(buffer, GREEN)) != NULL)
        {
            green = strtol(pBuffer + strlen(GREEN), NULL, DEC);
        }
 
        if((pBuffer = strstr(buffer, BLUE)) != NULL)
        {
            blue = strtol(pBuffer + strlen(BLUE), NULL, DEC);
        }
 
        if((pBuffer = strstr(buffer, OPACITY)) != NULL)
        {
            opacity = strtol(pBuffer + strlen(OPACITY), NULL, DEC);
        }
 
    }while(strstr(buffer, "#END_COLOR") == NULL);
}
 
static void get_pos(FILE *options)
{
    char buffer[50];
    char *pBuffer = NULL;
 
    long int x = 0;
    long int y = 0;
 
    do
    {
        fgets(buffer, sizeof(buffer), options);
 
        if((pBuffer = strstr(buffer, X)) != NULL)
        {
            x = strtol(pBuffer + strlen(X), NULL, DEC);
        }
 
        if((pBuffer = strstr(buffer, Y)) != NULL)
        {
            y = strtol(pBuffer + strlen(Y), NULL, DEC);
        }
 
    }while(strstr(buffer, "#END_POSITION") == NULL);
 
}
 
static void get_command(FILE *options)
{
    char buffer[50];
    char *pBuffer = NULL;
 
    do
    {
        fgets(buffer, sizeof(buffer), options);
 
        if((pBuffer = strstr(buffer, CTRL_ONE)) != NULL)
        {
 
        }
 
        if((pBuffer = strstr(buffer, CTRL_TWO)) != NULL)
        {
 
        }
 
    }while(strstr(buffer, "#END_COMMAND") == NULL);
}

Voici à quoi ressemble le fichier racquet_one.opt :

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
 
#RACQUET_ONE
 
    #COLOR
        red=100
        green=255
        blue=15
        opacity=255
    #END_COLOR
 
    #POSITION
        x=5
        y=225
    #END_POSITION
 
    #COMMAND
        ctrl_one=SDLK_z
        ctrl_two=SDLK_s
    #END_COMMAND
 
#END

Je vous remercie d'avoir prit le temps de lire ce post, et de peut-être y avoir répondu en me donnant votre avis.
Je vous souhaite une bonne journée/soirée.

Merci,
Cordialement.

**Sve@r** · 09/01/2015, 22h15

Bonsoir

Toute critique est forcément constructive mais tel que ton code est écrit, il est déjà pas mal du tout. En fait, si critique il y a, ce sera presque du détail. Mais bon, puisqu'il faut trouver des trucs à redire, alors trouvons...

Tout d'abord ta fonction "get_racquet_s_options" pourait être écrite ainsi

Code c :

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
int get_racquet_s_options(const char options_file[])
{
    char buffer[50];
 
    FILE *options = NULL;
    options = fopen(options_file, "r");
 
    if(options == NULL)
    {
        fprintf(stderr, "Erreur d'ouverteur du fichier : %s", options_file);
        return -1;
    }
 
    do
    {
        fgets(buffer, sizeof(buffer), options);
 
        if(strstr(buffer, "#COLOR") != NULL)
        {
            get_color(options);
        }
 
        if(strstr(buffer, "#POSITION") != NULL)
        {
            get_pos(options);
        }
 
        if(strstr(buffer, "#COMMAND") != NULL)
        {
            get_command(options);
        }
 
    }while(strstr(buffer, "#END") == NULL);
 
    fclose(options);
    return 0;
}

Déjà le fait de renvoyer une valeur permet de rendre ta fonction "testable" et quitter si fopen() échoue (certains puristes n'aiment pas quitter une fonction au milieu) a l'avantage de te faire gagner un bloc dans l'écriture et surtout ça évite en plus le fclose(NULL) (étourderie).

Si vraiment on veut aller plus loin, on peut alors améliorer sa modularité en intégrant les différentes possibilités dans un tableau qui sera traité séquentiellement

Code c :

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
int get_racquet_s_options(const char options_file[])
{
    char buffer[50];
    typedef struct {
        char *flag;
        void (*fct)(FILE *);
    } t_trt;
 
    t_trt tab_traitement[]={
        {"#COLOR", get_colors},
        {"#POSITION", get_pos},
        {"#COMMAND", get_command},
        {NULL, NULL},
    };
    t_trt *pt_trt;
 
    FILE *options = NULL;
    options = fopen(options_file, "r");
 
    if(options == NULL)
    {
        fprintf(stderr, "Erreur d'ouverteur du fichier : %s", options_file);
        return -1;
    }
 
    do
    {
        fgets(buffer, sizeof(buffer), options);
        for (pt_trt=tab_trt; pt_trt->flag != NULL; pt_trt++)
        {
            if(strstr(buffer, pt_trt->flag) != NULL)
           {
                pt_trt->fct(options);
           }
        }
     }while(strstr(buffer, "#END") == NULL);
 
    fclose(options);
    return 0;
}

Le jour où il faut rajouter un autre traitement, une ligne de plus et c'est réglé. Et tu peux utiliser le même système dans tes autres fonctions get_xxx() pour supprimer ces if() en cascade.

Concernant sizeof il faut faire attention car ça ne fait pas forcément ce que l'on croit. Prends ce bout de code

Code c :

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

1
2
3
4
5
6
7
int main()
{
    char saisie[50];
    fputs("Entrez un truc: ", stdout);
    fgets(tampon, sizeof(tampon), stdin);
    printf("Vous avez entré %s", tampon);
}

Ca fonctionne parfaitement. Puis un jour tu te dis que finalement c'est bien d'avoir une fonction dédiée à la saisie. Tu déportes alors ton code ce qui donnera

Code c :

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

1
2
3
4
5
6
7
8
9
10
11
12
void saisie(char *prompt, char *tampon)
{
    fputs(prompt, stdout);
    fgets(tampon, sizeof(tampon), stdin);
}
 
int main()
{
    char tampon[50];
    saisie("Entrez un truc:", tampon);
    printf("Vous avez entré %s", tampon);
}

Et là, ça clashe. Parce que sizeof(tableau) c'est ok mais sizeof(pointeur) ça donne 4 !!!

Donc perso je préfère toujours utiliser une macro pour mes zones et utiliser cette macro pour la taille de fgets.

Code c :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
#define SIZE_TAMPON      (50)
void saisie(char *prompt, char *tampon)
{
    fputs(prompt, stdout);
    fgets(tampon, SIZE_TAMPON + 1, stdin);
}
 
int main()
{
    char tampon[SIZE_TAMPON + 1];
    saisie("Entrez un truc:", tampon);
    printf("Vous avez entré %s", tampon);
}

En plus, le "+1" permet d'indiquer aux autres lecteurs que t'as bien pensé à l'emplacement pour le '\0' (et aussi que t'es parfaitement au courant que comme fgets() y pense aussi, il enlève systématiquement "1" à la taille demandée...)

Voilà. Sinon je vois rien de spécial. Peut-être éventuellement utiliser un autre nom que "options" pour ton pointeur de fichier...

**Reverse_** · 09/01/2015, 22h35

Bonsoir Sve@r,

merci de tes remarques j'en prends notes et je vais modifier mon code pour l'améliorer.
Par rapport à tes remarques, j'ai juste une question sur :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
 
typedef struct {
        char *flag;
        void (*fct)(FILE *);
    } t_trt;
 
    t_trt tab_traitement[]={
        {"#COLOR", get_colors},
        {"#POSITION", get_pos},
        {"#COMMAND", get_command},
        {NULL, NULL},
    };

le :

Code :

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

void (*fct)(FILE *);

c'est une pointeur sur fonction ?
Si oui, c'est ce qu'on appelle un callback ?

Merci.

**Sve@r** · 10/01/2015, 10h56

Envoyé par Reverse_

Code :

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

void (*fct)(FILE *);

c'est une pointeur sur fonction ?
Si oui, c'est ce qu'on appelle un callback ?

C'est effectivement un pointeur sur fonction. Le nom d'une fonction (sans les parenthèses) correspondant à l'adresse de son code, je stocke cette adresse dans un pointeur apte à la recevoir (pointeur de fonction) que je peux ensuite appeler (en le déréférençant) comme si c'était la vraie fonction.

Ca permet de faire facilement des traitements par lots (exécuter plusieurs fonctions en rafale) à condition que les prototypes de ces fonctions soient les mêmes

Code c :

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
#include <stdio.h>
#include <stdlib.h>
 
int carre(int x)
{
	return x*x;
}
 
int cube(int x)
{
	return x*carre(x);
}
 
int main()
{
	typedef struct {
		char *nom;
		int (*fct)(int);
	} t_work;
	t_work tab[]={
		{"carre", carre},
		{"cube", cube},
		{NULL, NULL},
	};
	t_work *pt;
	for (pt=tab; pt->nom != NULL; pt++)
		printf("%s(%d) -> %d\n", pt->nom, 5, pt->fct(5));
}

Mais ce n'est pas un callback. Un callback ce serait plutôt une fonction qui est appelée par un évènement externe au programme. Par exemple un "signal" (outil unix permettant d'appeler un programme depuis l'extérieur en lui envoyant un "signal" numérique qu'il peut interpréter de la façon qu'il veut)

Code c :

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
#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
 
void callback(int s)
{
	static int cpt=0;
	signal(s, callback);
	printf("Je compte les secondes (%d)\n", cpt++);
	alarm(1);
}
 
int main()
{
	signal(SIGALRM, callback);
	printf("mon pid est %d\n", getpid());
	alarm(1);
	while (1);
}

Dans ce code, (qui ne fonctionnera que sur unix/linux), je programme un callback pour qu'il réagisse au signal 14 (nommé SIGALRM). Ensuite je demande l'envoi d'un signal 14 à moi-même (via la fonction alarm) au bout d'une seconde (un programme peut bien entendu lui-aussi envoyer un signal à lui-même ou à un autre via kill() et la fonction alarm() programme automatiquement l'envoi du signal 14 au bout d'un temps donné) puis je le mets en boucle infinie (pour ne pas qu'il s'arrête)

Et dans le callback, j'affiche un compteur qui s'incrémente. Je prends soin aussi de réarmer le callback (car son appel l'a désarmé) et je lui demande le renvoi du même signal au bout d'une nouvelle seconde.

Dès que mon programme reçoit le signal 14, il appelle de lui-même la fonction sans que j'ai explicitement programmé cet appel à tel ou tel moment et on voit alors les secondes s'incrémenter.

Tu peux en plus l'interfacer depuis l'extérieur (le shell Unix) en appelant la commande kill -14 pid, la valeur "pid" étant son n° de processus. Comme je demande au programme d'afficher cette valeur à son lancement elle est alors facilement connue.

Le callback est la base de la programmation évènementielle (réaction aux évènements extérieurs) qu'on trouve beaucoup dans les IHM (si la souris passe sur tel bouton il se dégrise, etc...)

Et bien évidemment un callback ne fonctionne qu'avec des pointeurs de fonctions. Quand j'appelle signal(SIGALRM, callback), je demande à mon code de stocker l'adresse de la fonction callback() dans son outil de gestion des signaux, adresse qui sera alors appelée/exécutée si le programme reçoit le signal en question ; et l'outil (géré par le noyau Unix) stocke alors forcément cette adresse de son coté dans un pointeur de fonction...

Demande de critiques constructives - Code C

C

Discussions similaires

Partager

Partager