Need sharp criticism on my code.

[grishin-mailing-lists]

Hi guys,

I'm trying to write really neat program.
I can't decide how to write end-of-the loop better (as neatly as
possible, huh).

The loop stops in two cases:
1) EOF is met;
2) desired string is found;

The first is a failure,
the second is a good result.

Well, I break the loop as soon as I met the string.
But how to know whether all the fp has been looked through OR I met
the string *without* adding additional flag, something like
int found = 0;
and bumping it if the string appears.
I wouldn't like to add variables. I'd like more elegant solution.

Another way is to return from the function right after the string has
been got. It leads to code bloat -- I have to add another one
free(buff).

Any suggestions are appreciated.

/* sets fp on the first true function definition */
int FindDef (char *cDefinition, FILE *fp)
{
int result = EXIT_SUCCESS; /* placed in stdlib.h */
char *s = NULL;
char s_case1[] = "Далее"; /* ext var1 */
char s_case2_1[] = ";"; /* ext var2 */
char s_case2_2[] = "Возврат"; /* ext var3 */
char s_case3[] = "//"; /* ext var4 */
char *buff = NULL;

if ( NULL != cDefinition && NULL != fp )
{
fseek( fp, 0L, SEEK_SET );

buff = malloc( 500+1 ); /* dummy constant, but OKAY for now */
if ( NULL != buff )
{
while ( fgets( buff, 500+1, fp ) )
{
s = strstr( buff, cDefinition );
if ( NULL != s )
{

if ( NULL == strstr( buff, s_case1 ) )
{
if ( NULL == strstr( buff, s_case2_1 )
&& NULL == strstr( buff, s_case2_2 )
&& NULL == strstr( buff, s_case3 ) )
{
break;
}
}
}
}
if ( !fgets( buff, 1, fp ) )
{
result = EXIT_FAILURE;
}

}
else
result = EXIT_FAILURE;
}
else
result = EXIT_FAILURE;

free(buff);
return result;
}

 

[Ben Bacarisse]

I'm trying to write really neat program.
I can't decide how to write end-of-the loop better (as neatly as
possible, huh).

The loop stops in two cases:
1) EOF is met;
2) desired string is found;

The first is a failure,
the second is a good result.

Well, I break the loop as soon as I met the string.
But how to know whether all the fp has been looked through OR I met
the string *without* adding additional flag, something like
int found = 0;
and bumping it if the string appears.
I wouldn't like to add variables. I'd like more elegant solution.

I think you can just test for EOF or a read error. See below.

Another way is to return from the function right after the string has
been got. It leads to code bloat -- I have to add another one
free(buff).

Since the function mallocs and frees the buffer, you could use a static
array or an automatic (local) array. If a constant size it too
restrictive, C99 has variably modified arrays. Of course, if you plan
to grow the buffer in the future you will have to use malloc/realloc/free.

Any suggestions are appreciated.

