Mauvais élève en C++

Version imprimable

1 pièce(s) jointe(s)

Bonjour,

Je suis nouveau sur ce site, un peu moins en programmation mais presque, mais j'ai appris sur le tas, en m'inspirant des gens qui m'entourent (qui ne sont pas programmeurs à la base).
En me promenant sur le site, je me rends compte que je fais plein de trucs sales en C++ (#define, char*, FILE*, tableaux multidim, fichier monobloc...).

Il y a tellement de trucs à changer en même temps que je suis perdu, et c'est là que j'ai besoin de vous!

J'ai essayé de concentrer ces problèmes en coupant dans un programme que j'utilise et en le simplifiant. C'est un programme qui lit des tableau dans des fichiers (dans sa version complete pouvant lire différents types de fichiers) et mettant les données dans un tableau de format générique.

Voilà le 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 #include <fstream> #include <stdio.h> #include <iostream> using namespace std; #define NbAntMax 2 // Nombre de signaux #define NbChTimeMax 10001 // Nombre de canaux temporel max absolu #define NbChTimeAA 1000 // Nombre max AA (normalement il y a des signaux BB, avec une autre longueur) #define NbTypesSignaux 5 // Temps, NS, EW, vertical, autre parametre // Tableau destines a contenir les signaux float signalsAA[NbAntMax][NbTypesSignaux][NbChTimeMax]; // Declaration des fonction de lecture des fichiers void Read1FileAA(char* AAdir,char* AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant); void ReadAllFilesAA(char* AAdir,char* AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax]); // Chemin des fichiers de donnees AA char* dirAA="./dataAA/"; char* fileAA[NbAntMax]={"file1","file2"}; int main() { ReadAllFilesAA(dirAA,fileAA,signalsAA); printf("Last value: %3f\n",signalsAA[1][3][999]); return 0; } // ReadAllFileAA: Lit les trois composantes du champ de tous les fichiers AA void ReadAllFilesAA(char* AAdir,char* AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax]) { for(int i=0;i<NbAntMax;i++) { Read1FileAA(AAdir,AAfile[i],signals,i); } } // Read1FileAA: Lit les trois composantes du champ d un fichier AA a une position void Read1FileAA(char* AAdir,char* AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant) { int ii=ant; char AAdirfile[100]; sprintf(AAdirfile,"%s%s.dat",AAdir,AAfile); FILE *fAnt; if((fAnt = fopen(AAdirfile,"r"))==NULL) {printf("Cant open AA file: %s\n",AAdirfile);} for(int jj=0;jj<2012;jj++) { fscanf(fAnt,"%f",&signals[ii][0][jj]); // Time fscanf(fAnt,"%f",&signals[ii][1][jj]); // ns fscanf(fAnt,"%f",&signals[ii][2][jj]); // ew fscanf(fAnt,"%f",&signals[ii][3][jj]); // vert signals[ii][4][jj]=999; // nothing, utilise avec les signaux BB... } }
Je vous laisse admirer le style :cfou:

Vous pouvez m'aider à améliorer ça, histoire de repartir sur de bonnes bases?

Pour commencer, j'ai essayé hier de mettre les fonctions de lectures dans un fichier .cxx séparé, plus un header (appelé après les #define), mais à la compilation j'ai un message
undefined reference to ReadAllFilesAA.

:help:

PS: En fichier joint, le code plus les données pour que ça tourne
PS2: J'ai déjà écrit des milliers de lignes de C++(?) avec ce style :cry:

22/08/2007, 12h11
screetch

Deja ce n'est pas du C++ c'est du C ca ^^

ensuite je vais manger, je profite juste de cela pour augmenter de maniere artificielle mon nombre de posts.
22/08/2007, 12h22
craryb

si t as ecrit des milliers de lignes en C alors que tu croyais faire du C++, chapeau bas ! :aie:
22/08/2007, 12h35
keldorn

Dans les parties zapées, il y avait pas mal d'utilisations d'objets ROOT, que j'utilise assez intensivement par ailleurs.
Mais c'est vrai qu'a part ça, j'écris surtout du C. Comme je galère régulièrement (avec les appels de tableau multidims dans les fonctions par exemple) comme ça, et que je pense que ce serait plus simple avec des objets(c'est pour aller dans ce sens que je veux mettre les fonctions dans un fichier séparé), et plein d'autres raisons, que je veux passer au C++.

edit: C'est vrai que j'aurrais pu faire un effort pour mettre un cout à la place du printf, ça je sais faire (mais j'ai moins l'habitude)
22/08/2007, 14h53
jph_void

Salut,
effectivement c'est spécial pour du c++ ^^

