Critique

A

Andrew Clark

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 */
 
C

CBFalconer

Andrew said:
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/>
 
E

Emmanuel Delahaye

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."
 
C

Christian Kandeler

Emmanuel said:
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
 
C

CBFalconer

Christian said:
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.
 
L

Lawrence Kirby

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
 
P

Peter Nilsson

Andrew said:
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
#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?

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>
 
W

William Hughes

CBFalconer said:
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
 
A

Arthur J. O'Dwyer

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
 
C

CBFalconer

William said:
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.
 
A

Alan Balmer

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.
 
A

Alan Balmer

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.
 
W

William Hughes

CBFalconer said:
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
 
M

Michael Wojcik

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.
 
M

Mark McIntyre

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 :)
 
J

Joe Wright

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




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
 
A

Alan Balmer

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 ;-)
 
E

Emmanuel Delahaye

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."
 
A

Alan Balmer

Alan Balmer wrote on 13/04/05 :


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?
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.


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.>
 
M

Mark McIntyre

(of putting main at the end of the file and avoiding prototypes...)
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.
 

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,479
Members
44,899
Latest member
RodneyMcAu

Latest Threads

Top