Conseils pour améliorer mon code

Bonjour,

je suis stagiaire et je travaille à la mise en place d'un genre de client/serveur entre deux ordinateur, ceux-ci relié par des MODEM Acoustiques. Mon code fonctionne mais étant mauvais en programmation, je souhaitais quelques conseils pour pouvoir l'améliorer. Merci beaucoup pour votre aide.

Voici mon code :

Code:

---

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 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262 263 264 265 266 267 268 269 270 271 272 273 274 275 276 277 278 279 280 281

#include <WinSock2.h> #include <Windows.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <time.h>     #pragma comment(lib, "ws2_32.lib")   #define SEQ "+++" #define SIZE 250   void my_delay(int i); void delay_s(int i); void empty_buff(char * b);     int main() {   // Declaration des variables   WSADATA wsa; SOCKET sock; SOCKADDR_IN addr_in;   char buff_recv[SIZE]; char buff_send[SIZE]; char buff_affichage[SIZE]; char buff_passage_cmd[4];   char nomFichier[30]; char* nomFichierAEnvoyer = "fichier a envoyer"; FILE* fichier = NULL;   //variables d'etat des sockets u_long bloquant = 0; u_long non_bloquant = 1;   //variable de gestion du programme char boucle = 0; char choix[3];   char decision_r = 0; char decision_e = 0; char decision_q = 0; char decision_c = 0; char decision_fs = 0; char decision_fr = 0;   char cmd_act = 0; int n = 0;   //gestion des retours de fonctions int connection = 1; int octets_recus = 1; int octets_envoyes = 1;   int total_octets_recus = 0; int error = 1;   // Vidage des buffers d'envoi et de réception empty_buff(buff_recv); empty_buff(buff_send);   // Initialisation de la socket WSAStartup(MAKEWORD(2,0), &wsa); sock = socket(AF_INET, SOCK_STREAM, 0); addr_in.sin_addr.s_addr = inet_addr("192.168.0.144"); addr_in.sin_family = AF_INET; addr_in.sin_port = htons(9200);   //création et remplissage du fichier à envoyer /*do{ printf("Entrer le nom du fichier a creer/remplir, preciser le format\n"); gets(nomFichier); fichier = fopen(nomFichier, "w+"); if(fichier != NULL) { fputs("Debut du fichier de O\n ooooooooooooooooooooooooooooooooooo\n!!!!\n", fichier); fclose(fichier); } else printf("impossible d'ouvrir le fichier %s", nomFichier); }while(fichier == NULL); */ // Connection au serveur do { connection = connect(sock, (SOCKADDR*) &addr_in, sizeof(addr_in));// On se connecte à notre serveur (dans notre cas, le MODEM Acoustique) }while(connection != 0);   if(!connection) { printf("Connection etablie\n"); }   // Mode Socket non bloquante ioctlsocket(sock,FIONBIO,&non_bloquant);   // Boucle de communication while(!boucle) { printf("Choix du traitement : \nR : Reception de data\nE : Envoi de data\nC : Envoi de commande\n1 : Reception de fichier\n2 : Envoi de fichier\nQ : Quitter\n");   printf("decision : "); n = getchar(); sprintf(choix,"%d", n); decision_r = (strcmp("82",choix)==0); decision_e = (strcmp("69",choix)==0); decision_q = (strcmp("81",choix)==0); decision_c = (strcmp("67",choix)==0); decision_fs = (strcmp("50",choix)==0); decision_fr = (strcmp("49",choix)==0);   fflush(stdin); if(decision_r) { empty_buff(buff_recv); do{ octets_recus = recv(sock, buff_recv, sizeof(buff_recv), 0); if(octets_recus > 0) { printf("%d octets recus : %s\n", octets_recus, buff_recv); total_octets_recus += octets_recus; //printf("Affichage de notre buffer de reception : %s \n", buff_recv);   }else error = WSAGetLastError(); }while(error != 10035);   empty_buff(buff_recv); total_octets_recus = 0;   }else if(decision_e) { empty_buff(buff_send);   //scanf("%SIZE2s", buff_send); fgets(buff_send,SIZE, stdin); send(sock, buff_send, sizeof(buff_send),0); // Fonction d'envoi de chaine de caractère à notre serveur printf("%s", buff_send); empty_buff(buff_send);   }else if (decision_q) { shutdown(sock, SD_BOTH); closesocket(sock); WSACleanup(); boucle = 1;   }else if (decision_c) { do { empty_buff(buff_passage_cmd); send(sock, "+++ATC\n", sizeof("+++ATC\n"), 0); error = recv(sock, buff_passage_cmd, sizeof(buff_passage_cmd),0); if ( error > 0 ) printf("Octets recus: %d\n", error);   printf("%s\n", buff_passage_cmd);   if(strncmp(buff_passage_cmd, "OK\r\n",4) != 0) { printf("Echec passage en mode commande\n"); }else { printf("Mode commande active\n"); cmd_act = 1; } my_delay(1);   }while(!cmd_act);   while(cmd_act) { printf("Entrez votre commande : \n"); scanf("%s",buff_send); strcat(buff_send,"\n"); send(sock,buff_send,sizeof(buff_send),0); if(strcmp("ATO\n",buff_send) == 0) { printf("Mode Data Activé\n"); cmd_act = 0; }else { do { error = recv(sock, buff_recv, sizeof(buff_recv),0);   if ( error > 0 ) { printf("Octets recus: %d\n", error); printf("%s\n", buff_recv); }else { error = WSAGetLastError(); }   }while(error != 10035); } } }else if(decision_fs) { fichier = fopen("nomFichierAEnvoyer.txt", "r+"); empty_buff(buff_send); if(fichier != NULL) { while(fgets(buff_send, SIZE, fichier) != NULL) { printf("%s\n", buff_send); send(sock,buff_send,sizeof(buff_send),0); empty_buff(buff_send); } fclose(fichier); }else printf("Erreur dans l'ouverture du fichier %s", nomFichier);       }else if(decision_fr) { fichier = fopen("fichier recu.txt", "w+"); if(fichier != NULL) { empty_buff(buff_recv); do{ octets_recus = recv(sock, buff_recv, sizeof(buff_recv), 0); if(octets_recus > 0) { printf("%d octets recus : %s\n", octets_recus, buff_recv); total_octets_recus += octets_recus;   }else error = WSAGetLastError(); }while(error != 10035); fputs(buff_recv, fichier); empty_buff(buff_recv); fclose(fichier); }else printf("Erreur dans l'ouverture du fichier ""fichier recu""");   }             //empty_buff(choix); fflush(stdin);     }   // Mise en pause du système pour pouvoir lire ce qu'il y a dans la console system("pause");     // Fin du programme return 0 ; }       void delay_s(int i) // Pause l'application pour i seconds (non blocant) { i*=1000; Sleep(i); }   void my_delay(int i) // Pause l'application pour i seconds { clock_t start,end; start=clock(); while(((end=clock())-start)<=i*CLOCKS_PER_SEC); }   void empty_buff(char * b) { memset(b,0,sizeof(b)); }

