Propreté ???

In 'fr.comp.lang.c', Fauberteau Frédéric <triaxx@corporation.org> wrote:

Puis-je demander sur ce newsgroup si un code est "propre" ? J'ai exposé
mon problème plus haut : 'gestion de fichier [Débutant]'.

Le revue de code C standard fait partie des activités habituelles de ce
forum.

Voilà, j'ai modifié et ça permet d'écrire un tirage de 15 numéros dans
un fichier :

#include <stdio.h>
#include <stdlib.h>

void Saisie (FILE *fichier);

Pourquoi un prototype séparé? Il suffisait de suivre le vieil adage "définir
avant d'utiliser".

int main ()
{
FILE *fichier = NULL;

Saisie (fichier);

Je ne vois pas pourquoi tu passes NULL à la fonction. Ca sert à quoi? A
lui dire de ne rien faire? Puisque 'fichier' n'est pas utilisé après la
fonction, c'est sans doute qu'il est inutile ici.

return EXIT_SUCCESS;
}

void Saisie (FILE *fichier)
{
char *tmp = malloc (2);

Pourquoi pas

char tmp[2];

tout simplement?

Ca évitera des oublis comme le test de 'tmp' après allocation. Si il est
NULL, il ne faut pas continuer.

int i;

if((fichier = fopen ("tirages.dat", "a")) == NULL)

Modifier un paramètre est souvent le signe d'une erreur de conception. En
effet, 'fichier' n'a pas à être un paramètre, mais devrait être une variable
locale.

Eviter affectation ('=') et test d'expression ('==') dans la même ligne. Ca
n'apporte rien d'autre que de la confusion.

printf ("ERREUR : impossible d'ouvrir le fichier.n");

Par contre, il est recommandé de toujours utiliser les {}.

else
{
for(i = 1; i <= 15; i++)

La version 'idiomatique' est plutôt:

for(i = 0; i < 15; i++)

car les tableaux C sont basés à 0, et que i est souvent l'indice d'un
tableau.

{
scanf ("%2s", tmp);

fputc (tmp[0], fichier);

if(tmp[1] != 0)

C'est pas possible. %2s" ne permet de renter qu'un caractère. Dans ce cas, le
2ème est forcément à 0.

scanf() est difficile à bien utiliser. Ce n'est pas une fonction de débutant.
Lire la FAQ pour des alternatives.

fputc (tmp[1], fichier);

if(i < 15)

fputc ('-', fichier);

if(i == 15)

fputc ('n', fichier);
}

free (tmp);

fclose (fichier);
}
}

Y-a-t'il assez de contrôle ?

La question est plutôt. "Ce code est-il à l'épreuve des balles ?"

PS : Triaxx et Fauberteau Frédéric sont les mêmes personnes sous un OS
différent.

On avait compris!

--
-ed- emdelYOURBRA@noos.fr [remove YOURBRA before answering me]
The C-language FAQ: http://www.eskimo.com/~scs/C-faq/top.html
<blank line>
FAQ de f.c.l.c : http://www.isty-info.uvsq.fr/~rumeau/fclc/

#include <stdio.h>
#include <stdlib.h>

void Saisie (FILE *fichier);

Pourquoi un prototype séparé? Il suffisait de suivre le vieil adage "définir
avant d'utiliser".

Une habitude que j'avais prise car je n'ai pas appris à utiliser
plusieurs fichiers sources. Mais je vais faire comme vous dites.

int main ()
{
FILE *fichier = NULL;

Saisie (fichier);

Je ne vois pas pourquoi tu passes NULL à la fonction. Ca sert à quoi? A
lui dire de ne rien faire? Puisque 'fichier' n'est pas utilisé après la
fonction, c'est sans doute qu'il est inutile ici.
Sisi, je vais m'en sert, ce n'est qu'une ébauche de programme.

return EXIT_SUCCESS;
}

