Re: request for code review
From: Robert Stankowic (pcdoktor_at_netway.at)
Date: 11/18/03
- Next message: benakiva_at_wicc.weizmann.ac.il: "Re: gcc compile problem"
- Previous message: goose: "Re: @@@ A Serious Question @@@"
- In reply to: Debashish Chakravarty: "request for code review"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Tue, 18 Nov 2003 08:54:04 +0100
"Debashish Chakravarty" <desi_coder@yahoo.com> schrieb im Newsbeitrag
news:ad1286d0.0311172217.2b2281d1@posting.google.com...
> Hi,
>
> I have a file with a single column containing observations of an
> experiment, the number of observations is not known in advance and I
> have to read them all into an array, I am using realloc to keep on
> changing the size of the array. My program is below. I would be
> grateful for any comments.
>
> Deb
>
>
>
/***************************************************************************
***/
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
>
> #define BFSIZ 1024
>
> /*fname is the name of the file,on return nr contains the number of
> observations, the function returns a pointer to the array containing
> the observations if sucessful and NULL otherwise*/
>
> double
> *read_dbl_array(char *fname,int *nr)
> {
> size_t nd=32,/* space for doubles is allocated in multiples of nd
> */
> nread=0,/*number of lines read so far*/
> ninc=0;
Just personal preference - I'd write every definition in an extra line:
size_t nd = 32;
size_t nread = 0;
size_t ninc = 0;
> FILE *fp;
> double *d=NULL;
> char buf[BFSIZ];
> *nr=0;
> fp=fopen(fname,"r");
> if(NULL==fp)
I like that way of using == :)
Anyway, here i'd just write
if(!fp)
> {
> perror(NULL);
> return NULL;
> }
> while(fgets(buf,sizeof(buf),fp))
No need for parens around buf
> {
> int i;
> if(0==(nread%nd))/* time to allocate more memory*/
> {
> double *t;
> t=realloc(d,(++ninc)*sizeof(*t)*nd);
No need for parens around *t
> if(NULL==t)
> {
> fclose(fp);
> free(d);
> return NULL;
> }
> d=t;
> }
> i=sscanf(buf,"%lf",d+(nread++));
> if(i != 1)
> {
> fprintf(stderr,"invalid input at line %lu\n",(unsigned
> long) nread);
> fclose(fp);
> free(d);
> return NULL;
> }
> }
> if(ferror(fp))
> {
> perror("error reading file");
> fclose(fp);
> free(d);
> return NULL;
> }
> fclose(fp);
> *nr=nread;
> return d;
> }
>
> int
> main(void)
> {
> char fname[FILENAME_MAX+1],*s;
I am not sure about that, but as I understand the definition of FILENAME_MAX
in 7.19.1 of n869 FILENAME_MAX should be big enough to hold the longest
possible filename plus the terminating '\0'. It says:
(emphasis mine)
..which expands to an integer constant expression that is the size needed
for an array of
char large enough to hold the LONGEST FILENAME STRING that the
implementation
guarantees can be opened;
However, the +1 does not hurt..
And again I prefer an extra line for each variable. You are not using much
whitespace, and it took me a second glance to see, that the above line
defined two variables.
> double *d=NULL;
> int nr=0;
> printf("give file name\n");
> fgets(fname,sizeof(fname),stdin);
> if((s=strchr(fname,'\n')))
> *s=0;
If there is no newline found in the input it is quite likely, that the user
entered an invalid filename. Maybe it would be better to extend the if block
so, that it covers the whole processing and prompt the user for a correct
filename if no '\n' is present.
(untested, typos etc likely)
int
main(void)
{
char fname[FILENAME_MAX+1];
char *s;
double *d=NULL;
int nr=0;
int retry_count = 0;
printf("give file name\n");
do
{
fgets(fname,sizeof(fname),stdin);
s=strchr(fname,'\n');
if(s)
{
*s=0;
d=read_dbl_array(fname,&nr);
if(d)
{
printf("%d lines read\n"
"First value:%f\t"
"Last value:%f",
nr,d[0],d[nr-1]);
free(d);
return EXIT_SUCCESS;
}
else
{
return EXIT_FAILURE;
}
}
else
{
puts("Probably incorrect filename. "
"Please try again");
do
{
fgets(fname,sizeof(fname),stdin);
}while(!strchr(fname,'\n'));
}
}while(retry_count++ < 5 && !s);
puts("RTFM!!");
return EXIT_FAILURE;
}
> d=read_dbl_array(fname,&nr);
> if(d)
> {
> printf("%d lines read\nFirst value:%f\tLast
> value:%f",nr,d[0],d[nr-1]);
> }
> free(d);
> return 0;
> }
I liked your code :)
Robert
- Next message: benakiva_at_wicc.weizmann.ac.il: "Re: gcc compile problem"
- Previous message: goose: "Re: @@@ A Serious Question @@@"
- In reply to: Debashish Chakravarty: "request for code review"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
