Re: Critique
- From: "Peter Nilsson" <airia@xxxxxxxxxxx>
- Date: 12 Apr 2005 05:43:55 -0700
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
.
- References:
- Critique
- From: Andrew Clark
- Critique
- Prev by Date: Re: Critique
- Next by Date: Re: malloc problem
- Previous by thread: Re: Critique
- Next by thread: Question about multiple definition and undefined reference
- Index(es):
Relevant Pages
|