void Saisie (FILE *fichier)
{
char *tmp = malloc (2);

Pourquoi pas

char tmp[2];

tout simplement?
OK, je prends note ...

Ca évitera des oublis comme le test de 'tmp' après allocation. Si il est
NULL, il ne faut pas continuer.

int i;

if((fichier = fopen ("tirages.dat", "a")) == NULL)

Modifier un paramètre est souvent le signe d'une erreur de conception. En
effet, 'fichier' n'a pas à être un paramètre, mais devrait être une variable
locale.
Je ne cromprends pas ce que vous essayez de me dire. 'fichier' pointe

sur l'addresse mémoire de la structure qui permet de gérer ledit
fichier, non ? Je m'en sers donc avec fopen ...
Vous êtes sûrement bien callé en la matière, j'écoute vos conseils.

Eviter affectation ('=') et test d'expression ('==') dans la même ligne. Ca
n'apporte rien d'autre que de la confusion.
Alors, je fais comment ?

printf ("ERREUR : impossible d'ouvrir le fichier.n");

Par contre, il est recommandé de toujours utiliser les {}.
OK

else
{
for(i = 1; i <= 15; i++)

La version 'idiomatique' est plutôt:
C'est vrai, et pourtant, on me l'a tellement dit ...

for(i = 0; i < 15; i++)

car les tableaux C sont basés à 0, et que i est souvent l'indice d'un
tableau.

{
scanf ("%2s", tmp);

fputc (tmp[0], fichier);

if(tmp[1] != 0)

C'est pas possible. %2s" ne permet de renter qu'un caractère. Dans ce cas, le
2ème est forcément à 0.
Et pourtant, si, puisque mon programme marche ...

scanf() est difficile à bien utiliser. Ce n'est pas une fonction de débutant.
Lire la FAQ pour des alternatives.
Je vous crois, mais c'est pourtant l'une des première fonctions que l'on

nous a appris lorsque l'on a commencé le C

fputc (tmp[1], fichier);

if(i < 15)

fputc ('-', fichier);

if(i == 15)

fputc ('n', fichier);
}

free (tmp);

fclose (fichier);
}
}

Y-a-t'il assez de contrôle ?

La question est plutôt. "Ce code est-il à l'épreuve des balles ?"
Alors, je doute qu'il le soit.

PS : Triaxx et Fauberteau Frédéric sont les mêmes personnes sous un OS
différent.

On avait compris!
Ah bah tant mieux.

Merci pour cette attention, ça me touche.

Et voici le code plus avancé, pour mieux comprendre pourquoi ai-je fais ça :

#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include <time.h>

static int rand_increment = 1;

void Saisie (FILE *fichier)
{
char tmp[2];

int i;

if((fichier = fopen ("tirages.dat", "a")) == NULL)
{
printf ("ERREUR : impossible d'ouvrir le fichier.n");
}

else
{
for(i = 0; i < 15; i++)
{
scanf ("%2s", tmp);

fputc (tmp[0], fichier);

if(tmp[1] != 0)

fputc (tmp[1], fichier);

if(i < 14)

fputc ('-', fichier);

if(i == 14)

fputc ('n', fichier);
}

fclose (fichier);
}
}

void Lecture (FILE *fichier, int grille[])
{
char tmp[2],
c;

int i;

for(i = 0; i <= 55; i++)
{
grille[i] = 0;
}

if((fichier = fopen ("tirages.dat", "r")) == NULL)
{
printf ("ERREUR : impossible d'ouvrir le fichier.n");
}

else
{
i = 0;

while((c = fgetc (fichier)) != EOF)
{
if(c != '-' && c != 'n')
{
tmp[i] = c;

i++;
}

else
{
i = 0;

grille[(atoi (tmp))-1]++;

tmp[0] = 0;
tmp[1] = 0;
}
}
}

fclose (fichier);
}

int Aleatoire ()
{
int rand_num;

srand (time(0) * rand_increment);

rand_num = (int) (2.0*rand()/(RAND_MAX+1.0));

rand_increment++;

return rand_num;
}