"Il y a tellement de truc a changer, c'est la que j'ai besoin de vous" : Les gens qui sont sur le forum sont pas la non plus pour te donner un cour particulier :p
Tu es sur un super site avec plein de tutos, tu devrais trouver pas mal de renseignements pour arriver a tes fins.
Sinon il y a aussi les librairies... Non je parles des batiments avec des gens qui vendent des livres ^^
22/08/2007, 14h58
aoyou
Déjà, tu peux
- enlever <stdio.h>
- remplacer #define var value par static const int var = value;
- bannir char * et n'utiliser que des strings
- remplacer printf par cout
- plus besoin de sprintf : concaténer les strings (string AAdirfile = AAdir + AAfile + ".dat";)
- Ne pas utiliser FILE mais les fstream
- Plus de scanf avec les fstream (préférer fAnt >> signals[ii][0][jj] >> ...)
- Ne pas oublier de fermer le fichier une fois que tu as terminé avec
- Mettre les fonctions de lecture dans un fichier à part (réessaie, ça doit marcher, sûrement une erreur dans ta ligne de commande). En faire une classe ?
- Le tableau multidimensionnel, bof pourquoi pas, tu peux-etre définir une classe qui correspondrait à une ligne du tableau et utiliser un vecteur à la place, c'est toi qui voit ce que tu veux faire.
- Regarder un livre de C++.
Bon bah voilà, ça te fait déjà un paquets de choses à faire pour passer en C++.

N'hésite pas à nous dire comment se passe ta reconversion.

1 pièce(s) jointe(s)

Merci pour vos réponses.

aoyou a dit:

Citation:

enlever <stdio.h>
remplacer printf par cout
Ne pas oublier de fermer le fichier une fois que tu as terminé avec
Mettre les fonctions de lecture dans un fichier à part (réessaie, ça doit marcher, sûrement une erreur dans ta ligne de commande). En faire une classe ?

Les trois premiers points sont fait, et je vais m'attaquer aux autres après avoir reglé le 4ème: quand je divise le code, je n'arrive plus a compiler. J'ai les fichiers:

lecture2.C

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
 
#include <fstream>
#include <iostream>
 
using namespace std;
 
#define NbAntMax 2		// Nombre de signaux
#define NbChTimeMax 10001	// Nombre de canaux temporel max absolu
#define NbChTimeAA 1000		// Nombre max AA (normalement il y a des signaux BB, avec une autre longueur)
#define NbTypesSignaux 5	// Temps, NS, EW, vertical, autre parametre
 
#include "fonctions.h"
 
// Tableau destines a contenir les signaux
float signalsAA[NbAntMax][NbTypesSignaux][NbChTimeMax];
 
 
// Chemin des fichiers de donnees AA
char* dirAA="./dataAA/";
char* fileAA[NbAntMax]={"file1","file2"};
 
int main()
{
  ReadAllFilesAA(dirAA,fileAA,signalsAA);
  cout<<"Last value: "<<signalsAA[1][3][999]<<endl;
  return 0;
}

fonctions.h:

Code:

1
2
3
4
5
6
7
8
9
 
#ifndef FONCTIONS_H
#define FONCTIONS_H
 
 // Declaration des fonction de lecture des fichiers
void Read1FileAA(char* AAdir,char* AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant);
void ReadAllFilesAA(char* AAdir,char* AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax]);
 
#endif

et fonctions.cxx:

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
 
#include <fstream>
#include <iostream>
 
using namespace std;
 
#include "fonctions.h"
 
// ReadAllFileAA: Lit les trois composantes du champ de tous les fichiers AA
void ReadAllFilesAA(char* AAdir,char* AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax])
{
  for(int i=0;i<NbAntMax;i++) {
    Read1FileAA(AAdir,AAfile[i],signals,i);
  }
}
 
// Read1FileAA: Lit les trois composantes du champ d un fichier AA a une position
void Read1FileAA(char* AAdir,char* AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant)
{
  int ii=ant;
 
  char AAdirfile[100];
  sprintf(AAdirfile,"%s%s.dat",AAdir,AAfile);
 
  FILE *fAnt;
  if((fAnt = fopen(AAdirfile,"r"))==NULL) {printf("Cant open AA file: %s\n",AAdirfile);}
 
  for(int jj=0;jj<2012;jj++) {
    fscanf(fAnt,"%f",&signals[ii][0][jj]);	// Time
    fscanf(fAnt,"%f",&signals[ii][1][jj]);	// ns
    fscanf(fAnt,"%f",&signals[ii][2][jj]);	// ew
    fscanf(fAnt,"%f",&signals[ii][3][jj]);	// vert
    signals[ii][4][jj]=999;			// nothing, utilise avec les signaux BB...
  }
  fclose(fAnt);
}

