fclose... what is better

[collinm]

hi

is it better to do:

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
fclose(fp);
}
else
printf("fail\n");


or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

thanks

 

[Walter Roberson]

is it better to do:
or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

What is the defined behaviour of fclose(NULL) ?

If you don't know off the top of your head, chances are that
other people maintaining your program will not either. Thus,
regardless of the defined behaviour of fclose(NULL), your
program would be more maintenance-friendly if you do not
invoke that behaviour.


By the way, as a minor matter of style: messages such as "fail"
are most often associated with errors rather than with normal
program behaviour. Messages associated with errors are usually
sent to stderr instead of stdout.

If the unavailability of the file is a routine matter for the program,
then the associated message would usually not be "fail" but instead
something closer to "skipping unavailable file %s\n" or "Sorry, no
winking blinking lights available today!\n"

 

[Thomas Matthews]

hi

is it better to do:

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
fclose(fp);
}
else
printf("fail\n");


or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

thanks

All depends on your style and error handling.
My preference is:
unsigned char successful = 0;
fp = fopen(...);
successful = fp == NULL;
if (!successful)
{
/* File open failure */
}

if (successful)
{
successful = Process_File_Contents();
}

if (successful)
{
/* ... */
}

As for when to close a file, my opinion is to
close it as soon as you are finished with it.
Leaving a file open for longer than necessary
may allow things to happen to the internal data
{by your program or other ones).

--
Thomas Matthews

C++ newsgroup welcome message:
http://www.slack.net/~shiva/welcome.txt
C++ Faq: http://www.parashift.com/c++-faq-lite
C Faq: http://www.eskimo.com/~scs/c-faq/top.html
alt.comp.lang.learn.c-c++ faq:
http://www.comeaucomputing.com/learn/faq/
Other sites:
http://www.josuttis.com -- C++ STL Library book
http://www.sgi.com/tech/stl -- Standard Template Library

 

[Fred L. Kleinschmidt]

hi

is it better to do:

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
fclose(fp);
}
else
printf("fail\n");

or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

Do not call fclose(fp) if fp is NULL.

 

[Eric Sosman]

hi

is it better to do:

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
fclose(fp);
}
else
printf("fail\n");


or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

"Better" depends on what you want to accomplish, of
course. However, note that if the fopen() fails in the
second version you will wind up calling fclose(NULL),
which is Not A Good Idea ...

 

[Malcolm]

is it better to do:

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
fclose(fp);
}
else
printf("fail\n");


or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

As others ahve pointed out, you don't want to call fclose() on a null
pointer.

The fopen() expression is not a disaster, but makes it look as if the
purpose of the call is to check a condition, whilst in fact the purpose is
to assign a file pointer to fp.

So

FILE *fp;

fp = fopen(local_led, "r");
if(fp)
{
/* manipulate the file */
fclose(fp);
}
else
printf("fail\n");

is the style that I use.

 

[August Karlstrom]

hi

is it better to do:

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
fclose(fp);
}
else
printf("fail\n");


or

FILE *fp;
if((fp = fopen(local_led, "r"))!=NULL)
{
printf("oepn\n");
}
else
printf("fail\n");
fclose(fp);

thanks

I think the general recommendation is to deal with exceptions first:

FILE *fp;

fp = fopen(local_led, "r");
if(fp == NULL) {
fprintf(stderr, "Could not open the file %s", local_red);
exit(EXIT_FAILURE);
}
....
fclose(fp);

-- August

 

[Mark]

I think the general recommendation is to deal with exceptions first:

FILE *fp;

fp = fopen(local_led, "r");
if(fp == NULL) {
fprintf(stderr, "Could not open the file %s", local_red);
exit(EXIT_FAILURE);
}
...
fclose(fp);

-- August

'THE' general recommendation? BS.
What happens when you can't exit() because of your requirements?
Do you replace the call with a return? I've fixed more problems caused
by sloppy coding conventions similar to this one you 'recommend'...
especially when multiple files are being opened and manipulated at once.
My personal recommendation would be to code an fopen() as follows:

FILE *fp;
if((fp = fopen(local_led, "r"))) {

fclose(fp);
} else
perror(local_led);

Mark

 

[August Karlstrom]

'THE' general recommendation? BS.
What happens when you can't exit() because of your requirements?
Do you replace the call with a return? I've fixed more problems caused
by sloppy coding conventions similar to this one you 'recommend'...
especially when multiple files are being opened and manipulated at once.
My personal recommendation would be to code an fopen() as follows:

FILE *fp;
if((fp = fopen(local_led, "r"))) {

fclose(fp);
} else
perror(local_led);

Mark

Okay, my point was really the order of the branches:

FILE *fp;

fp = fopen(local_led, "r");
if(fp == NULL) {
/* Do whatever has to be done when local_red cannot be opened. */
} else {
...
fclose(fp);
}

Of course, what you have do when `fopen' fails depends on how the
program is structured; preconditions, postconditions etc. (My previous
example didn't need the else clause because of the invokation of `exit'.)

-- August

 

[Dave Thompson]

On Thu, 19 May 2005 13:56:37 GMT, Thomas Matthews

All depends on your style and error handling.
My preference is:
unsigned char successful = 0;
fp = fopen(...);
successful = fp == NULL;

if (!successful)
{
/* File open failure */
}

if (successful)
{
successful = Process_File_Contents();

You should pass fp, otherwise it needs to be a shared "global" (at
least file-scope static, maybe more) which is yucky(tm) far beyond
this mildly clunky error handling.

Presumably the fclose() is done within Process_File_Contents(), in
which case the name is not fully descriptive, or added here ...

}

because now 'successful' no longer tells you accurately whether the
fopen was successful -- although fp does, it you are willing to
violate the structuring you just put so much effort into.

if (successful)
{
/* ... */
}

As for when to close a file, my opinion is to
close it as soon as you are finished with it.
Leaving a file open for longer than necessary
may allow things to happen to the internal data
{by your program or other ones).

Promptly closing a file (not opened read-only enforced by the OS) can
protect it from malfunctions in other parts of the same program --
although it would be better to fix those malfunctions anyway -- but is
very unlikely to help protect against other _programs_; indeed it may
release locks and allow _more_ access to the file by other programs.

- David.Thompson1 at worldnet.att.net

 

[Chris Croughton]

On Thu, 19 May 2005 13:56:37 GMT, Thomas Matthews


Shirley <G> you mean fp != NULL .

I always put parentheses round conditions in assignments, it's too easy
to read (and type and not notice) it as

successful = fp = NULL;

but putting

successful = (fp == NULL);

makes the condition more obvious, and thus the logical error you point
out stands out more as well.

cHris C