void Affichage (int grille[])
{
int combi[10],
i,
j,
tmp,
max;

for(i = 0; i < 56; i++)
{
printf ("%d", grille[i]);
}

printf ("n");

for(i = 0; i < 10; i++)
{
combi[i] = 0;
}

for(i = 0; i < 10; i++)
{
tmp = 0;

for(j = 0; j < 56; j++)
{
if(grille[j] > tmp || (grille[j] == tmp && Aleatoire ()))
{
tmp = grille[j];

max = j+1;
}
}

combi[i] = max;

grille[max-1] = INT_MIN;
}

for(i = 0; i < 10; i++)
{
for(j = 0; j < 10; j++)
{
if(combi[j] > combi[i])
{
tmp = combi[i];

combi[i] = combi[j];

combi[j] = tmp;
}
}
}

for(i = 0; i < 10; i++)
{
printf ("%d", combi[i]);

if(i < 9)
{
printf ("-");
}

if(i == 9)
{
printf ("n");
}
}
}

void Optirage (FILE *fichier)
{
int grille[56];

Lecture (fichier, grille);

Affichage (grille);
}

int main ()
{
FILE *fichier = NULL;

char selection;

printf ("OpTirage 0.0.1 (développement)nn");

do
{
printf ("- Saisir un tirage (s)n");
printf ("- Trouver le meilleur tirage (t)n");
printf ("- Quitter (q)n");

scanf (" %c", &selection);

switch(selection)
{
case 's' : Saisie (fichier);
break;

case 't' : Optirage (fichier);
break;

case 'q' : return EXIT_SUCCESS;

default : break;
}
}
while(selection != 'q');

return EXIT_SUCCESS;
}

if((fichier = fopen ("tirages.dat", "a")) == NULL)

Modifier un paramètre est souvent le signe d'une erreur de conception. En
effet, 'fichier' n'a pas à être un paramètre, mais devrait être une
variable locale.
Je ne cromprends pas ce que vous essayez de me dire. 'fichier' pointe

sur l'addresse mémoire de la structure qui permet de gérer ledit
fichier, non ? Je m'en sers donc avec fopen ...
Vous êtes sûrement bien callé en la matière, j'écoute vos conseils.

ici 'fichier' n'a une valeur utilisée que localement, à l'intérieur de la
fonction Saisie(). La variable de même nom dans main vaut toujours NULL.
Donc passer fichier en argument de Saisie() n'a pas d'utilité immédiate.

Ce que tu peux faire, c'est :