mais quand je compile avec

Citation:

g++ lecture2.C -o LECT

, j'obtiens le message d'erreur

Citation:

/tmp/ccBkVWLD.o(.text+0x12c): In function `main':
: undefined reference to `ReadAllFilesAA(char*, char**, float (*) [5][10001])'
collect2: ld returned 1 exit status

Qu'est ce qui ne va pas?

Merci pour votre temps (j'ai passé toute la journée d'hier à essayer de résoudre ce point tout seul sur ce site et d'autres, sans succès)

Keld.

22/08/2007, 15h46
Laurent Gomila

Tu essayes de créer l'exécutable LECT uniquement en compilant / liant lecture2.C. Il faut ajouter fonctions.cxx pour que la fonction ReadAllFilesAA soit trouvée.
22/08/2007, 16h00
keldorn

Du coup je dois faire comment?

Compiler les deux d'un coup (via une commande que je ne connais pas), ou compiler d'abord fonctions.cxx (mais elle a besoin des constantes définies dans le programme principal - à modifier?)?
22/08/2007, 16h14
aoyou
Après avoir déplacé tes constantes dans le fichier.h (oui), tu peux faire

Code:

g++ -o LECT lecture2.C fonctions.cxx

A faire: réviser les processus de compilation, c'est pareil en C qu'en C++.
Si on décompose,
Code:

1 2 3 4 g++ -c fonctions.cxx g++ -c lecture2.C g++ -o LECT lecture2.o fonctions.o
L'extension .C c'est choquant. Préfère .c.

Au fait, c'est pas encore vraiment du C++ tout ça.
22/08/2007, 21h20
keldorn

Merci, ça marche maintenant. Je vais essayer de bosser un peu dans mon coin avant de reposter.

Voilà ce que ça donne, je pense que j'ai fait le tour:

lecture2.cpp:

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
 
#include <iostream>
#include <string>
 
using namespace std;
 
#include "fonctions.h"
 
// Tableau destines a contenir les signaux
float signalsAA[NbAntMax][NbTypesSignaux][NbChTimeMax];
 
 
// Chemin des fichiers de donnees AA
string dirAA="./dataAA/";
string fileAA[NbAntMax]={"file1","file2"};
 
int main()
{
  ReadAllFilesAA(dirAA,fileAA,signalsAA);
  cout<<"Last value: "<<signalsAA[1][3][999]<<endl;
  return 0;
}

fonctions.h:

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
#ifndef FONCTIONS_H
#define FONCTIONS_H
 
static const int NbAntMax=2;		// Nombre de signaux
static const int NbChTimeMax=10001;	// Nombre de canaux temporel max absolu
static const int NbChTimeAA=1000;	// Nombre max AA (normalement il y a des signaux BB, avec une autre longueur)
static const int NbTypesSignaux=5;	// Temps, NS, EW, vertical, autre parametre
 
 // Declaration des fonction de lecture des fichiers
void Read1FileAA(string AAdir,string AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant);
void ReadAllFilesAA(string AAdir,string AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax]);
 
#endif

fonctions.cxx:

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
 
#include <fstream>
#include <iostream>
#include <string>
 
using namespace std;
 
#include "fonctions.h"
 
// ReadAllFileAA: Lit les trois composantes du champ de tous les fichiers AA
void ReadAllFilesAA(string AAdir,string AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax])
{
  for(int i=0;i<NbAntMax;i++) {
    Read1FileAA(AAdir,AAfile[i],signals,i);
  }
}
 
// Read1FileAA: Lit les trois composantes du champ d un fichier AA a une position
void Read1FileAA(string AAdir,string AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant)
{
  int ii=ant;
 
  string AAdirfile;
  AAdirfile=AAdir+AAfile+".dat";
 
  ifstream fAnt(AAdirfile.c_str());
  if(!fAnt) cout<<"Cant open AA file: "<<AAdirfile<<endl;
 
  for(int jj=0;jj<NbChTimeAA;jj++) {
    fAnt>>signals[ii][0][jj]>>signals[ii][1][jj]>>signals[ii][2][jj]>>signals[ii][3][jj];
    signals[ii][4][jj]=999;
  }
  fAnt.close();
}

Il ne reste rien de choquant? Qu'aurriez vous fait différemment?
Sinon, j'ai encore une question: Si j'ai bien compri, c'est mieux de mettre const à la place de #define pour définir une constante car le compilateur effectue une vérification de type, mais je ne comprend pas l'intéret du static devant?

Merci,
Keld.