/* sets fp on the first true function definition */
int FindDef (char *cDefinition, FILE *fp)
{
int result = EXIT_SUCCESS; /* placed in stdlib.h */

I'd use these only for exit codes, not function return values. If the
return is success/failure (with no distinction between failure types) I
just use 1 and 0.

char *s = NULL;
char s_case1[] = "Далее"; /* ext var1 */
char s_case2_1[] = ";"; /* ext var2 */
char s_case2_2[] = "Возврат"; /* ext var3 */
char s_case3[] = "//"; /* ext var4 */
char *buff = NULL;

if ( NULL != cDefinition && NULL != fp )
{
fseek( fp, 0L, SEEK_SET );

buff = malloc( 500+1 ); /* dummy constant, but OKAY for now */
if ( NULL != buff )
{
while ( fgets( buff, 500+1, fp ) )
{
s = strstr( buff, cDefinition );
if ( NULL != s )
{

if ( NULL == strstr( buff, s_case1 ) )
{
if ( NULL == strstr( buff, s_case2_1 )
&& NULL == strstr( buff, s_case2_2 )
&& NULL == strstr( buff, s_case3 ) )
{
break;
}
}
}
}
if ( !fgets( buff, 1, fp ) )

You should test for feof or ferror here. When you find the string and
break, this will read (or try to read) another line.

If you abandon the EXIT_FAILURE/SUCCESS codes you can just write

result = feof(fp) || ferror(fp);

here.

You could put the stop (well, the "carry on") condition in the loop
since it is just a bunch of strstr calls. Some people don't like big
conditions like that but they can be quite convenient:

while (fgets(buff, 500+1, fp) &&
(!strstr(buff, cDefinition) ||
strstr(buff, s_case1) || strstr(buff, s_case2_1) ||
strstr(buff, s_case2_2) || strstr(buff, s_case3))) {}

(I may have got that wrong -- check first!).

Even if you don't do this, I would remove the apparent special case when
you test for s_case1. Writing if (C1) if (C2 && C3) break; is the same
as writing if (C1 && C2 && C3) break;

 

[grishin-mailing-lists]

Thank you, Ben.
I've considered all of your suggestions.

I spent about an hour to get this invariant
while (fgets( buff, sizeof buff, fp ) && !(strstr( buff, cDefinition )
&& !( strstr( buff, s_case1 ) || strstr( buff, s_case2_1 ) ||
strstr( buff, s_case2_2 ) || strstr( buff, s_case3 ) )))
{
}

but it seems to me as "too much" because it would be difficult to add
something (especially after a year or two).
I decided to have this one:
while (fgets( buff, sizeof buff, fp ))
{
if (strstr( buff, cDefinition ) && !( strstr( buff, s_case1 )
|| strstr( buff, s_case2_1 ) || strstr( buff, s_case2_2 ) ||
strstr( buff, s_case3 ) ))
{
break;
}
}

Neat enough and can be easily modified.

 

[Ben Bacarisse]

Thank you, Ben.
I've considered all of your suggestions.

I spent about an hour to get this invariant
while (fgets( buff, sizeof buff, fp ) && !(strstr( buff, cDefinition )
&& !( strstr( buff, s_case1 ) || strstr( buff, s_case2_1 ) ||
strstr( buff, s_case2_2 ) || strstr( buff, s_case3 ) )))
{
}

Do you know De Morgan's laws? They are very helpful in cases like this:

!(C1 && C2 && C3...) => !C1 || !C2 || !C3...

!(C1 || C2 || C3...) => !C1 && !C2 && !C3...

Just to check that we both came to the same conclusion, here is your
condition with the function call replaced by simple identifiers:

fgets && !(cDef && !(s_case1 || s_case2_1 || s_case2_2 || s_case3))

De Morgan's laws allow you to re-write the second part of the && as:

fgets && (!cDef || !!(s_case1 || s_case2_1 || s_case2_2 || s_case3))

and we can (in this situation) replace !!C1 with C1:

fgets && (!cDef || (s_case1 || s_case2_1 || s_case2_2 || s_case3))

and remove the now redundant brackets:

fgets && (!cDef || s_case1 || s_case2_1 || s_case2_2 || s_case3)

which is (fortunately!) the condition I suggested.

but it seems to me as "too much" because it would be difficult to add
something (especially after a year or two).

Probably, though my version is a little simpler.

I decided to have this one:
while (fgets( buff, sizeof buff, fp ))
{
if (strstr( buff, cDefinition ) && !( strstr( buff, s_case1 )
|| strstr( buff, s_case2_1 ) || strstr( buff, s_case2_2 ) ||
strstr( buff, s_case3 ) ))
{
break;
}
}

Neat enough and can be easily modified.

The "exceptions" should really be in an array, of course. Then you
could write:

while (fgets(buff, 500+1, fp) &&
!strstr(buff, cDefinition) &&
!match_any(buff, exceptions)) {...}

One day, this might even have to become:

while (fgets(buff, 500+1, fp) &&
match_any(buff, definitons) &&
!match_any(buff, exceptions)) {...}

 

[grishin-mailing-lists]

Do you know De Morgan's laws? They are very helpful in cases like this:

...

Yep. They appeared in a discrete math book I read last year.
Unfortunately there were no connections to real-life problems and I
didn't notice it's possible to use them in my day-to-day work. Now
it's really time to reread the chapter and do some additional
exercises.

The "exceptions" should really be in an array, of course. Then you
could write:

while (fgets(buff, 500+1, fp) &&
!strstr(buff, cDefinition) &&
!match_any(buff, exceptions)) {...}

One day, this might even have to become:

while (fgets(buff, 500+1, fp) &&
match_any(buff, definitons) &&
!match_any(buff, exceptions)) {...}

like that?
char *exceptions[] = { "exception1", "exception2", "exception3" };
and treating them within a subroutine
for (i = 0; **(exception + i); i++ )
{
puts(*(exception + i));
}
Is that correct? I'm not sure about the invariant.

Thank you for your time in this matter.

 

[Ben Bacarisse]

Do you know De Morgan's laws? They are very helpful in cases like this:
...

Yep. They appeared in a discrete math book I read last year.
Unfortunately there were no connections to real-life problems and I
didn't notice it's possible to use them in my day-to-day work. Now
it's really time to reread the chapter and do some additional
exercises.

The "exceptions" should really be in an array, of course. Then you
could write:

while (fgets(buff, 500+1, fp) &&
!strstr(buff, cDefinition) &&
!match_any(buff, exceptions)) {...}

One day, this might even have to become:

while (fgets(buff, 500+1, fp) &&
match_any(buff, definitons) &&
!match_any(buff, exceptions)) {...}

like that?
char *exceptions[] = { "exception1", "exception2", "exception3" };
and treating them within a subroutine
for (i = 0; **(exception + i); i++ )
{
puts(*(exception + i));
}
Is that correct? I'm not sure about the invariant.

First off, it's not an invariant. I think a confusion between loop
conditions and loop invariants has crept into another thread and you
maybe picked up the term from there. Anyway, I assume you mean the loop
condition.

It is indeed not correct. For one thing it looks like you want to stop
the loop when you see a null pointer, but your arrays don't end with
one. You should write:

char *exceptions[] = { "exception1", "exception2", "exception3", NULL };

The pointers in the array are simply

exceptions[0], exceptions[1] .. exceptions[3]

No need for ** or adding to a pointer (yes, X means *(X+I) but the []
notation is there to be used).

I presume the puts is from some testing you were doing. The match_any
function would return true (1) as soon as buff is seen to contain
exceptions as a substring (using strstr). It would return false (0)
if the loop ends without have seen any such match.

Thank you for your time in this matter.

You're welcome.