Critique

Discussion in 'C Programming' started by Andrew Clark, Apr 11, 2005.

  1. Andrew Clark

    Andrew Clark Guest

    Hello all,

    Wow, has it ever been a long time since I posted anything to this
    group. I don't keep as current as I used to anymore; I only lurk from
    time to time when I have a spare moment.
    I am trying to write a program to open a file and process it
    according to command line parameters. All I need are two simple search
    and replace operations. I invoke the program with an int representing a
    site code and a long representing a date and my program should replace
    some hard-coded text strings in the file with the int and the long I pass
    in. I may have some insight as to why this is not working now, but I'd
    like some constructive criticism here. I tried to conform to the standard
    (as I know it), but, well, let's see how it goes... Thanks for reading!

    Andrew


    /* SiteUpdate.c */
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>

    #define BUFSIZE 256
    #define SITEMIN 0
    #define SITEMAX 599
    #define STARTDATE 20050401
    #define ENDDATE 29991231
    #define IN "SiteUpdate.sql"

    int fgetline ( FILE *, char * );

    int main ( int argc, char *argv[] )
    {
    int rc = EXIT_FAILURE;
    int site_code = SITEMIN - 1;
    long date = STARTDATE - 1;

    if ( 3 == argc )
    {
    site_code = atoi ( argv[1] );
    if ( site_code < SITEMIN || SITEMAX < site_code )
    {
    fprintf ( stderr, "Site code must be a number between %d and
    %d.\n", SITEMIN, SITEMAX );
    }
    else
    {
    date = atol ( argv[2] );
    if ( date < STARTDATE || ENDDATE < date )
    {
    fprintf ( stderr, "%s\n", "Date must be in the format
    \"YYYYMMDD\" and between" );
    fprintf ( stderr, "%s\n", "today and December 31,
    2999." );
    }
    else
    {
    char *outfile, code[4];
    FILE *in, *out;

    sprintf ( code, "%d", site_code + 400 );

    /* subtract 1 for "site" -> "xxx" and */
    /* add one for NUL character */
    outfile = malloc ( strlen ( IN ) - 1 + 1 );

    if ( outfile )
    {
    sprintf ( outfile, "%sUpdate.sql", code );
    in = fopen ( IN, "r" );
    out = fopen ( outfile, "w+" );
    if ( in && out )
    {
    char buf[BUFSIZE], *p;
    char *temp;

    while ( EOF != fgetline ( in, buf ) )
    {
    if ( buf == strstr ( buf, "--" ) )
    {
    /* If the line is a comment, do not */
    /* include it in the final script */
    continue;
    }
    else if ( ( p = strstr ( buf, "xxx" ) ) )
    {
    temp = malloc ( strlen ( buf ) + 1 );
    if ( temp )
    {
    /* Replace "xxx" with site code */
    strcpy ( temp, buf );
    sprintf ( p, "%s", code );
    strcat ( buf, strstr ( temp, "xxx" )
    + strlen ( "xxx" ) );
    fprintf ( out, "%s\n", buf );

    free ( temp );
    temp = NULL;
    }
    else
    {
    fprintf ( stderr, "%s\n", "Not enough
    memory." );
    }
    }
    else if ( ( p = strstr ( buf, "yyyyyyyy" ) )
    )
    {
    temp = malloc ( strlen ( buf ) + 1 );
    if ( temp )
    {
    /* Replace "yyyyyyyy" with date */
    strcpy ( temp, buf );
    sprintf ( p, "%ld", date );
    strcat ( buf, strstr ( temp,
    "\\gen" ) );
    fprintf ( out, "%s\n", buf );

    free ( temp );
    temp = NULL;
    }
    else
    {
    fprintf ( stderr, "%s\n", "Not enough
    memory." );
    }
    }
    else
    {
    /* Nothing to do, just copy the line */
    fprintf ( out, "%s\n", buf );
    }
    }
    fclose ( in );
    fclose ( out );
    rc = EXIT_SUCCESS;
    }
    else
    {
    fprintf ( stderr, "Could not open %s!\n", ( in ?
    outfile : IN ) );
    }

    free ( outfile );
    }
    }
    }
    }
    else
    {
    fprintf ( stderr, "\n" );
    fprintf ( stderr, "Usage: %s <site_code> <date>\n", argv[0] );
    fprintf ( stderr, "%s\n", " site_code two digit number
    representing the site" );
    fprintf ( stderr, "%s\n", " date eight digit date
    in the format \"YYYYMMDD\"" );
    fprintf ( stderr, "%c\n", '\n' );
    fprintf ( stderr, "%s\n", "Generates a SQL script for the month.
    Input file is .\\SiteUpdate.sql" );
    }

    return rc;
    }

    int fgetline ( FILE *fp, char *buf )
    {
    char *p;
    int c, i = 0;

    p = buf;
    while ( EOF != ( c = fgetc ( fp ) ) && '\n' != c && i < BUFSIZE )
    {
    *p = c;
    ++p;
    ++i;
    }
    *p = 0;
    if ( ( p = strchr ( buf, '\n' ) ) ) *p = 0;

    return c;
    }

    /* end SiteUpdate.c */
     
    Andrew Clark, Apr 11, 2005
    #1
    1. Advertising

  2. Andrew Clark

    CBFalconer Guest

    Andrew Clark wrote:
    >
    > Wow, has it ever been a long time since I posted anything to this
    > group. I don't keep as current as I used to anymore; I only lurk
    > from time to time when I have a spare moment.
    >
    > I am trying to write a program to open a file and process it
    > according to command line parameters. All I need are two simple
    > search and replace operations. I invoke the program with an int
    > representing a site code and a long representing a date and my
    > program should replace some hard-coded text strings in the file
    > with the int and the long I pass in. I may have some insight as
    > to why this is not working now, but I'd like some constructive
    > criticism here. I tried to conform to the standard (as I know
    > it), but, well, let's see how it goes... Thanks for reading!


    Fundamentally you try to cram too much into one routine, main.
    Things will be much clearer if you break operations up. Your
    getline is a step in the right direction.

    However there is available code to do almost exactly what you
    want. Take a look on my site for ggets and id2id-20. id2id will
    do your job if you write a two line configuration file to supply
    the replacements for xxx and yyyyyyyy, but this may not be quite
    suitable. At any rate the portable source is there, and you can
    easily change it to work from command line parameters. See:

    <http://cbfalconer.home.att.net/download/>

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
     
    CBFalconer, Apr 11, 2005
    #2
    1. Advertising

  3. Andrew Clark wrote on 11/04/05 :
    Some comments (-ed-). Feel free to ask for details :

    string instrumentation : defined in 'sys.h' at
    http://mapage.noos.fr/emdel/clib/ed/inc/sys.h


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

    #define BUFSIZE 256
    #define SITEMIN 0
    #define SITEMAX 599
    #define STARTDATE 20050401L
    #define ENDDATE 29991231L

    /* -ed- some system are limited to the 8.3 file name format */
    #define IN_BEG "SITE"
    #define IN_END "UP~1.SQL"
    #define IN_ "../data/" IN_BEG IN_END

    int fgetline (FILE *, char *);
    /* -ed- avoid separated prototypes with a better layout... */

    int main (int argc, char *argv[])
    {
    int rc = EXIT_FAILURE;
    int site_code = SITEMIN - 1;
    long date = STARTDATE - 1;

    if (3 == argc)
    {
    /* -ed-

    site_code = atoi (argv[1]);

    * atoi() is deprecated. Use strtol()
    * here is a naive way.
    * Should be improved for character set control,
    * end pointer checking, range value etc.
    */
    site_code = (int) strtol (argv[1], NULL, 10);

    if (site_code < SITEMIN || SITEMAX < site_code)
    {
    fprintf (stderr, "Site code must be a number between %d and
    %d.\n", SITEMIN, SITEMAX);
    }
    else
    {
    date = strtol (argv[2], NULL, 10);
    if (date < STARTDATE || ENDDATE < date)
    {
    fprintf (stderr, "%s\n", "Date must be in the format
    \"YYYYMMDD\" and between");
    fprintf (stderr, "%s\n", "today and December 31, 2999.");
    /* -ed-
    * The 'today' thing is not tested... STARTDATE should be a
    calculated variable ...
    */
    }
    else
    {
    char *outfile, code[4];
    FILE *in, *out;

    LIM_STR (code);
    sprintf (code, "%d", site_code + 400);
    CHK_STR (code);
    /* -ed- 599 + 400 = 999. OK. */

    /* subtract 1 for "site" -> "xxx" and */
    /* add one for NUL character */
    outfile = malloc (strlen (IN_) - 1 + 1);
    /* -ed-
    * the requested size should eventually
    * be stored somewhere for further controls...
    *
    * That said, unless you intent to realloc the array,
    malloc()
    * is not necessary here because the size is a compile-time
    * constant if expressed by 'sizeof IN - 1', IN being a
    string
    * literal.
    *
    * Of course, if it becomes a variable (array of string),
    it's
    * a different story.
    *
    * An array would suffice.
    */

    if (outfile)
    /* -ed- if (outfile != NULL) is clearer... */
    {
    sprintf (outfile, "%s" IN_END, code);
    in = fopen (IN_, "r");
    /* -ed- no need for "w+". Use "w" or "a". */
    out = fopen (outfile, "w");
    if (in && out)
    {
    char buf[BUFSIZE], *p;
    char *temp;

    LIM_STR (buf);

    while (EOF != fgetline (in, buf))
    /* -ed- BAD! You have reinvented the gets() bug.
    * You have missed the opportunity to build a
    solid
    * read line function with flexible bound
    checking.

    while (EOF != fgetline (in, buf, sizeof buf))

    */

    {
    if (buf == strstr (buf, "--"))
    {
    /* If the line is a comment, do not */
    /* include it in the final script */
    continue;
    }
    else if ((p = strstr (buf, "xxx")) != NULL)
    {
    /* -ed-
    * why is the definiton of 'temp' not here ?
    * Scope reduction makes the code more
    readable,
    * more safe, and prepare it to modularization.
    */

    size_t size = strlen (buf) + 1;
    temp = malloc (size);
    if (temp)
    {
    LIM_PTR (temp, size);
    /* Replace "xxx" with site code */
    strcpy (temp, buf);
    CHK_PTR (temp, size);

    /* -ed-
    * you should consider to build some
    str_dup()
    * function for these dynamic string copies.
    */

    sprintf (p, "%s", code);
    /* -ed- Very suspicious lack of bound
    cheking... */
    CHK_STR (buf);

    strcat (buf, strstr (temp, "xxx") + strlen
    ("xxx"));
    /* -ed- ditto... */
    CHK_STR (buf);

    fprintf (out, "%s\n", buf);

    free (temp);
    temp = NULL;
    }
    else
    {
    fprintf (stderr, "%s\n", "Not enough
    memory.");
    }
    }
    else if ((p = strstr (buf, "yyyyyyyy")) != NULL)
    {
    size_t size = strlen (buf) + 1;
    temp = malloc (size);
    if (temp)
    {
    LIM_PTR (temp, size);

    /* Replace "yyyyyyyy" with date */
    strcpy (temp, buf);
    CHK_PTR (temp, size);

    sprintf (p, "%ld", date);
    CHK_STR (buf);

    strcat (buf, strstr (temp, "\\gen"));
    CHK_STR (buf);

    fprintf (out, "%s\n", buf);

    free (temp);
    temp = NULL;
    }
    else
    {
    fprintf (stderr, "%s\n", "Not enough
    memory.");
    }
    }
    else
    {
    /* Nothing to do, just copy the line */
    fprintf (out, "%s\n", buf);
    }
    }
    fclose (in);
    fclose (out);
    rc = EXIT_SUCCESS;
    }
    else
    {
    fprintf (stderr, "Could not open %s!\n"
    ,(in ? outfile : IN_));
    }

    free (outfile);
    }
    }
    }
    }
    else
    {
    fprintf (stderr, "\n");
    fprintf (stderr, "Usage: %s <site_code> <date>\n", argv[0]);
    fprintf (stderr, "%s\n", " site_code two digit number
    representing the site");
    fprintf (stderr, "%s\n", " date eight digit date
    in the format \"YYYYMMDD\"");
    fprintf (stderr, "%c\n", '\n');
    fprintf (stderr, "%s\n", "Generates a SQL script for the month.
    Input file is .\\SiteUpdate.sql");
    }

    return rc;
    }

    int fgetline (FILE * fp, char *buf)
    {
    char *p;
    int c, i = 0;

    p = buf;
    while (EOF != (c = fgetc (fp)) && '\n' != c && i < BUFSIZE)
    {
    *p = c;
    ++p;
    ++i;
    }
    *p = 0;
    if ((p = strchr (buf, '\n')) != NULL)
    {
    *p = 0;
    }
    /* -ed- What are you doing with the pending characters ? */
    return c;
    }

    --
    Emmanuel
    The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
    The C-library: http://www.dinkumware.com/refxc.html

    "There are 10 types of people in the world today;
    those that understand binary, and those that dont."
     
    Emmanuel Delahaye, Apr 11, 2005
    #3
  4. Emmanuel Delahaye wrote:

    > int fgetline (FILE *, char *);
    > /* -ed- avoid separated prototypes with a better layout... */


    Does anyone agree with this notion? Having separate prototypes at the top of
    a file relieves me of the burden of having to arrange my function
    implementations in a specific order; more specifically, it allows me to
    always have main() as the first function. For that, I am willing to pay a
    price in the form of a small, innocuous redundancy.


    Christian
     
    Christian Kandeler, Apr 12, 2005
    #4
  5. Andrew Clark

    CBFalconer Guest

    Christian Kandeler wrote:
    > Emmanuel Delahaye wrote:
    >
    >> int fgetline (FILE *, char *);
    >> /* -ed- avoid separated prototypes with a better layout... */

    >
    > Does anyone agree with this notion? Having separate prototypes at
    > the top of a file relieves me of the burden of having to arrange my
    > function implementations in a specific order; more specifically, it
    > allows me to always have main() as the first function. For that, I
    > am willing to pay a price in the form of a small, innocuous
    > redundancy.


    Definitely. That redundance can be the source of silly hard to
    find errors. By paying strict attention to "declare and define
    before use" I always know which way to look for the routine
    source. The only (and rare) exception is when designing mutually
    recursive functions.

    Since we are no longer using line oriented editors there is no
    penalty to be paid for writing main first, and then moving above it
    to create the various subroutines. By creating them as stubs we
    can check a good deal of our logic very early.

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
     
    CBFalconer, Apr 12, 2005
    #5
  6. On Tue, 12 Apr 2005 09:08:24 +0200, Christian Kandeler wrote:

    > Emmanuel Delahaye wrote:
    >
    >> int fgetline (FILE *, char *);
    >> /* -ed- avoid separated prototypes with a better layout... */

    >
    > Does anyone agree with this notion? Having separate prototypes at the top of
    > a file relieves me of the burden of having to arrange my function
    > implementations in a specific order; more specifically, it allows me to
    > always have main() as the first function. For that, I am willing to pay a
    > price in the form of a small, innocuous redundancy.


    It is a matter of personal style and taste. Personally I agree with you.
    Having higher level functions first is a natural layout for a top-down
    divide and conquer approach to programming. It is entirely reasonable that
    the first function you come to when you look at a program is where the
    program starts. That's also true for "modules" where you encounter the
    (external linkage) functions that defvine the interface first and any
    helper functions (internal linkage) they need come after.

    Some people don't like separate declarations but

    1. you need them anyway (in a header) for functions with external linkage
    so the issue here applies only to internal linkage functions.

    2. The declarations provided a VALIDATED summary of the functions in the
    source file which can be amazingly useful. The validation also means
    that there is very little chance of a functionality impacting error in
    the declarations going undiagnosed.

    In short, in my view top-down is the natural way to organise a program and
    doing things the natural way can only help readability. The perceived
    downside of this appraoch i.e the extra lines you have to type and the
    possibility of inconsistency turn out to be rather insignificant when you
    actually look into it, but the readability advantages, especially the
    valiated summary are far from insignificant. When you are looking at a
    source file for the fist time having such a summary is a great help. I
    would argue that you should provide a function declaration list even if
    your preferred style is "backwards".

    The bottom line is however that you should do what is most natural to you,
    unless you are working to a project style in which case it is more
    important to be consistent.

    Lawrence
     
    Lawrence Kirby, Apr 12, 2005
    #6
  7. Andrew Clark wrote:
    > I am trying to write a program to open a file and process it
    > according to command line parameters. All I need are two simple
    > search and replace operations. I invoke the program with an int
    > representing a site code and a long representing a date and my
    > program should replace some hard-coded text strings in the file
    > with the int and the long I pass in. ...


    [I've halved the spacing.]

    > /* SiteUpdate.c */
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <string.h>
    >
    > #define BUFSIZE 256
    > #define SITEMIN 0
    > #define SITEMAX 599
    > #define STARTDATE 20050401
    > #define ENDDATE 29991231


    This potentially clashes with reserved EXXXX identifiers from
    <errno.h>

    > #define IN "SiteUpdate.sql"


    #define FIND1 "xxx"
    #define FIND2 "yyyyyyyy"

    #define FIND1_LEN ((sizeof FIND1) - 1)
    #define FIND2_LEN ((sizeof FIND2) - 1)

    > int fgetline ( FILE *, char * );


    What does this routine buy you over fgets()?

    >
    > int main ( int argc, char *argv[] )


    Why not make this a basic stdin/stdout filter and avoid hardcoding
    the input and output files? [The input file at least.]

    > {
    > int rc = EXIT_FAILURE;
    > int site_code = SITEMIN - 1;
    > long date = STARTDATE - 1;
    >
    > if ( 3 == argc )
    > {
    > site_code = atoi ( argv[1] );


    Use the more robust strtoi(argv[1],0,10) in place of atoi.

    > if ( site_code < SITEMIN || SITEMAX < site_code )
    > {
    > fprintf ( stderr, "Site code must be a number between %d and
    > %d.\n", SITEMIN, SITEMAX );
    > }
    > else
    > {
    > date = atol ( argv[2] );
    > if ( date < STARTDATE || ENDDATE < date )
    > {
    > fprintf ( stderr, "%s\n", "Date must be in the format
    > \"YYYYMMDD\" and between" );
    > fprintf ( stderr, "%s\n", "today and December 31,
    > 2999." );
    > }
    > else
    > {
    > char *outfile, code[4];


    To add robustness you might do something like...

    #define STR(x) #x
    #define STRSTR(x) STR(x)

    char code[sizeof STRSTR(SITEMAX)];

    > FILE *in, *out;
    >
    > sprintf ( code, "%d", site_code + 400 );
    >
    > /* subtract 1 for "site" -> "xxx" and */
    > /* add one for NUL character */
    > outfile = malloc ( strlen ( IN ) - 1 + 1 );
    >
    > if ( outfile )
    > {
    > sprintf ( outfile, "%sUpdate.sql", code );


    Why is Update.sql a magic literal?

    > in = fopen ( IN, "r" );
    > out = fopen ( outfile, "w+" );


    Why the "+"?

    > if ( in && out )
    > {
    > char buf[BUFSIZE], *p;
    > char *temp;
    >
    > while ( EOF != fgetline ( in, buf ) )
    > {
    > if ( buf == strstr ( buf, "--" ) )
    > {
    > /* If the line is a comment, do not */
    > /* include it in the final script */
    > continue;
    > }
    > else if ( ( p = strstr ( buf, "xxx" ) ) )
    > {
    > temp = malloc ( strlen ( buf ) + 1 );
    > if ( temp )
    > {
    > /* Replace "xxx" with site code */
    > strcpy ( temp, buf );
    > sprintf ( p, "%s", code );
    > strcat ( buf, strstr ( temp, "xxx" )
    > + strlen ( "xxx" ) );
    > fprintf ( out, "%s\n", buf );
    >
    > free ( temp );
    > temp = NULL;
    > }


    else if ( ( p = strstr ( buf, FIND1 ) ) )
    {
    fprintf(out, ".*s%s%s\n", (int) (p - buf), buf,
    code,
    p + FIND1_LEN );
    }

    > <snip>
    > else
    > {
    > /* Nothing to do, just copy the line */
    > fprintf ( out, "%s\n", buf );


    Why use a utility routine that strips the newline, only to
    put it back manually each time?

    > <snip>


    You handle comments inconsistently in that a '-- xxx' in the
    middle of a line may invoke substitution, but an -- comment
    at the very start of a line won't. Also, you ignore /* */
    style comments.

    I think you can simplify the whole task by scanning character
    by character with a simple state machine (ignoring sql comments
    as you go). All you need to do is check is whether an 'x' is
    followed by two more 'x's, and whether a 'y' is followed by 7
    more 'y's. If an 'x' isn't followed by two more, then just
    output the number of consecutive 'x's read to that point.

    Going off topic, I'm sure you can find existing tools to do
    such simple replacements.

    Personally, I think even those tools are probably overkill
    for the deeper problem at hand.

    Since you already have a template file, let's suppose that file
    can be abstracted down to...

    select 'xxx', 'yyyyyyyy' from dual;

    Why not just make your template (Update.sql) file more like...

    select '&1', '&2' form dual;

    Then your program output is just...

    @Update <site> <date>

    --
    Peter
     
    Peter Nilsson, Apr 12, 2005
    #7
  8. CBFalconer wrote:
    > Christian Kandeler wrote:
    > > Emmanuel Delahaye wrote:
    > >
    > >> int fgetline (FILE *, char *);
    > >> /* -ed- avoid separated prototypes with a better layout... */

    > >
    > > Does anyone agree with this notion? Having separate prototypes at
    > > the top of a file relieves me of the burden of having to arrange my
    > > function implementations in a specific order; more specifically, it
    > > allows me to always have main() as the first function. For that, I
    > > am willing to pay a price in the form of a small, innocuous
    > > redundancy.

    >
    > Definitely. That redundance can be the source of silly hard to
    > find errors. By paying strict attention to "declare and define
    > before use" I always know which way to look for the routine
    > source. The only (and rare) exception is when designing mutually
    > recursive functions.
    >
    > Since we are no longer using line oriented editors there is no
    > penalty to be paid for writing main first, and then moving above it
    > to create the various subroutines. By creating them as stubs we
    > can check a good deal of our logic very early.
    >



    What errors are you referring to? I use the "prototypes, main,
    subroutines"
    layout, and I have not had any problems with it. True, if I change
    the calling sequence of a subroutine, I have to update the prototype,
    but if I do not, I get a compilation error.

    - William Hughes
     
    William Hughes, Apr 12, 2005
    #8
  9. On Tue, 12 Apr 2005, Lawrence Kirby wrote:
    >
    > On Tue, 12 Apr 2005 09:08:24 +0200, Christian Kandeler wrote:
    >> Emmanuel Delahaye wrote:
    >>> /* -ed- avoid separated prototypes with a better layout... */

    >>
    >> Does anyone agree with this notion? Having separate prototypes at the
    >> top of a file relieves me of the burden of having to arrange my function
    >> implementations in a specific order; more specifically, it allows me to
    >> always have main() as the first function. For that, I am willing to pay a
    >> price in the form of a small, innocuous redundancy.

    >
    > It is a matter of personal style and taste. Personally I agree with you.

    [...]
    > 2. The declarations provided a VALIDATED summary of the functions in the
    > source file which can be amazingly useful. The validation also means
    > that there is very little chance of a functionality impacting error in
    > the declarations going undiagnosed.


    True. Also, I have within the past year started writing my top-of-file
    prototypes with indentation corresponding to the caller-callee
    relationships between them:

    int process(FILE *in, FILE *out);
    Triangle *get_triangle(FILE *in);
    Triangle *rotate_triangle(Triangle *tri);
    Point *rotate_point(Point *pt);
    int put_triangle(Triangle *tri, FILE *out);
    int do_error(const char *, ...);
    int do_help(int man);

    This order may or may not correspond to the order of the actual function
    definitions, but that's why editors have the "Find" command. I think it
    is useful to have this kind of documentary stuff at the top of each file.
    (Well, /near/ the top... above this I have directives, a block comment,
    probably some struct and enum definitions... but basically at the top. ;)

    > The bottom line is however that you should do what is most natural to you,
    > unless you are working to a project style in which case it is more
    > important to be consistent.


    -Arthur
     
    Arthur J. O'Dwyer, Apr 12, 2005
    #9
  10. Andrew Clark

    CBFalconer Guest

    William Hughes wrote:
    > CBFalconer wrote:
    >> Christian Kandeler wrote:
    >>> Emmanuel Delahaye wrote:
    >>>
    >>>> int fgetline (FILE *, char *);
    >>>> /* -ed- avoid separated prototypes with a better layout... */
    >>>
    >>> Does anyone agree with this notion? Having separate prototypes
    >>> at the top of a file relieves me of the burden of having to
    >>> arrange my function implementations in a specific order; more
    >>> specifically, it allows me to always have main() as the first
    >>> function. For that, I am willing to pay a price in the form of
    >>> a small, innocuous redundancy.

    >>
    >> Definitely. That redundance can be the source of silly hard to
    >> find errors. By paying strict attention to "declare and define
    >> before use" I always know which way to look for the routine
    >> source. The only (and rare) exception is when designing
    >> mutually recursive functions.
    >>
    >> Since we are no longer using line oriented editors there is no
    >> penalty to be paid for writing main first, and then moving above
    >> it to create the various subroutines. By creating them as stubs
    >> we can check a good deal of our logic very early.

    >
    > What errors are you referring to? I use the "prototypes, main,
    > subroutines" layout, and I have not had any problems with it.
    > True, if I change the calling sequence of a subroutine, I have to
    > update the prototype, but if I do not, I get a compilation error.


    I think whether or not you get an error is compiler dependant.
    However we normally take pains to define constants in one place,
    and then use that definition throughout. Why do you suddenly want
    to take an entirely different attitude to function prototypes? I
    see no reason to ever keep two separate entities in sync manually.

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
     
    CBFalconer, Apr 12, 2005
    #10
  11. Andrew Clark

    Alan Balmer Guest

    On Tue, 12 Apr 2005 09:08:24 +0200, Christian Kandeler
    <_invalid> wrote:

    >Emmanuel Delahaye wrote:
    >
    >> int fgetline (FILE *, char *);
    >> /* -ed- avoid separated prototypes with a better layout... */

    >
    >Does anyone agree with this notion? Having separate prototypes at the top of
    >a file relieves me of the burden of having to arrange my function
    >implementations in a specific order; more specifically, it allows me to
    >always have main() as the first function. For that, I am willing to pay a
    >price in the form of a small, innocuous redundancy.
    >

    And it puts the prototypes all in one place, which I like. I do not
    agree with Emmanuel on this one.

    --
    Al Balmer
    Balmer Consulting
     
    Alan Balmer, Apr 12, 2005
    #11
  12. Andrew Clark

    Alan Balmer Guest

    On Tue, 12 Apr 2005 07:42:33 GMT, CBFalconer <>
    wrote:

    >Christian Kandeler wrote:
    >> Emmanuel Delahaye wrote:
    >>
    >>> int fgetline (FILE *, char *);
    >>> /* -ed- avoid separated prototypes with a better layout... */

    >>
    >> Does anyone agree with this notion? Having separate prototypes at
    >> the top of a file relieves me of the burden of having to arrange my
    >> function implementations in a specific order; more specifically, it
    >> allows me to always have main() as the first function. For that, I
    >> am willing to pay a price in the form of a small, innocuous
    >> redundancy.

    >
    >Definitely. That redundance can be the source of silly hard to
    >find errors.

    How so? My compiler happily informs me if there's any discrepancy.

    >By paying strict attention to "declare and define
    >before use" I always know which way to look for the routine
    >source.

    Me, too. It's just the opposite direction.

    >The only (and rare) exception is when designing mutually
    >recursive functions.
    >
    >Since we are no longer using line oriented editors there is no
    >penalty to be paid for writing main first, and then moving above it
    >to create the various subroutines. By creating them as stubs we
    >can check a good deal of our logic very early.


    But that has nothing to do with the order in the text file.

    --
    Al Balmer
    Balmer Consulting
     
    Alan Balmer, Apr 12, 2005
    #12
  13. CBFalconer wrote:
    > William Hughes wrote:
    > > CBFalconer wrote:
    > >> Christian Kandeler wrote:
    > >>> Emmanuel Delahaye wrote:
    > >>>
    > >>>> int fgetline (FILE *, char *);
    > >>>> /* -ed- avoid separated prototypes with a better layout... */
    > >>>
    > >>> Does anyone agree with this notion? Having separate prototypes
    > >>> at the top of a file relieves me of the burden of having to
    > >>> arrange my function implementations in a specific order; more
    > >>> specifically, it allows me to always have main() as the first
    > >>> function. For that, I am willing to pay a price in the form of
    > >>> a small, innocuous redundancy.
    > >>
    > >> Definitely. That redundance can be the source of silly hard to
    > >> find errors. By paying strict attention to "declare and define
    > >> before use" I always know which way to look for the routine
    > >> source. The only (and rare) exception is when designing
    > >> mutually recursive functions.
    > >>
    > >> Since we are no longer using line oriented editors there is no
    > >> penalty to be paid for writing main first, and then moving above
    > >> it to create the various subroutines. By creating them as stubs
    > >> we can check a good deal of our logic very early.

    > >
    > > What errors are you referring to? I use the "prototypes, main,
    > > subroutines" layout, and I have not had any problems with it.
    > > True, if I change the calling sequence of a subroutine, I have to
    > > update the prototype, but if I do not, I get a compilation error.

    >
    > I think whether or not you get an error is compiler dependant.


    Probably. I use gcc for development, so maybe I am spoiled here.
    However, I would expect any good compiler to pick
    up on a prototype/definition error. Are there
    compilers that do not catch this?


    > However we normally take pains to define constants in one place,
    > and then use that definition throughout. Why do you suddenly want
    > to take an entirely different attitude to function prototypes?


    Possibly because of the use of separate .h files for prototypes
    of functions to be called from outside of the source file
    (the source file includes the .h file in order to catch
    prototype/definition conflicts). Possibly, because we started
    doing it that way and had no problems.

    >I see no reason to ever keep two separate entities in sync manually.


    Well it's not much work (especially as we tend to use separate files
    for each function, so prototype maintanence cannot usually be
    avoided). Still, I guess I would agree with you, I just don't
    consider it a very important.

    -William Hughes
     
    William Hughes, Apr 12, 2005
    #13
  14. In article <>, CBFalconer <> writes:
    > Christian Kandeler wrote:
    > > Emmanuel Delahaye wrote:
    > >
    > >> int fgetline (FILE *, char *);
    > >> /* -ed- avoid separated prototypes with a better layout... */

    > >
    > > Does anyone agree with this notion? Having separate prototypes at
    > > the top of a file relieves me of the burden of having to arrange my
    > > function implementations in a specific order; more specifically, it
    > > allows me to always have main() as the first function.


    Always? Very few of the C source files I create have main() in them
    at all. But no doubt milage varies.

    > Definitely. That redundance can be the source of silly hard to
    > find errors.


    How so? If the prototype does not match the definition, it's a
    constraint violation. (C90 6.5: "All declarations in the same scope
    that refer to the same object or function shall specify compatible
    types"; by the rules of 6.5.4.3, any meaningful mismatch between a
    prototype that includes a parameter list and the definition of the
    corresponding function will produce incompatible function types.)

    > By paying strict attention to "declare and define
    > before use" I always know which way to look for the routine
    > source.


    Only if it's in the same TU, presumably. And is searching for a
    function definition difficult?

    > Since we are no longer using line oriented editors there is no
    > penalty to be paid for writing main first, and then moving above it
    > to create the various subroutines.


    A similar argument could be made regarding editors with decent
    search capabilities, code-indexing features (like ctags, "browse"
    facilties, etc), and the like, and not worrying about the relative
    position of various functions in a TU.

    > By creating them as stubs we
    > can check a good deal of our logic very early.


    I fail to see how their order in the TU has any effect on this
    principle.

    I have no argument with "declare dependents first" as a matter of
    style, but I think its purported advantages are largely subjective.

    Personally, I prefer to arrange the functions in an order which seems
    to me to be logical while reading the source; typically that involves
    introducing higher-level functions first (and using meaningful names
    for lower-level ones, plus of course ample comments), though "helper"
    leaf functions that are only used in one or two places may preceed
    their callers. But I stick to no hard and fast rule in this matter.
    I prefer to preserve it as a degree of stylistic freedom, one of
    several I try to employ to keep the code readable. Much as I might
    use a dependent clause on its own as a sentence in prose.

    --
    Michael Wojcik

    Proverbs for Paranoids, 2: The innocence of the creatures is in inverse
    proportion to the immorality of the Master. -- Thomas Pynchon
     
    Michael Wojcik, Apr 12, 2005
    #14
  15. On Tue, 12 Apr 2005 07:42:33 GMT, in comp.lang.c , CBFalconer
    <> wrote:

    >Christian Kandeler wrote:
    >> Emmanuel Delahaye wrote:
    >>
    >>> int fgetline (FILE *, char *);
    >>> /* -ed- avoid separated prototypes with a better layout... */

    >>
    >> Does anyone agree with this notion?

    >
    >Definitely. That redundance can be the source of silly hard to
    >find errors.


    FWIW I disagree. Prototyping in my experience /removes/ the ability to
    have annoying errors, especially when functions are split over several
    files.

    >By paying strict attention to "declare and define
    >before use" I always know which way to look for the routine
    >source.


    I personally find this a completely painful way to work. If my editor
    won't tell me where to find a function, I get a better editor :)

    --
    Mark McIntyre
    CLC FAQ <http://www.eskimo.com/~scs/C-faq/top.html>
    CLC readme: <http://www.ungerhu.com/jxh/clc.welcome.txt>

    ----== Posted via Newsfeeds.Com - Unlimited-Uncensored-Secure Usenet News==----
    http://www.newsfeeds.com The #1 Newsgroup Service in the World! 120,000+ Newsgroups
    ----= East and West-Coast Server Farms - Total Privacy via Encryption =----
     
    Mark McIntyre, Apr 12, 2005
    #15
  16. Andrew Clark

    Joe Wright Guest

    Mark McIntyre wrote:
    > On Tue, 12 Apr 2005 07:42:33 GMT, in comp.lang.c , CBFalconer
    > <> wrote:
    >
    >
    >>Christian Kandeler wrote:
    >>
    >>>Emmanuel Delahaye wrote:
    >>>
    >>>
    >>>>int fgetline (FILE *, char *);
    >>>>/* -ed- avoid separated prototypes with a better layout... */
    >>>
    >>>Does anyone agree with this notion?

    >>
    >>Definitely. That redundance can be the source of silly hard to
    >>find errors.

    >
    >
    > FWIW I disagree. Prototyping in my experience /removes/ the ability to
    > have annoying errors, especially when functions are split over several
    > files.
    >
    >
    >>By paying strict attention to "declare and define
    >>before use" I always know which way to look for the routine
    >>source.

    >
    >
    > I personally find this a completely painful way to work. If my editor
    > won't tell me where to find a function, I get a better editor :)


    I apologize for not snipping but I am really commenting on all of it.
    I'm pretty sure that CBFalconer and I agree completely on this. Given a
    single translation unit for your program, it makes no sense to prototype
    functions before main so that you can have something like..

    #include <sdtio.h>

    int squar(int);

    int main(void) {
    int i = 3;
    printf("The square of %d is %d\n", i, squar(i));
    return 0;
    }

    int squar(int n) {
    return n * n;
    }

    ...is silly IMO.

    If functions are in one or more other files, as Mark suggests, I would
    create a header with those prototypes and #include it before main.

    If there is more than one function in main.c, 'main()' is the last of them.

    YMMV

    --
    Joe Wright mailto:
    "Everything should be made as simple as possible, but not simpler."
    --- Albert Einstein ---
     
    Joe Wright, Apr 12, 2005
    #16
  17. Andrew Clark

    Alan Balmer Guest

    On Tue, 12 Apr 2005 18:40:28 -0400, Joe Wright
    <> wrote:

    >I apologize for not snipping but I am really commenting on all of it.
    >I'm pretty sure that CBFalconer and I agree completely on this. Given a
    >single translation unit for your program, it makes no sense to prototype
    >functions before main so that you can have something like..
    >
    >#include <sdtio.h>
    >
    >int squar(int);
    >
    >int main(void) {
    > int i = 3;
    > printf("The square of %d is %d\n", i, squar(i));
    > return 0;
    >}
    >
    >int squar(int n) {
    > return n * n;
    >}


    I wish the programs I work with were really like that!

    Instead, I have something like:

    /* local functions */
    static void get_campus_id(void) ;
    static int read_rec(FILE *p);
    static int file_report(void);
    static int read_rec1(FILE *p);
    static int file_report1(void);
    static void open_db(void);
    static int acct_type(void);
    static void init_op_totals(void);
    static void do_all_totals(void);
    static struct acct_sum *is_account_in_totals_list(int
    account_number,struct acct_sum *base_ptr,int *position,int
    *total_accounts);
    static int acct_comp(void *parg1,void *parg2);
    static void do_totals(void);
    static void is_account_in_list(void) ;
    static int r_lock( int rec_type,char *lock_type,int dbnum);
    static int qualify_tran2(void);
    static void acct_list(void);
    static void priv_list(void);
    static int comp(void *parg1,void *parg2);
    static void do_detail_header(int no_data);
    static int do_report(void);
    static void submit_report(void);
    static void setup_report(void);
    static void set_terminals(void);
    static struct a* lookup_acct(unsigned short acct_num);

    followed by main, then slightly under 3000 lines of code inplementing
    those functions. Your way, that puts main at about the middle of page
    49. That doesn't appeal to me.

    Besides, your way it's much more difficult to print the list of
    function prototypes and hang it on the wall ;-)

    --
    Al Balmer
    Balmer Consulting
     
    Alan Balmer, Apr 13, 2005
    #17
  18. Alan Balmer wrote on 13/04/05 :
    > I wish the programs I work with were really like that!
    >
    > Instead, I have something like:
    >
    > /* local functions */
    > static void get_campus_id(void) ;
    > static int read_rec(FILE *p);

    <...>
    >
    > followed by main, then slightly under 3000 lines of code inplementing


    3000 lines in a single source ? You have a design problem...

    > those functions. Your way, that puts main at about the middle of page
    > 49. That doesn't appeal to me.
    >

    The good place for main() is the last function of main.c

    Easy to implement, easy to find and Vulcan-logic.

    > Besides, your way it's much more difficult to print the list of
    > function prototypes and hang it on the wall ;-)


    If you have a good code organisation, you just have to print out the
    headers where the detached prototypes are.

    The 'static' functions are local and probably of lower interest. Whatis
    important are the entry points.

    I recommend this layout:

    Headers

    /* macros
    ============================================================== */
    /* constants
    =========================================================== */
    /* types
    =============================================================== */
    /* structures
    ========================================================== */
    /* internal public functions
    =========================================== */
    /* internal public data
    ================================================ */
    /* entry points
    ======================================================== */
    /* public variables
    ==================================================== */


    Compile units (not main)

    /* macros
    ============================================================== */
    /* constants
    =========================================================== */
    /* types
    =============================================================== */
    /* structures
    ========================================================== */
    /* private variables
    =================================================== */
    /* private functions
    =================================================== */
    /* internal public functions
    =========================================== */
    /* internal public data
    ================================================ */
    /* entry points
    ======================================================== */
    /* public variables
    ==================================================== */

    main.c (main entry point)

    /* macros
    ============================================================== */
    /* constants
    =========================================================== */
    /* types
    =============================================================== */
    /* structures
    ========================================================== */
    /* private variables
    =================================================== */
    /* private functions
    =================================================== */
    /* entry points
    ======================================================== */

    --
    Emmanuel
    The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
    The C-library: http://www.dinkumware.com/refxc.html

    "There are 10 types of people in the world today;
    those that understand binary, and those that dont."
     
    Emmanuel Delahaye, Apr 13, 2005
    #18
  19. Andrew Clark

    Alan Balmer Guest

    On Wed, 13 Apr 2005 19:41:02 +0200, "Emmanuel Delahaye"
    <> wrote:

    >Alan Balmer wrote on 13/04/05 :
    >> I wish the programs I work with were really like that!
    >>
    >> Instead, I have something like:
    >>
    >> /* local functions */
    >> static void get_campus_id(void) ;
    >> static int read_rec(FILE *p);

    ><...>
    >>
    >> followed by main, then slightly under 3000 lines of code inplementing

    >
    >3000 lines in a single source ? You have a design problem...
    >

    Why should I separate local function, used by no other program, into
    separate source modules?

    >> those functions. Your way, that puts main at about the middle of page
    >> 49. That doesn't appeal to me.
    >>

    >The good place for main() is the last function of main.c
    >

    None of the main programs I work with are named main.c. And you're
    just repeating what you said before. And it would still put main about
    the middle of page 49.

    >Easy to implement, easy to find and Vulcan-logic.
    >
    >> Besides, your way it's much more difficult to print the list of
    >> function prototypes and hang it on the wall ;-)

    >
    >If you have a good code organisation, you just have to print out the
    >headers where the detached prototypes are.
    >

    Headers? For static functions? How would putting the prototypes in a
    separate header serve your goal of not having separate prototypes?

    >The 'static' functions are local and probably of lower interest. Whatis
    >important are the entry points.
    >

    There's one entry point - main(). This is a program, not a library. I
    don't know why the local functions would be of "lower interest." The
    program won't work without them.

    <snipped another of the hundreds of "recommended layouts" I've seen in
    the last 35 years - and one of the less desirable ones.>

    --
    Al Balmer
    Balmer Consulting
     
    Alan Balmer, Apr 13, 2005
    #19
  20. On Tue, 12 Apr 2005 18:40:28 -0400, in comp.lang.c , Joe Wright
    <> wrote:

    >Mark McIntyre wrote:
    >> On Tue, 12 Apr 2005 07:42:33 GMT, in comp.lang.c , CBFalconer
    >> <> wrote:
    >>

    (of putting main at the end of the file and avoiding prototypes...)

    >>>Definitely. That redundance can be the source of silly hard to
    >>>find errors.

    >>
    >> FWIW I disagree.


    >I apologize for not snipping but I am really commenting on all of it.
    >I'm pretty sure that CBFalconer and I agree completely on this. Given a
    >single translation unit for your program, it makes no sense to prototype
    >functions before main


    My problem is that this is fine for tiny programs, with a half-dozen
    functions.

    But as soon as you get a few lengthy functions, it becomes quite
    annoying. And it makes for significant maintenance the second you need
    to split the project across multiple files. So my tendency is to start
    out as if the program would later become multifile. I even tend to put
    all the fn prototypes into a header straight away.

    --
    Mark McIntyre
    CLC FAQ <http://www.eskimo.com/~scs/C-faq/top.html>
    CLC readme: <http://www.ungerhu.com/jxh/clc.welcome.txt>
     
    Mark McIntyre, Apr 13, 2005
    #20
    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. Fao, Sean

    Critique Request: CheckBoxColumn

    Fao, Sean, Feb 15, 2006, in forum: ASP .Net
    Replies:
    0
    Views:
    526
    Fao, Sean
    Feb 15, 2006
  2. Ike

    Pls critique

    Ike, Oct 1, 2003, in forum: Java
    Replies:
    1
    Views:
    459
    John C. Bollinger
    Oct 1, 2003
  3. Cynthia Turcotte

    critique request

    Cynthia Turcotte, Sep 12, 2003, in forum: HTML
    Replies:
    7
    Views:
    496
    Chris Leonard
    Sep 13, 2003
  4. Andrew Cameron

    Critique request: x01

    Andrew Cameron, Sep 14, 2003, in forum: HTML
    Replies:
    53
    Views:
    1,388
    picayunish
    Sep 17, 2003
  5. Dariusz

    Test page critique

    Dariusz, Sep 15, 2003, in forum: HTML
    Replies:
    16
    Views:
    624
    Mark Jones
    Sep 16, 2003
Loading...

Share This Page