23/08/2007, 14h00
Alp

Le mieux serait que tu jettes un oeil à la FAQ C++. Beaucoup de ces points y sont traités.

http://cpp.developpez.com/faq/cpp/
23/08/2007, 14h30
keldorn

Donc pour le static, si j'ai compris (faq et les bouquins sur mon bureau), il sert uniquement à rendre visible une variable à l'extérieur de la structure dnas laquelle elle est déclarée, à la rendre 'globale'.

Dans mon cas, les déclarations des constantes n'étant pas dans une structure, le static est inutile, je peux le supprimer, ca ne changera rien.
23/08/2007, 18h20
keldorn

En tout cas merci à tous, ça m'a beaucoup aidé!

A plus,
Keld.
23/08/2007, 18h31
aoyou
Comme ton fichier fonctions.h ressemble plutôt à du C, j'aurais plutôt lu la FAQ C.

Citation:

Sur une variable globale, le modificateur static permet de réduire la portée de la variable au fichier où elle est déclarée.

Donc, non le static n'est pas inutile.

Citation:

Qu'aurriez vous fait différemment?

Tu n'as pas besoin de savoir que Read1FileAA existe pour utiliser ReadAllFilesAA. Sa déclaration n'est donc utile que dans fonctions.cxx.

Après, bah, ça fait programmation impérative écrite en C++ alors que le C++ est avant tout un langage orienté objet.

Tu pourrais encapsuler ton tableau

Code:

float signalsAA[NbAntMax][NbTypesSignaux][NbChTimeMax];

dans une classe et écrire deux méthodes publiques (Tel que c'est, ton tableau n'est pas à l'abri de tout danger et malveillance)
Code:

1 2 3 void ReadAllFilesAA(string AAdir,string AAfile[NbAntMax]); // Remplit le tableau float getSignalsAA(int, int, int); // Accède à une donnée du tableau
Bon bah voilà c'est une idée mais c'est toi qui sait le mieux ce que tu as besoin de faire.

Salut,

En fait, il faut savoir que, que ce soit en C ou en C++, il est déconseillé d'avoir recours aux variables globales...

La raison en est simple: Elles sont disponibles de ... partout :P (tu me diras que c'est logique, vu qu'elles sont globales :D)

L'astuce, c'est que, d'un côté, tu cours le risque de "cacher" une variable globale en déclarant une variable du même nom dans une portée
Code:

1 2 3 4 5 6 7 8 int cpt; void MaFonct() { int cpt; /*...*/ ++cpt;/*La variable globale? hé non, la variable locale :P */ }
D'un autre, selon l'ordre d'appel des fonctions, tu risques de partir sur des hypothèses fausses qui peuvent mener à des catastrophes:
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 int* tab; int cpt=3; void init() { tab=new int[cpt]; } void liberer() { delete[] tab; } void utiliser() { /* normalement init a été appelé avant utilise ... * Mais si ce n'est pas le cas ???? * ... Et si liberer a été appelé ???? */ tab[cpt-1]=...; }
Tu va, lorsque tu décidera d'appeler utiliser, partir de l'hypothèse que init() a déjà été appelé et que liberer() ne l'a surtout pas encore été... Mais que va-t-il se passer si ce n'est pas le cas :question:

De plus, il y a très peu de variables globales qui sont réellement appelées à... être utilisées partout :P

Dans les quatre variables globales que tu déclares, certaines ne vont en fait être utilisées que de manière "très ponctuelle" (dans une, voire deux fonctions au mieux)

Du point de vue du mot clé static, il faut savoir qu'en fait il agit sur la manière dont l'éditeur de liens va effectuer la liaison des données avec le reste de l'application.

Il aura pour effet de rendre une variable accessible uniquement dans l'unité de compilation dans laquelle elle est déclarée (autrement dit: uniquement dans les *.cpp dans lesquels le jeu d'inclusion des fichiers fait que le *.h dans lequel la variable est déclarée est inclus).

On dit alors qu'elle a une liaison interne des données.

Cela permet de "gérer" la portée de la variable plus finement:

Dans les gros projets, on se rend souvent compte que, si on veut utiliser des variables globales (ou qu'on "doit" le faire), elles ne seront nécessaires que dans un nombre restreint d'unités de compilation (cf l'une de mes remarques concernant les variables globales).

L'astuce est toujours la même: si tu rend disponible dans une unité de compilation une variable qui n'est pas utile à cette unité de compilation, tu risque toujours d'être tenté d'utiliser (ou pire: de modifier) cette variable, avec des résultat qui peuvent être catastrophiques.

Par contre, le risque diminuera d'autant plus que... tu rendra cette variable accessible dans peu d'unités de compilation ;)

D'un autre coté, pour une série de chose, tu devrais envisager l'utilisation de références plutôt que d'objets, et tu devrais faire attention à la "const correctness" (le fait de déclarer un un argument constant si tu n'a pas l'intention de modifier la valeur qu'il représente)

