My own functions for working with files
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
4
down vote
favorite
I wrote the functions for opening a file, getting a file size and getting a file contents. I would welcome any suggestions to improve my code.
You might think I'm over-commenting the code. But I do it mostly just for the visual separation of the code parts. I hope to know your opinion about it.
P. S. English isn't my native language, but I try to write all code comments in English. So I would welcome any corrections also!
/*
===============
open_file
===============
*/
FILE *open_file(char const * restrict name, char const * restrict mode)
/* Debug: checking the input arguments. */
assert(name != NULL);
assert(mode != NULL);
/* Opening the file and checking if an error occurs. */
FILE *file = fopen(name, mode);
if (file == NULL)
LOG("Error: the file "%s" couldn't be opened (in the "%s" mode). "
"The reason: "%s".n", name, mode, strerror(errno));
return file;
/*
===============
get_file_size
===============
*/
long get_file_size(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Setting the file position to the ending of the file. */
fseek(file, 0, SEEK_END);
/* Getting the size of the file. */
const long size = ftell(file);
if (size == -1)
LOG("Error: can't get the current value of the file position. The "
"reason: "%s".n", strerror(errno));
/* Setting the file position to the beginning of the file. */
rewind(file);
return size;
/*
===============
get_file_contents
===============
*/
char *get_file_contents(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Getting the size of the file. */
long const size = get_file_size(file);
if (size == -1)
return NULL;
/* Allocating memory for the contents of the file. */
char * const contents = calloc(size + 1, 1);
if (contents == NULL)
LOG("Error: can't allocate %ld bytes for a contents of a file. "
"The reason: "%s".n", size, strerror(errno));
/* Reading the contents of the file and checking if an error occurs. */
size_t const read_size = fread(contents, 1, size, file);
if (read_size < (size_t) size)
LOG("Warning: the size of elements successfully read (%zu bytes) is "
"less than the size of the file (%ld bytes). It also may occurs "
"if the file uses the CR+LF line endings (it is the Microsoft "
"Windows system usually).n", read_size, size);
return contents;
c file
add a comment |Â
up vote
4
down vote
favorite
I wrote the functions for opening a file, getting a file size and getting a file contents. I would welcome any suggestions to improve my code.
You might think I'm over-commenting the code. But I do it mostly just for the visual separation of the code parts. I hope to know your opinion about it.
P. S. English isn't my native language, but I try to write all code comments in English. So I would welcome any corrections also!
/*
===============
open_file
===============
*/
FILE *open_file(char const * restrict name, char const * restrict mode)
/* Debug: checking the input arguments. */
assert(name != NULL);
assert(mode != NULL);
/* Opening the file and checking if an error occurs. */
FILE *file = fopen(name, mode);
if (file == NULL)
LOG("Error: the file "%s" couldn't be opened (in the "%s" mode). "
"The reason: "%s".n", name, mode, strerror(errno));
return file;
/*
===============
get_file_size
===============
*/
long get_file_size(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Setting the file position to the ending of the file. */
fseek(file, 0, SEEK_END);
/* Getting the size of the file. */
const long size = ftell(file);
if (size == -1)
LOG("Error: can't get the current value of the file position. The "
"reason: "%s".n", strerror(errno));
/* Setting the file position to the beginning of the file. */
rewind(file);
return size;
/*
===============
get_file_contents
===============
*/
char *get_file_contents(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Getting the size of the file. */
long const size = get_file_size(file);
if (size == -1)
return NULL;
/* Allocating memory for the contents of the file. */
char * const contents = calloc(size + 1, 1);
if (contents == NULL)
LOG("Error: can't allocate %ld bytes for a contents of a file. "
"The reason: "%s".n", size, strerror(errno));
/* Reading the contents of the file and checking if an error occurs. */
size_t const read_size = fread(contents, 1, size, file);
if (read_size < (size_t) size)
LOG("Warning: the size of elements successfully read (%zu bytes) is "
"less than the size of the file (%ld bytes). It also may occurs "
"if the file uses the CR+LF line endings (it is the Microsoft "
"Windows system usually).n", read_size, size);
return contents;
c file
add a comment |Â
up vote
4
down vote
favorite
up vote
4
down vote
favorite
I wrote the functions for opening a file, getting a file size and getting a file contents. I would welcome any suggestions to improve my code.
You might think I'm over-commenting the code. But I do it mostly just for the visual separation of the code parts. I hope to know your opinion about it.
P. S. English isn't my native language, but I try to write all code comments in English. So I would welcome any corrections also!
/*
===============
open_file
===============
*/
FILE *open_file(char const * restrict name, char const * restrict mode)
/* Debug: checking the input arguments. */
assert(name != NULL);
assert(mode != NULL);
/* Opening the file and checking if an error occurs. */
FILE *file = fopen(name, mode);
if (file == NULL)
LOG("Error: the file "%s" couldn't be opened (in the "%s" mode). "
"The reason: "%s".n", name, mode, strerror(errno));
return file;
/*
===============
get_file_size
===============
*/
long get_file_size(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Setting the file position to the ending of the file. */
fseek(file, 0, SEEK_END);
/* Getting the size of the file. */
const long size = ftell(file);
if (size == -1)
LOG("Error: can't get the current value of the file position. The "
"reason: "%s".n", strerror(errno));
/* Setting the file position to the beginning of the file. */
rewind(file);
return size;
/*
===============
get_file_contents
===============
*/
char *get_file_contents(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Getting the size of the file. */
long const size = get_file_size(file);
if (size == -1)
return NULL;
/* Allocating memory for the contents of the file. */
char * const contents = calloc(size + 1, 1);
if (contents == NULL)
LOG("Error: can't allocate %ld bytes for a contents of a file. "
"The reason: "%s".n", size, strerror(errno));
/* Reading the contents of the file and checking if an error occurs. */
size_t const read_size = fread(contents, 1, size, file);
if (read_size < (size_t) size)
LOG("Warning: the size of elements successfully read (%zu bytes) is "
"less than the size of the file (%ld bytes). It also may occurs "
"if the file uses the CR+LF line endings (it is the Microsoft "
"Windows system usually).n", read_size, size);
return contents;
c file
I wrote the functions for opening a file, getting a file size and getting a file contents. I would welcome any suggestions to improve my code.
You might think I'm over-commenting the code. But I do it mostly just for the visual separation of the code parts. I hope to know your opinion about it.
P. S. English isn't my native language, but I try to write all code comments in English. So I would welcome any corrections also!
/*
===============
open_file
===============
*/
FILE *open_file(char const * restrict name, char const * restrict mode)
/* Debug: checking the input arguments. */
assert(name != NULL);
assert(mode != NULL);
/* Opening the file and checking if an error occurs. */
FILE *file = fopen(name, mode);
if (file == NULL)
LOG("Error: the file "%s" couldn't be opened (in the "%s" mode). "
"The reason: "%s".n", name, mode, strerror(errno));
return file;
/*
===============
get_file_size
===============
*/
long get_file_size(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Setting the file position to the ending of the file. */
fseek(file, 0, SEEK_END);
/* Getting the size of the file. */
const long size = ftell(file);
if (size == -1)
LOG("Error: can't get the current value of the file position. The "
"reason: "%s".n", strerror(errno));
/* Setting the file position to the beginning of the file. */
rewind(file);
return size;
/*
===============
get_file_contents
===============
*/
char *get_file_contents(FILE *file)
/* Debug: checking the input arguments. */
assert(file != NULL);
/* Getting the size of the file. */
long const size = get_file_size(file);
if (size == -1)
return NULL;
/* Allocating memory for the contents of the file. */
char * const contents = calloc(size + 1, 1);
if (contents == NULL)
LOG("Error: can't allocate %ld bytes for a contents of a file. "
"The reason: "%s".n", size, strerror(errno));
/* Reading the contents of the file and checking if an error occurs. */
size_t const read_size = fread(contents, 1, size, file);
if (read_size < (size_t) size)
LOG("Warning: the size of elements successfully read (%zu bytes) is "
"less than the size of the file (%ld bytes). It also may occurs "
"if the file uses the CR+LF line endings (it is the Microsoft "
"Windows system usually).n", read_size, size);
return contents;
c file
asked Jun 9 at 16:52
eanmos
1265
1265
add a comment |Â
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
3
down vote
accepted
Don't Reinvent The Wheel
It might be better to use built-in functions that already do what you want. You can use stat()
to get the size of a file. You don't even need read or write permission to the file. You need to #include <sys/stat.h>
and pass it a path to a file and a pointer to a struct stat
. (On Windows you can use _stat()
et al.) It would look something like this:
#include <sys/stat.h>
long get_file_size(const char *file_path)
/* Debug: checking the input arguments. */
assert(file_path != NULL);
struct stat status;
int err = stat(file_path, &status);
long size = -1;
if (err == -1)
LOG("Error: can't get the of the file. The "
"reason: "%s".n", strerror(errno));
else
size = status.st_size;
return size;
While this function is about as long as yours, it doesn't need to open the file and seek to the end. You can use it to get sizes on files you can't read, for example. This is handy in situations where you want to let a user check how much disk space is used, for example. You don't want to open
, seek
, close
each file.
Remove Unnecessary Stuff
I don't see a lot of point in your open_file()
function. It does one more thing than fopen()
alone, which is print out an error if it fails. That's usually something you want to leave up to the caller, anyway, as one message is probably not appropriate for all the cases where you open a file.
Watch Out For Errors!
Your get_file_contents()
function has an error in it. In the case where the memory can't be allocated, it reads data into contents
, which will be NULL
which will crash.
add a comment |Â
up vote
4
down vote
I must admit that I didn't went through all of your code and have not even compiled it. Some ideas for improvement:
open_file:
I would change your function from
FILE *open_file(char const * restrict name, char const * restrict mode)
to something like this
int open_file(const char *name, const char *modes, FILE **file)
I prefer using a return value that indicates a successful function execution for functions that can fail. By using this you don't have to remember that the function is not successful if the return values is NULL
. You will always get a FAILURE
return for each function. Inside the function you could use something like this:
int open_file(const char *name, const char *modes, FILE **file)
if (name == NULL) goto fail;
if (modes == NULL) goto fail;
*file = fopen(name, modes);
if (*file == NULL) goto fail;
return SUCCESS;
fail:
return FAILURE;
Even if someone tells you using goto
would be bad. I absolutely love it for this situation. The main reason against goto
is, that it can create code that is hard to understand. But in this case to me there is no clearer version due to the usage of goto
.
If that is still not readable enough for you, you can create macros for error-checking. In these macros you could also integrate printing or logging error messages. For example for developement you could switch on printing error messages and for production code compile it without any error messages if the user shall not see them.
#include <stdio.h>
#include <stdlib.h>
#define SUCCESS EXIT_SUCCESS
#define FAILURE EXIT_FAILURE
#define ERRTEST_ARG_NULL(arg, tag)
do
if (!(arg))
printf("%s:%d: In function '%s': error: %s: ERROR_ARG_NULLn", __FILE__, __LINE__, __func__, tag);
goto fail;
while (0)
int open_file(const char *name, const char *modes, FILE **file)
ERRTEST_ARG_NULL(name, "name");
ERRTEST_ARG_NULL(modes, "modes");
*file = fopen(name, modes);
ERRTEST_ARG_NULL(*file, "*file");
return SUCCESS;
fail:
return FAILURE;
int main(void)
FILE *file;
printf("%dn", open_file("C:/pathToFile/file.txt", "rb", &file));
getchar();
return 0;
assert
I would go with the above solution for error-checking because you are able to continue with you program even if an error occured. Think of an user that tries to open a file but uses a wrong file name. Using 'assert' your program would stop working from the first problem.
fread
'fread' will fail for loading bigger files than 1GB (at least on my machine). You can still use it for bigger files by reading them sequentially in chunks with sizes smaller than 1GB and then adding them at the right spot in your allocated array.
+1 for introducing restrict
to me. That was new to me.
1
I would never advise anyone to use agoto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.
â pacmaninbw
Jun 9 at 21:10
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations wheregoto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying agoto
is evil isn't wise :-)
â t3chb0t
Jun 9 at 21:41
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
 |Â
show 2 more comments
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
3
down vote
accepted
Don't Reinvent The Wheel
It might be better to use built-in functions that already do what you want. You can use stat()
to get the size of a file. You don't even need read or write permission to the file. You need to #include <sys/stat.h>
and pass it a path to a file and a pointer to a struct stat
. (On Windows you can use _stat()
et al.) It would look something like this:
#include <sys/stat.h>
long get_file_size(const char *file_path)
/* Debug: checking the input arguments. */
assert(file_path != NULL);
struct stat status;
int err = stat(file_path, &status);
long size = -1;
if (err == -1)
LOG("Error: can't get the of the file. The "
"reason: "%s".n", strerror(errno));
else
size = status.st_size;
return size;
While this function is about as long as yours, it doesn't need to open the file and seek to the end. You can use it to get sizes on files you can't read, for example. This is handy in situations where you want to let a user check how much disk space is used, for example. You don't want to open
, seek
, close
each file.
Remove Unnecessary Stuff
I don't see a lot of point in your open_file()
function. It does one more thing than fopen()
alone, which is print out an error if it fails. That's usually something you want to leave up to the caller, anyway, as one message is probably not appropriate for all the cases where you open a file.
Watch Out For Errors!
Your get_file_contents()
function has an error in it. In the case where the memory can't be allocated, it reads data into contents
, which will be NULL
which will crash.
add a comment |Â
up vote
3
down vote
accepted
Don't Reinvent The Wheel
It might be better to use built-in functions that already do what you want. You can use stat()
to get the size of a file. You don't even need read or write permission to the file. You need to #include <sys/stat.h>
and pass it a path to a file and a pointer to a struct stat
. (On Windows you can use _stat()
et al.) It would look something like this:
#include <sys/stat.h>
long get_file_size(const char *file_path)
/* Debug: checking the input arguments. */
assert(file_path != NULL);
struct stat status;
int err = stat(file_path, &status);
long size = -1;
if (err == -1)
LOG("Error: can't get the of the file. The "
"reason: "%s".n", strerror(errno));
else
size = status.st_size;
return size;
While this function is about as long as yours, it doesn't need to open the file and seek to the end. You can use it to get sizes on files you can't read, for example. This is handy in situations where you want to let a user check how much disk space is used, for example. You don't want to open
, seek
, close
each file.
Remove Unnecessary Stuff
I don't see a lot of point in your open_file()
function. It does one more thing than fopen()
alone, which is print out an error if it fails. That's usually something you want to leave up to the caller, anyway, as one message is probably not appropriate for all the cases where you open a file.
Watch Out For Errors!
Your get_file_contents()
function has an error in it. In the case where the memory can't be allocated, it reads data into contents
, which will be NULL
which will crash.
add a comment |Â
up vote
3
down vote
accepted
up vote
3
down vote
accepted
Don't Reinvent The Wheel
It might be better to use built-in functions that already do what you want. You can use stat()
to get the size of a file. You don't even need read or write permission to the file. You need to #include <sys/stat.h>
and pass it a path to a file and a pointer to a struct stat
. (On Windows you can use _stat()
et al.) It would look something like this:
#include <sys/stat.h>
long get_file_size(const char *file_path)
/* Debug: checking the input arguments. */
assert(file_path != NULL);
struct stat status;
int err = stat(file_path, &status);
long size = -1;
if (err == -1)
LOG("Error: can't get the of the file. The "
"reason: "%s".n", strerror(errno));
else
size = status.st_size;
return size;
While this function is about as long as yours, it doesn't need to open the file and seek to the end. You can use it to get sizes on files you can't read, for example. This is handy in situations where you want to let a user check how much disk space is used, for example. You don't want to open
, seek
, close
each file.
Remove Unnecessary Stuff
I don't see a lot of point in your open_file()
function. It does one more thing than fopen()
alone, which is print out an error if it fails. That's usually something you want to leave up to the caller, anyway, as one message is probably not appropriate for all the cases where you open a file.
Watch Out For Errors!
Your get_file_contents()
function has an error in it. In the case where the memory can't be allocated, it reads data into contents
, which will be NULL
which will crash.
Don't Reinvent The Wheel
It might be better to use built-in functions that already do what you want. You can use stat()
to get the size of a file. You don't even need read or write permission to the file. You need to #include <sys/stat.h>
and pass it a path to a file and a pointer to a struct stat
. (On Windows you can use _stat()
et al.) It would look something like this:
#include <sys/stat.h>
long get_file_size(const char *file_path)
/* Debug: checking the input arguments. */
assert(file_path != NULL);
struct stat status;
int err = stat(file_path, &status);
long size = -1;
if (err == -1)
LOG("Error: can't get the of the file. The "
"reason: "%s".n", strerror(errno));
else
size = status.st_size;
return size;
While this function is about as long as yours, it doesn't need to open the file and seek to the end. You can use it to get sizes on files you can't read, for example. This is handy in situations where you want to let a user check how much disk space is used, for example. You don't want to open
, seek
, close
each file.
Remove Unnecessary Stuff
I don't see a lot of point in your open_file()
function. It does one more thing than fopen()
alone, which is print out an error if it fails. That's usually something you want to leave up to the caller, anyway, as one message is probably not appropriate for all the cases where you open a file.
Watch Out For Errors!
Your get_file_contents()
function has an error in it. In the case where the memory can't be allocated, it reads data into contents
, which will be NULL
which will crash.
answered Jun 10 at 0:27
user1118321
10.1k11144
10.1k11144
add a comment |Â
add a comment |Â
up vote
4
down vote
I must admit that I didn't went through all of your code and have not even compiled it. Some ideas for improvement:
open_file:
I would change your function from
FILE *open_file(char const * restrict name, char const * restrict mode)
to something like this
int open_file(const char *name, const char *modes, FILE **file)
I prefer using a return value that indicates a successful function execution for functions that can fail. By using this you don't have to remember that the function is not successful if the return values is NULL
. You will always get a FAILURE
return for each function. Inside the function you could use something like this:
int open_file(const char *name, const char *modes, FILE **file)
if (name == NULL) goto fail;
if (modes == NULL) goto fail;
*file = fopen(name, modes);
if (*file == NULL) goto fail;
return SUCCESS;
fail:
return FAILURE;
Even if someone tells you using goto
would be bad. I absolutely love it for this situation. The main reason against goto
is, that it can create code that is hard to understand. But in this case to me there is no clearer version due to the usage of goto
.
If that is still not readable enough for you, you can create macros for error-checking. In these macros you could also integrate printing or logging error messages. For example for developement you could switch on printing error messages and for production code compile it without any error messages if the user shall not see them.
#include <stdio.h>
#include <stdlib.h>
#define SUCCESS EXIT_SUCCESS
#define FAILURE EXIT_FAILURE
#define ERRTEST_ARG_NULL(arg, tag)
do
if (!(arg))
printf("%s:%d: In function '%s': error: %s: ERROR_ARG_NULLn", __FILE__, __LINE__, __func__, tag);
goto fail;
while (0)
int open_file(const char *name, const char *modes, FILE **file)
ERRTEST_ARG_NULL(name, "name");
ERRTEST_ARG_NULL(modes, "modes");
*file = fopen(name, modes);
ERRTEST_ARG_NULL(*file, "*file");
return SUCCESS;
fail:
return FAILURE;
int main(void)
FILE *file;
printf("%dn", open_file("C:/pathToFile/file.txt", "rb", &file));
getchar();
return 0;
assert
I would go with the above solution for error-checking because you are able to continue with you program even if an error occured. Think of an user that tries to open a file but uses a wrong file name. Using 'assert' your program would stop working from the first problem.
fread
'fread' will fail for loading bigger files than 1GB (at least on my machine). You can still use it for bigger files by reading them sequentially in chunks with sizes smaller than 1GB and then adding them at the right spot in your allocated array.
+1 for introducing restrict
to me. That was new to me.
1
I would never advise anyone to use agoto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.
â pacmaninbw
Jun 9 at 21:10
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations wheregoto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying agoto
is evil isn't wise :-)
â t3chb0t
Jun 9 at 21:41
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
 |Â
show 2 more comments
up vote
4
down vote
I must admit that I didn't went through all of your code and have not even compiled it. Some ideas for improvement:
open_file:
I would change your function from
FILE *open_file(char const * restrict name, char const * restrict mode)
to something like this
int open_file(const char *name, const char *modes, FILE **file)
I prefer using a return value that indicates a successful function execution for functions that can fail. By using this you don't have to remember that the function is not successful if the return values is NULL
. You will always get a FAILURE
return for each function. Inside the function you could use something like this:
int open_file(const char *name, const char *modes, FILE **file)
if (name == NULL) goto fail;
if (modes == NULL) goto fail;
*file = fopen(name, modes);
if (*file == NULL) goto fail;
return SUCCESS;
fail:
return FAILURE;
Even if someone tells you using goto
would be bad. I absolutely love it for this situation. The main reason against goto
is, that it can create code that is hard to understand. But in this case to me there is no clearer version due to the usage of goto
.
If that is still not readable enough for you, you can create macros for error-checking. In these macros you could also integrate printing or logging error messages. For example for developement you could switch on printing error messages and for production code compile it without any error messages if the user shall not see them.
#include <stdio.h>
#include <stdlib.h>
#define SUCCESS EXIT_SUCCESS
#define FAILURE EXIT_FAILURE
#define ERRTEST_ARG_NULL(arg, tag)
do
if (!(arg))
printf("%s:%d: In function '%s': error: %s: ERROR_ARG_NULLn", __FILE__, __LINE__, __func__, tag);
goto fail;
while (0)
int open_file(const char *name, const char *modes, FILE **file)
ERRTEST_ARG_NULL(name, "name");
ERRTEST_ARG_NULL(modes, "modes");
*file = fopen(name, modes);
ERRTEST_ARG_NULL(*file, "*file");
return SUCCESS;
fail:
return FAILURE;
int main(void)
FILE *file;
printf("%dn", open_file("C:/pathToFile/file.txt", "rb", &file));
getchar();
return 0;
assert
I would go with the above solution for error-checking because you are able to continue with you program even if an error occured. Think of an user that tries to open a file but uses a wrong file name. Using 'assert' your program would stop working from the first problem.
fread
'fread' will fail for loading bigger files than 1GB (at least on my machine). You can still use it for bigger files by reading them sequentially in chunks with sizes smaller than 1GB and then adding them at the right spot in your allocated array.
+1 for introducing restrict
to me. That was new to me.
1
I would never advise anyone to use agoto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.
â pacmaninbw
Jun 9 at 21:10
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations wheregoto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying agoto
is evil isn't wise :-)
â t3chb0t
Jun 9 at 21:41
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
 |Â
show 2 more comments
up vote
4
down vote
up vote
4
down vote
I must admit that I didn't went through all of your code and have not even compiled it. Some ideas for improvement:
open_file:
I would change your function from
FILE *open_file(char const * restrict name, char const * restrict mode)
to something like this
int open_file(const char *name, const char *modes, FILE **file)
I prefer using a return value that indicates a successful function execution for functions that can fail. By using this you don't have to remember that the function is not successful if the return values is NULL
. You will always get a FAILURE
return for each function. Inside the function you could use something like this:
int open_file(const char *name, const char *modes, FILE **file)
if (name == NULL) goto fail;
if (modes == NULL) goto fail;
*file = fopen(name, modes);
if (*file == NULL) goto fail;
return SUCCESS;
fail:
return FAILURE;
Even if someone tells you using goto
would be bad. I absolutely love it for this situation. The main reason against goto
is, that it can create code that is hard to understand. But in this case to me there is no clearer version due to the usage of goto
.
If that is still not readable enough for you, you can create macros for error-checking. In these macros you could also integrate printing or logging error messages. For example for developement you could switch on printing error messages and for production code compile it without any error messages if the user shall not see them.
#include <stdio.h>
#include <stdlib.h>
#define SUCCESS EXIT_SUCCESS
#define FAILURE EXIT_FAILURE
#define ERRTEST_ARG_NULL(arg, tag)
do
if (!(arg))
printf("%s:%d: In function '%s': error: %s: ERROR_ARG_NULLn", __FILE__, __LINE__, __func__, tag);
goto fail;
while (0)
int open_file(const char *name, const char *modes, FILE **file)
ERRTEST_ARG_NULL(name, "name");
ERRTEST_ARG_NULL(modes, "modes");
*file = fopen(name, modes);
ERRTEST_ARG_NULL(*file, "*file");
return SUCCESS;
fail:
return FAILURE;
int main(void)
FILE *file;
printf("%dn", open_file("C:/pathToFile/file.txt", "rb", &file));
getchar();
return 0;
assert
I would go with the above solution for error-checking because you are able to continue with you program even if an error occured. Think of an user that tries to open a file but uses a wrong file name. Using 'assert' your program would stop working from the first problem.
fread
'fread' will fail for loading bigger files than 1GB (at least on my machine). You can still use it for bigger files by reading them sequentially in chunks with sizes smaller than 1GB and then adding them at the right spot in your allocated array.
+1 for introducing restrict
to me. That was new to me.
I must admit that I didn't went through all of your code and have not even compiled it. Some ideas for improvement:
open_file:
I would change your function from
FILE *open_file(char const * restrict name, char const * restrict mode)
to something like this
int open_file(const char *name, const char *modes, FILE **file)
I prefer using a return value that indicates a successful function execution for functions that can fail. By using this you don't have to remember that the function is not successful if the return values is NULL
. You will always get a FAILURE
return for each function. Inside the function you could use something like this:
int open_file(const char *name, const char *modes, FILE **file)
if (name == NULL) goto fail;
if (modes == NULL) goto fail;
*file = fopen(name, modes);
if (*file == NULL) goto fail;
return SUCCESS;
fail:
return FAILURE;
Even if someone tells you using goto
would be bad. I absolutely love it for this situation. The main reason against goto
is, that it can create code that is hard to understand. But in this case to me there is no clearer version due to the usage of goto
.
If that is still not readable enough for you, you can create macros for error-checking. In these macros you could also integrate printing or logging error messages. For example for developement you could switch on printing error messages and for production code compile it without any error messages if the user shall not see them.
#include <stdio.h>
#include <stdlib.h>
#define SUCCESS EXIT_SUCCESS
#define FAILURE EXIT_FAILURE
#define ERRTEST_ARG_NULL(arg, tag)
do
if (!(arg))
printf("%s:%d: In function '%s': error: %s: ERROR_ARG_NULLn", __FILE__, __LINE__, __func__, tag);
goto fail;
while (0)
int open_file(const char *name, const char *modes, FILE **file)
ERRTEST_ARG_NULL(name, "name");
ERRTEST_ARG_NULL(modes, "modes");
*file = fopen(name, modes);
ERRTEST_ARG_NULL(*file, "*file");
return SUCCESS;
fail:
return FAILURE;
int main(void)
FILE *file;
printf("%dn", open_file("C:/pathToFile/file.txt", "rb", &file));
getchar();
return 0;
assert
I would go with the above solution for error-checking because you are able to continue with you program even if an error occured. Think of an user that tries to open a file but uses a wrong file name. Using 'assert' your program would stop working from the first problem.
fread
'fread' will fail for loading bigger files than 1GB (at least on my machine). You can still use it for bigger files by reading them sequentially in chunks with sizes smaller than 1GB and then adding them at the right spot in your allocated array.
+1 for introducing restrict
to me. That was new to me.
answered Jun 9 at 20:45
TimFinnegan
512
512
1
I would never advise anyone to use agoto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.
â pacmaninbw
Jun 9 at 21:10
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations wheregoto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying agoto
is evil isn't wise :-)
â t3chb0t
Jun 9 at 21:41
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
 |Â
show 2 more comments
1
I would never advise anyone to use agoto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.
â pacmaninbw
Jun 9 at 21:10
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations wheregoto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying agoto
is evil isn't wise :-)
â t3chb0t
Jun 9 at 21:41
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
1
1
I would never advise anyone to use a
goto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.â pacmaninbw
Jun 9 at 21:10
I would never advise anyone to use a
goto
on this site. It happens to be a fairly bad programming practice. Among other things it leads to unstructured programming. You also forgot to mention that if the user compiles with NODEBUG defined the assert will simply go away.â pacmaninbw
Jun 9 at 21:10
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations where
goto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying a goto
is evil isn't wise :-)â t3chb0t
Jun 9 at 21:41
@pacmaninbw isn't it a little bit too strong saying never? There are certain situations where
goto
s are pretty useful... like jumping out of nested loops. I find everthing has it's use-case and avoiding it because some people are praying a goto
is evil isn't wise :-)â t3chb0t
Jun 9 at 21:41
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@t3chb0t obviously there are situations where an experienced user can use it. I've advised users on this site to use setjmp and longjmp which is a super goto for a specific purpose. Most of the users on this site may not have the experience to use goto and may not use it properly. If you want to discuss this more you can meet me on the 2nd monitor.
â pacmaninbw
Jun 9 at 22:06
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@pacmaninbw If you exclude your macros for error checking in a different header file that is very likely never again going to be changed and you only work with the macros that will redirect the flow when an error occured to the single point where the function ends with a failure (probably cleaning up can be done before returning), how can that be unstructured programming? See for example: stackoverflow.com/questions/24451/⦠and MPICH implementation from Argonne National Laboratory.
â TimFinnegan
Jun 9 at 22:11
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
@TimFinnegan OK I upvoted that answer because it is correct. Did you notice at the end of the answer that he has only used goto 3 time in 15 to 20 years?
â pacmaninbw
Jun 9 at 22:19
 |Â
show 2 more comments
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f196171%2fmy-own-functions-for-working-with-files%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password