My own functions for working with files

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
4
down vote

favorite
1












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;







share|improve this question

























    up vote
    4
    down vote

    favorite
    1












    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;







    share|improve this question





















      up vote
      4
      down vote

      favorite
      1









      up vote
      4
      down vote

      favorite
      1






      1





      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;







      share|improve this question











      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;









      share|improve this question










      share|improve this question




      share|improve this question









      asked Jun 9 at 16:52









      eanmos

      1265




      1265




















          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.






          share|improve this answer




























            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.






            share|improve this answer

















            • 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










            • @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 :-)
              – 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










            Your Answer




            StackExchange.ifUsing("editor", function ()
            return StackExchange.using("mathjaxEditing", function ()
            StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
            StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
            );
            );
            , "mathjax-editing");

            StackExchange.ifUsing("editor", function ()
            StackExchange.using("externalEditor", function ()
            StackExchange.using("snippets", function ()
            StackExchange.snippets.init();
            );
            );
            , "code-snippets");

            StackExchange.ready(function()
            var channelOptions =
            tags: "".split(" "),
            id: "196"
            ;
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function()
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled)
            StackExchange.using("snippets", function()
            createEditor();
            );

            else
            createEditor();

            );

            function createEditor()
            StackExchange.prepareEditor(
            heartbeatType: 'answer',
            convertImagesToLinks: false,
            noModals: false,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            );



            );








             

            draft saved


            draft discarded


















            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






























            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.






            share|improve this answer

























              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.






              share|improve this answer























                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.






                share|improve this answer













                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.







                share|improve this answer













                share|improve this answer



                share|improve this answer











                answered Jun 10 at 0:27









                user1118321

                10.1k11144




                10.1k11144






















                    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.






                    share|improve this answer

















                    • 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










                    • @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 :-)
                      – 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














                    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.






                    share|improve this answer

















                    • 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










                    • @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 :-)
                      – 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












                    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.






                    share|improve this answer













                    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.







                    share|improve this answer













                    share|improve this answer



                    share|improve this answer











                    answered Jun 9 at 20:45









                    TimFinnegan

                    512




                    512







                    • 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










                    • @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 :-)
                      – 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




                      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 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 :-)
                      – 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 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 :-)
                    – t3chb0t
                    Jun 9 at 21:41





                    @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 :-)
                    – 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












                     

                    draft saved


                    draft discarded


























                     


                    draft saved


                    draft discarded














                    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













































































                    Popular posts from this blog

                    Chat program with C++ and SFML

                    Function to Return a JSON Like Objects Using VBA Collections and Arrays

                    Will my employers contract hold up in court?