L'utilisation de références plutôt que d'objets permet d'éviter un certain nombre de copies inutile, et donc de rendre certains appels de fonction "plus efficaces".

La "const correctness" te permettra d'un autre côté de te faire "insulter" par le compilateur si, par erreur, tu essaye de modifier quelque chose... que tu t'étais engagé à ne pas modifier.

Enfin, l'utilisation de références constantes peut te permettre de mettre en oeuvre certaines conversions automatiques de type (typiquement: un const char* peut être converti automatiquement en std::string)

Ainsi, pour les fonctions
Code:

1 2 void Read1FileAA(string AAdir,string AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant); void ReadAllFilesAA(string AAdir,string AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax]);
il serait pas mal d'utiliser une référence sur une chaîne pour AAdir, histoire d'éviter la recopie inutile, mais en plus (car je présumes qu'il n'est pas dans ton intention de la modifier) de t'engager à ne pas modifier cette chaîne.

L'avantage "extra" est que les fonctions deviendront compatible avec des const char*

Le prototype des fonctions deviendrait donc
Code:

1 2 void Read1FileAA(const string& AAdir, const string& AAfile,float signals[NbAntMax][NbTypesSignaux][NbChTimeMax],int ant); void ReadAllFilesAA(const string& AAdir,string AAfile[NbAntMax],float signals[NbAntMax][NbTypesSignaux][NbChTimeMax]);
On pourrait ajouter le fait que, idéalement, il vaut mieux éviter d'avoir recours à des tableaux multi dimentionnels (signals [][][])... Cela nécessite une gestion un peu différente, mais pas *vraiment* plus compliquée:

Ainsi un tableau tri dimentionnel du genre de

Code:

float signals[x][y][z];

pourrait être avantageusement remplacé par un tableau à une dimention

Code:

float signals[x*y*z];

La gestion en serait différente parce qu'au lieu d'accéder à un élément bien précis sous la forme de

Code:

signals[3][2][5]=...

il faudrait y accéder sous la forme de

Code:

signals[(3*y*z)+(2*z)+5]=...

mais cela t'évitera pas mal de problèmes, entre autres, lorsqu'il y aura une fonction qui doit renvoyer le tableau ;)

Pour terminer - car, une fois n'est pas coutume, je me suis laissé emporter par ma verve- je vais signaler le fait que, idéalement, il est préférable d'éviter d'avoir recours aux tableaux "C style" (string AAfile[], float signals[][][]....), que la STL fournit une quantité impressionnante de conteneurs qui sécurisent grandement la gestion d'éléments multiples.

Leur grand intérêt, en plus d'être sécurisants à l'utilisation, étant qu'ils peuvent tous être passés sous forme de référence (constante ou non), avec les avantages dont j'ai parlé plus haut ;)

Par exemple, il existe la classe std::vector qui fournit exactement les mêmes capacités, et bien plus encore, la sécurité en plus, qu'un tableau "C style".

Tu pourrait envisager de l'utiliser pour le tableau AAfile[], et meme pour la matrice signals[][][] sous une forme proche de
Code:

1 2 3 4 5 std::vector<std::string> AAfile; /*et*/ std::vector<std::vector<std::vector<float> > > signals; /* à moins que tu ne préfère gérer les trois dimensions en une: */ std::vector<float> signals:
Comme il est plus que vraisemblable que tu n'aie aucune intention de modifier les noms représentés dans AAfile, tu pourra donc le passer sous forme de référence constante, et comme l'idée est, sans doute, de remplir la matrice signal, tu le passera sous forme de référence non constantes sous une forme proche de
Code:

1 2 3 4 5 6 7 8 9 10 void Read1FileAA(const string& AAdir, const string& AAfile, vector<vector<vector<float> > >,int ant); void ReadAllFilesAA(const string& AAdir,const vector<string>& AAfile, vector<vector<vector<float> > >& signals); /* à moins que tu ne préfère gérer les trois dimensions en une: */ void Read1FileAA(const string& AAdir, const string& AAfile, vector<float>,int ant); void ReadAllFilesAA(const string& AAdir,const vector<string>& AAfile, vector<float >& signals);

23/08/2007, 22h13
keldorn

Cool, je vais avoir plein de trucs à regarder.

Pas tout de suite, j'ai autre chose àà faire, mais je m'y recolle des lundi!

Merci,
Keld.