request for code review

Discussion in 'C Programming' started by Debashish Chakravarty, Nov 18, 2003.

  1. 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;
    }
     
    Debashish Chakravarty, Nov 18, 2003
    #1
    1. Advertising

  2. 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)
    Debashish Chakravarty wrote:
    > 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, Nov 18, 2003
    #2
    1. Advertising

  3. "Debashish Chakravarty" <> schrieb im Newsbeitrag
    news:...
    > 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, Nov 18, 2003
    #3
  4. "Debashish Chakravarty" <> schrieb im Newsbeitrag
    news:...
    > 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, Nov 18, 2003
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Code Review Request

    , Jun 30, 2005, in forum: Java
    Replies:
    2
    Views:
    560
    Andrew Thompson
    Jun 30, 2005
  2. P Kenter

    Request for code review

    P Kenter, May 28, 2004, in forum: C++
    Replies:
    4
    Views:
    375
    P Kenter
    Jun 2, 2004
  3. Ben Hanson

    Request for Code Review

    Ben Hanson, Jul 2, 2004, in forum: C++
    Replies:
    19
    Views:
    687
    Chris Gordon-Smith
    Jul 4, 2004
  4. Adam Monsen

    code review request: basic file I/O

    Adam Monsen, Aug 16, 2004, in forum: C Programming
    Replies:
    9
    Views:
    351
    Arthur J. O'Dwyer
    Aug 19, 2004
  5. www
    Replies:
    51
    Views:
    1,515
Loading...

Share This Page