---

Pour commencer, remplace ta prise de décision par une conversion char* -> int (avec la fonction strtol()) et un switch.

Code:

---

1 2 3 4

void empty_buff(char * b) { memset(b,0,sizeof(b)); }

---

Cette fonction est fausse.
En effet, sizeof(b) est sizeof(char*).
C'est la taille d'un pointeur, qui n'a rien à voir avec le tableau que tu pourrais vouloir réinitialiser.
C'est bien pour cela que memset prend une taille en argument.

Code:

---

fflush(stdin);

---

Ce n'est conseillé de faire ça : http://c.developpez.com/faq/?page=Ge...buffer-clavier

Code:

---

system("pause");

---

Ne marche pas si le système de la commande "pause" (et ça arrive plus souvent qu'on ne le pense). Utilise plutôt getchar().

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12

void delay_s(int i) // Pause l'application pour i seconds (non blocant) { i*=1000; Sleep(i); }   void my_delay(int i) // Pause l'application pour i seconds { clock_t start,end; start=clock(); while(((end=clock())-start)<=i*CLOCKS_PER_SEC); }

---

Sleep() est bloquante (arrrg ! la faute de français !)donc delay_s le sera aussi. Pourquoi faire 2 fonctions qui font la même chose ? Pourquoi ne pas mettre des noms plus explicites comme delay_second(int seconds) ?

Merci pour vos réponses, les changements apportés ont allégés mon code (beaucoup de variables ne sont plus utilisées). Pour "memset", j'ai regardé le prototype et j'ai compris mon erreur. Voici mon code après les modifications :

Code:

---

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 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262

#include <WinSock2.h> #include <Windows.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <time.h>     #pragma comment(lib, "ws2_32.lib")   #define SEQ "+++" #define SIZE 250   void empty_buff(char * b); void delay_seconds(int seconds); void clean_stdin();   int main() {   // Declaration des variables   WSADATA wsa; SOCKET sock; SOCKADDR_IN addr_in;   char buff_recv[SIZE]; char buff_send[SIZE];   char buff_passage_cmd[4];   char nomFichier[30]; char* nomFichierAEnvoyer = "fichier a envoyer"; FILE* fichier = NULL;   //variables d'etat des sockets u_long bloquant = 0; u_long non_bloquant = 1;   //variable de gestion du programme char boucle = 0;   int decision = 0;   char cmd_act = 0; int n = 0;   //gestion des retours de fonctions int connection = 1; int octets_recus = 1; int octets_envoyes = 1;   int total_octets_recus = 0; int error = 1;   // Vidage des buffers d'envoi et de réception empty_buff(buff_recv); empty_buff(buff_send);   // Initialisation de la socket WSAStartup(MAKEWORD(2,0), &wsa); sock = socket(AF_INET, SOCK_STREAM, 0); addr_in.sin_addr.s_addr = inet_addr("192.168.0.144"); addr_in.sin_family = AF_INET; addr_in.sin_port = htons(9200);   //création et remplissage du fichier à envoyer do{ printf("Entrer le nom du fichier a creer/remplir, preciser le format\n"); gets(nomFichier); fichier = fopen(nomFichier, "w+"); if(fichier != NULL) { fputs("Debut du fichier de O\n ooooooooooooooooooooooooooooooooooo\n!!!!\n", fichier); fclose(fichier); } else printf("impossible d'ouvrir le fichier %s", nomFichier); }while(fichier == NULL);   // Connection au serveur do { connection = connect(sock, (SOCKADDR*) &addr_in, sizeof(addr_in));// On se connecte à notre serveur (dans notre cas, le MODEM Acoustique) }while(connection != 0);   if(!connection) { printf("Connection etablie\n"); }   // Mode Socket non bloquante ioctlsocket(sock,FIONBIO,&non_bloquant);   // Boucle de communication while(!boucle) { printf("Choix du traitement : \nR : Reception de data\nE : Envoi de data\nC : Envoi de commande\n1 : Reception de fichier\n2 : Envoi de fichier\nQ : Quitter\n");   printf("decision : "); decision = getchar();   clean_stdin(); switch(decision) { case 82: empty_buff(buff_recv); do{ octets_recus = recv(sock, buff_recv, sizeof(buff_recv), 0); if(octets_recus > 0) { printf("%d octets recus : %s\n", octets_recus, buff_recv); total_octets_recus += octets_recus; //printf("Affichage de notre buffer de reception : %s \n", buff_recv);   }else error = WSAGetLastError(); }while(error != 10035);   empty_buff(buff_recv); total_octets_recus = 0; break; case 69:   empty_buff(buff_send);   //scanf("%SIZE2s", buff_send); fgets(buff_send,SIZE, stdin); send(sock, buff_send, sizeof(buff_send),0); // Fonction d'envoi de chaine de caractère à notre serveur printf("%s", buff_send); empty_buff(buff_send); break; case 81:   shutdown(sock, SD_BOTH); closesocket(sock); WSACleanup(); boucle = 1; break; case 67 do { empty_buff(buff_passage_cmd); send(sock, "+++ATC\n", sizeof("+++ATC\n"), 0); error = recv(sock, buff_passage_cmd, sizeof(buff_passage_cmd),0); if ( error > 0 ) printf("Octets recus: %d\n", error);   printf("%s\n", buff_passage_cmd);   if(strncmp(buff_passage_cmd, "OK\r\n",4) != 0) { printf("Echec passage en mode commande\n"); }else { printf("Mode commande active\n"); cmd_act = 1; } delay_seconds(1);   }while(!cmd_act);   while(cmd_act) { printf("Entrez votre commande : \n"); scanf("%s",buff_send); strcat(buff_send,"\n"); send(sock,buff_send,sizeof(buff_send),0); if(strcmp("ATO\n",buff_send) == 0) { printf("Mode Data Activé\n"); cmd_act = 0; }else { do { error = recv(sock, buff_recv, sizeof(buff_recv),0);   if ( error > 0 ) { printf("Octets recus: %d\n", error); printf("%s\n", buff_recv); }else { //printf("Erreur de reception, code : %d\n", WSAGetLastError()); error = WSAGetLastError(); }   }while(error != 10035); } } break; case 49 :   fichier = fopen("nomFichierAEnvoyer.txt", "r+"); empty_buff(buff_send); if(fichier != NULL) { while(fgets(buff_send, SIZE, fichier) != NULL) { printf("%s\n", buff_send); send(sock,buff_send,sizeof(buff_send),0); empty_buff(buff_send); } fclose(fichier); }else printf("Erreur dans l'ouverture du fichier %s", nomFichier);       break; case 50 :   fichier = fopen("fichier recu.txt", "w+"); if(fichier != NULL) { empty_buff(buff_recv); do{ octets_recus = recv(sock, buff_recv, sizeof(buff_recv), 0); if(octets_recus > 0) { printf("%d octets recus : %s\n", octets_recus, buff_recv); total_octets_recus += octets_recus; //printf("Affichage de notre buffer de reception : %s \n", buff_recv);   }else error = WSAGetLastError(); }while(error != 10035); fputs(buff_recv, fichier); empty_buff(buff_recv); fclose(fichier); }else printf("Erreur dans l'ouverture du fichier ""fichier recu"""); break; } clean_stdin(); }   // Mise en pause du système pour pouvoir lire ce qu'il y a dans la console getchar();;     // Fin du programme return 0 ; }   void delay_seconds(int seconds) // Pause l'application pour i seconds { clock_t start,end; start=clock(); while(((end=clock())-start)<=seconds*CLOCKS_PER_SEC); }   void empty_buff(char * b) { memset(b,0,SIZE); }   void clean_stdin() { int c; do { c = getchar(); }while(c != '\n' && c != EOF); }

---

Bonjour

Ton code n'est pas trop mal écrit. Bien entendu tu manques d'habitude et ça se sent mais c'est prometteur.

Quelques remarques

• évite les nombres magiques => while(error != 10035). Pourquoi 10035 ? Que signifie ce nombre ? Certainement qu'il y a quelque part un include approprié qui te permettra d'écrire à la place while(error != ERROR_TRUC). Non seulement c'est plus parlant mais en plus ça facilite la portabilité
• prends l'habitude de terminer tes printf() par un "\n". Déjà l'affichage est alors immédiat mais en plus cet affichage est propre à l'écran (au lieu d'avoir des messages se succèdant à la file les uns des autres).
• c'est dommage un gros main énorme et assez imbuvable. Essaye de découper tes tâches en petites fonctions dédiées. Ainsi une fonction pour établir ta connexion, une autre pour le dialogue etc.
• à l'inverse du point précédent, il est inutile de créer une fonction empty_buf() qui ne fait rien d'autre que d'appeler memset(). Même si le nom de l'autre ne te plait pas, avec l'habitude ça te parlera et ça deviendra ensuite tout aussi simple d'utiliser directement memset().
• évite sizeof autant que possible. Leternel explique bien pourquoi. sizeof(tableau) renverra la taille du tableau mais sizeof(pointeur) renverra la taille du pointeur (c.a.d. 4). Or suffit que tu déportes un travail manipulant un tableau et utilisant sizeof()dans une fonction recevant alors le tableau sous forme de pointeur pour que le code ne fonctionne plus.

Ca me semble à peu près tout. Ah oui bien entendu on ne fflushe jamais stdin. Tu dois en permanence maitriser ton clavier et savoir ce qu'il contient et pourquoi ça le contient et comment alors vider proprement ce qui ne convient pas. Surtout qu'un programme contenant un fflush(stdin) (même dans le cas où cette instruction est implémentée) se comportera de façon anarchique si tu le places derrière un pipe (ce qui arrive parfois dans le monde Unix/Linux).

Bon courage ;)

Merci pour tous vos conseils. Je comptais le passer sous forme de petites fonctions mais je me sentais plus à l'aise de le tester comme ça au début. Je m'y met maintenant. Je vous recontacterai si je rencontre d'autres difficultés. Encore merci pour tout.