request for code review

[Debashish Chakravarty]

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;
FILE *fp;
double *d=NULL;
char buf[BFSIZ];
*nr=0;
fp=fopen(fname,"r");
if(NULL==fp)
{
perror(NULL);
return NULL;
}
while(fgets(buf,sizeof(buf),fp))
{
int i;
if(0==(nread%nd))/* time to allocate more memory*/
{
double *t;
t=realloc(d,(++ninc)*sizeof(*t)*nd);
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;
double *d=NULL;
int nr=0;
printf("give file name\n");
fgets(fname,sizeof(fname),stdin);
if((s=strchr(fname,'\n')))
*s=0;
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;
}

 

[Pushkar Pradhan]

I've a doubt, doesn't calling realloc for every entry slow down your
code, here's what I did when I didn't know num. of entries in file in
advance.
Count num. of lines in file by calling the "system" command "wc -l
filename > numlines.txt"

numlines.txt contains the num. of lines in the file. Read this number.

You now call calloc only once.

system spawns a shell and executes the cmd. (sort of system specific)

 

[Robert Stankowic]

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

 

[Robert Stankowic]

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