void Saisie(void)
{
FILE *fichier; // <- variable locale

ou :

int main(void)
{
FILE *fichier = fopen("tirages.dat", "a");
Saisie(fichier);
...
fclose(fichier);
return 0;
}

void Saisie(FILE *fichier)
{
if(fichier) {
...
}
}

Dans ce cas, passer la valeur de 'fichier' a un sens, puisqu'elle est
établit en dehors de Saisie.

Eviter affectation ('=') et test d'expression ('==') dans la même ligne.
Ca n'apporte rien d'autre que de la confusion.
Alors, je fais comment ?

Décompose en deux lignes :
fichier = fopen(...);
if (fichier == NULL) {
...
}

--
Richard

In 'fr.comp.lang.c', Triaxx <triaxx@corporation.net> wrote:

int main ()
{
FILE *fichier = NULL;

Saisie (fichier);

Je ne vois pas pourquoi tu passes NULL à la fonction. Ca sert à quoi? A
lui dire de ne rien faire? Puisque 'fichier' n'est pas utilisé après la
fonction, c'est sans doute qu'il est inutile ici.
Sisi, je vais m'en sert, ce n'est qu'une ébauche de programme.

Alors il faut passer à la fonction une valeur valide, donc faire le fopen()
avant l'appel. Ne compte pas sur 'Saisie()' pour modifier la valeur de
'fichier' dans main(). Je te rappelle qu'en C, les passages de paramètre se
font uniquement par valeur. La valeur reçue dans la fonction n'est qu'une
/copie/ de l'original. L'original ne peut pas être modifié par la fonction.
C'est pour ça que je t'ai fait remarquer que modifier un paramètre était
souvent le signe d'une erreur de conception.

return EXIT_SUCCESS;
}

void Saisie (FILE *fichier)
{
int i;

if((fichier = fopen ("tirages.dat", "a")) == NULL)

Modifier un paramètre est souvent le signe d'une erreur de conception.
En effet, 'fichier' n'a pas à être un paramètre, mais devrait être une
variable locale.
Je ne cromprends pas ce que vous essayez de me dire. 'fichier' pointe

sur l'addresse mémoire de la structure qui permet de gérer ledit
fichier, non ? Je m'en sers donc avec fopen ...

Certes, mais localement uniquement. Le fait de modifier cette valeur ne
modifie pas la valeur originale de 'fichier' dans main().

Il suffisait d'écrire

void Saisie (void)
{
FILE *fichier = fopen ("tirages.dat", "a");

if (fichier == NULL)
{
...


fclose (fichier);
}

scanf() est difficile à bien utiliser. Ce n'est pas une fonction de
débutant. Lire la FAQ pour des alternatives.
Je vous crois, mais c'est pourtant l'une des première fonctions que l'on

nous a appris lorsque l'on a commencé le C

Mauvais instructeur.

<snip le code que je n'ai pas vérifié>

Ben j'ai toujours pas compris à quoi servait le paramètre 'fichier'. J'aurais
compris que ce soit le nom du fichier, mais un FILE* à NULL, franchement,
c'est inutile. J'aime pas le code inutile.

--
-ed- emdelYOURBRA@noos.fr [remove YOURBRA before answering me]
The C-language FAQ: http://www.eskimo.com/~scs/C-faq/top.html
<blank line>
FAQ de f.c.l.c : http://www.isty-info.uvsq.fr/~rumeau/fclc/

int i;

if((fichier = fopen ("tirages.dat", "a")) == NULL)

Modifier un paramètre est souvent le signe d'une erreur de
conception. En effet, 'fichier' n'a pas à être un paramètre, mais
devrait être une variable locale.
Je ne cromprends pas ce que vous essayez de me dire. 'fichier' pointe

sur l'addresse mémoire de la structure qui permet de gérer ledit
fichier, non ? Je m'en sers donc avec fopen ...
Vous êtes sûrement bien callé en la matière, j'écoute vos conseils.

A mon avis tu voulais passer un pointeur vers un FILE* et mettre à jour
ce pointeur dans ta fonction :
void Saisie( FILE **fichier );
et
*fichier = fopen(...)

Eviter affectation ('=') et test d'expression ('==') dans la même
ligne. Ca n'apporte rien d'autre que de la confusion.
Alors, je fais comment ?

Tu fais deux lignes :
fichier = fopen(...)
if( fichier == NULL )
{
...
}

scanf() est difficile à bien utiliser. Ce n'est pas une fonction de
débutant. Lire la FAQ pour des alternatives.
Je vous crois, mais c'est pourtant l'une des première fonctions que

l'on nous a appris lorsque l'on a commencé le C

 mon avis c'est parce qu'elle permet de faire rapidment des choses. Mais
avant de bien la maîtriser il s'en passe un bout de temps.

for(i = 0; i < 15; i++)
{
scanf ("%2s", tmp);

fputc (tmp[0], fichier);

if(tmp[1] != 0)

fputc (tmp[1], fichier);

if(i < 14)

fputc ('-', fichier);

if(i == 14)

fputc ('n', fichier);
}

fclose (fichier);
}
}

Là je ne vois pas trop quelle manipulation tu cherches à faire alors je
ne peux pas trop te conseiller.
Mets les accolades et utilise autre chose que scanf() (sic.)

void Lecture (FILE *fichier, int grille[])
{
char tmp[2],
c;

int i;

for(i = 0; i <= 55; i++)

i<56 est mieux

grille[(atoi (tmp))-1]++;

AAAAAAh
A mon avis il faudrait développer ça sur au moins 3 lignes. Pourquoi
atoi(tmp)-1 serait un indice valide pour ton tableau ?


--
_|_|_| CnS
_|_| for(n=0;b;n++)
_| b&=b-1; /*pp.47 K&R*/

"Triaxx" <triaxx@corporation.net> a écrit dans le message de news:
bg1cqp$6vr$1@news-reader3.wanadoo.fr...

Bonjour,

Merci pour cette attention, ça me touche.

Et voici le code plus avancé, pour mieux comprendre pourquoi ai-je fais ça
:

#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include <time.h>

static int rand_increment = 1;

void Saisie (FILE *fichier)

En fait, comme Emmanuel je ne vois pas pourquoi tu passes ce fichier. Dans
toutes les fonctions de ton programme, tu passes un FILE* qui est NULL et
que tu modifies dans ta fonction avec un fopen puis un fclose.
Pourquoi ne pa déclarer un FILE* dans ces fonctions? Tu évites un empilement
inutile lors de l'appel de fonction. Remarque ca se fera bien un jour mais
bon...
Et attention faute assez fréquence que j'ai vu souvent dans ma promo, si
jamais tu veut ouvrir ton fichier dans ta fonction saisie, et ne pas le
fermer après, alors il faut soit retourner le FILE* soit passer un FILE**.
Je m'explique, imagine tu fais ca :

void Saisie(FILE* fichier) {
fichier = fopen("blabla", "rw");
}

Si tu fais ca, alors fichier sera inchangé dans la fonction appelante et tu
auras des bugs dans ton programme.
Voila pour ce que j'avais à dire sur le FILE*.

{
[...]

for(i = 0; i < 15; i++)
{

[...]

if(i < 14)

Ces deux if sont assez lourd, les tests sont fait 14 fois pour rien...
Pourquoi ne pas sortir le 14 de ta boucle, vu que c'est aussi la fin de ton
for, il ne passe jamais par i == 15

[...]

}

void Lecture (FILE *fichier, int grille[])
{

[...]

grille[(atoi (tmp))-1]++;

préfère strtol() à atoi() qui a ma connaissance est obselete et ne fais
aucune vérification!

[...]

}

int Aleatoire ()
{
int rand_num;

srand (time(0) * rand_increment);

rand_num = (int) (2.0*rand()/(RAND_MAX+1.0));

rand_increment++;

return rand_num;
}

pas compris l'interet de ton rand_increment ? D'ailleurs je ne vois pas
pourquoi tu refais un srand() à chaque fois.
En toute logique, le srand() ne se fait qu'une fois, la toute première!
ensuite tu fais des rand() avec la commande que tu as mise, et c'est bon.

void Affichage (int grille[])
{
[...]

hum beaucoup de for(i=0; i<10; i++)
tu ne pourrais pas en rassembler quelques uns? les deux premiers au moins...

int main ()
{
FILE *fichier = NULL;

char selection;

printf ("OpTirage 0.0.1 (développement)nn");

do
{
printf ("- Saisir un tirage (s)n");
printf ("- Trouver le meilleur tirage (t)n");
printf ("- Quitter (q)n");

scanf (" %c", &selection);

Vraiment pas bonne idée ca!
Un moment dans ton programme tu rajouteras peut etre un scanf ("%d...") par
exemple et la tu verras que ton scanf("%c") n'est pas pris en compte.
d'ailleurs en toute logique, il se fait même un tour gratuit à chaque fois,
vu que scanf laisse un n dans le buffer, il est pris par le scanf("%c")
d'après.

switch(selection)
{
case 's' : Saisie (fichier);
break;

case 't' : Optirage (fichier);
break;

case 'q' : return EXIT_SUCCESS;

default : break;
}
}
while(selection != 'q');

Si on veut, faut bien mettre une condition, mais dans ce cas, ne met pas de
default ni de case 'q', tu auras le même effet.
enlever le default fera que tu tourneras au suivant direct, et enlever le
case 'q' fera que c'est le return en dessous qui servira pour quitter.

return EXIT_SUCCESS;
}

--
Anthony

Emmanuel Delahaye wrote:

In 'fr.comp.lang.c', Fauberteau Frédéric <triaxx@corporation.org> wrote:

#include <stdio.h>
#include <stdlib.h>

void Saisie (FILE *fichier);

Pourquoi un prototype séparé? Il suffisait de suivre le vieil adage "définir
avant d'utiliser".

Sauf qu'on n'est pas forcement adepte de l'organisation bottom-up, et

qu'on peut preferer avoir les fonctions "top" au debut de l'unite de
compilation, et les fonctions "bottom" plus loin, meme si c'est au prix
de la redaction d'un prototype separe.

Perso, je prefere :-)
--
Bertrand Mollinier Toublet
"Reality exists" - Richard Heathfield, 1 July 2003

OK, j'ai compris, en fait, je n'ai pas besoin de travailler avec
'fichier' dans le main. Ou alors faire comme Cyrille dit ...

C'est très dur de ne pas utiliser scanf ...
voilà, et bien merci à tous.

In 'fr.comp.lang.c', Bertrand Mollinier Toublet
<bertrand.molliniertoublet@enst-bretagne.fr> wrote:

Emmanuel Delahaye wrote:

In 'fr.comp.lang.c', Fauberteau Frédéric <triaxx@corporation.org>
wrote:

#include <stdio.h>
#include <stdlib.h>

void Saisie (FILE *fichier);

Pourquoi un prototype séparé? Il suffisait de suivre le vieil adage
"définir avant d'utiliser".

Sauf qu'on n'est pas forcement adepte de l'organisation bottom-up, et

qu'on peut preferer avoir les fonctions "top" au debut de l'unite de
compilation, et les fonctions "bottom" plus loin, meme si c'est au prix
de la redaction d'un prototype separe.

Perso, je prefere :-)

Ma préférence ("définir avant d'utiliser") permet, avec les fonctions privées
('static') de se passer de prototype séparé sanf dans les cas où il y aurait
des appels récursifs (désirés ou non). Connaissant les risques liés à la
récursion, je préfère que les cas de récursion soient visibles. Si on met
systématiquement des prototypes séparés, n'importe qui peut appeler n'importe
qui, et c'est le bazar.

D'ailleurs en y réflechissant bien il serait tout à fait logique, afin d'en
réduire la portée, de placer le prototype séparé d'une fonction appelée
récursivement dans le corps de la fonction appelante. (Je crois me souvenir
que c'est autorisé).

static void a(void)
{
/* declaration 'forward' a cause de l'appel recursif */
static void b(void);

b();
}

static void b(void)
{
a();
}

Une belle boucle en tout cas! Cassage de mémoire auto garanti!

--
-ed- emdelYOURBRA@noos.fr [remove YOURBRA before answering me]
The C-language FAQ: http://www.eskimo.com/~scs/C-faq/top.html
<blank line>
FAQ de f.c.l.c : http://www.isty-info.uvsq.fr/~rumeau/fclc/

In 'fr.comp.lang.c', Fauberteau Frédéric <triaxx@corporation.org> wrote:

C'est très dur de ne pas utiliser scanf ...

Ah?

fgets()
fgets() + strto[u]l()
fgets() + strtod()

Rien de dur. Entraine toi, tu verras. En plus, de nombreux exemples ont été
postés (chercher 'get_s')

--
-ed- emdelYOURBRA@noos.fr [remove YOURBRA before answering me]
The C-language FAQ: http://www.eskimo.com/~scs/C-faq/top.html
<blank line>
FAQ de f.c.l.c : http://www.isty-info.uvsq.fr/~rumeau/fclc/