4
\$\begingroup\$

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;
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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_NULL\n", __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("%d\n", 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.

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – pacmaninbw Jun 9 '18 at 21:10
  • \$\begingroup\$ @pacmaninbw isn't it a little bit too strong saying never? There are certain situations where gotos 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 :-) \$\endgroup\$ – t3chb0t Jun 9 '18 at 21:41
  • \$\begingroup\$ @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. \$\endgroup\$ – pacmaninbw Jun 9 '18 at 22:06
  • \$\begingroup\$ @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. \$\endgroup\$ – TimFinnegan Jun 9 '18 at 22:11
  • \$\begingroup\$ @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? \$\endgroup\$ – pacmaninbw Jun 9 '18 at 22:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.