Basic C copy file implementation

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












9












$begingroup$


I'm a java programmer by trade and starting to learn C/C++. I'd appreciate any pointers on glaring issues with my code style which is acceptable in java but not in C. I feel like open_files is probably bad in C I'm used to that sort of thing in java to make code easier to scan ( setting *src_file on one line and testing the next)



#include <stdio.h>
#include <string.h>

void get_paths(char *src, char *dest, int buf_size)
fprintf(stdout, "Please enter src path:n");
fgets(src, buf_size, stdin);
src[strcspn(src, "rn")] = 0;
fprintf(stdout, "nPlease enter dest path:n");
fgets(dest, buf_size, stdin);
dest[strcspn(dest, "rn")] = 0;


int open_files(char *src, char *dest, FILE **src_file, FILE **dest_file)
*src_file = fopen(src, "r");
if(!*src_file)
fprintf(stderr, "n%s is not a valid file or permission deniedn", src);
return 0;


*dest_file = fopen(dest, "w");
if(!*dest_file)
fprintf(stderr, "ncould not open %s for writing, check path exists and you have write permissionsn", dest);
fclose(*src_file);
return 0;

return 1;


int cpy(char *cpy_buf, FILE *src_file, FILE *dest_file)
size_t num_elements;
while((num_elements = fread(cpy_buf, sizeof(char), sizeof(cpy_buf), src_file)) > 0)
if(fwrite(cpy_buf, sizeof(char), num_elements, dest_file) != num_elements)
fprintf(stderr, "nError while writing to destination nAborting...");
return 0;



return 1;



int main()
const int buf_size = 256;
const int cpy_buf_siz = 64;
char src[buf_size];
char dest[buf_size];
char cpy_buf[cpy_buf_siz];
FILE *src_file;
FILE *dest_file;

get_paths(src, dest, buf_size);
fprintf(stdout, "nAttempting to copy from %s to %s...n", src, dest);

if(!open_files(src, dest, &src_file, &dest_file))
return -1;

fprintf(stdout, "nStarting copy from %s to %s...n", src, dest);
int success = cpy(cpy_buf, src_file, dest_file);
fclose(src_file);
fclose(dest_file);

return success ? 0 : 1;










share|improve this question











$endgroup$







  • 1




    $begingroup$
    You should not use fread() in units of more than one byte. It is designed to allow transfers of any multiple of any length, but it is misdesigned to return the number of transfers rather than the number of bytes, so it cannot report a short transfer. As a consequence it is almost certainly going to misreport the data count at end of file.
    $endgroup$
    – user207421
    Feb 11 at 8:56







  • 2




    $begingroup$
    If you're learning C++, then I don't necessarily recommend going via C. OTOH if you're learning both C and C++, prepare for a bumpy ride!
    $endgroup$
    – Toby Speight
    Feb 11 at 10:58










  • $begingroup$
    Good use of src[strcspn(src, "rn")] = 0;.
    $endgroup$
    – chux
    Feb 14 at 4:41















9












$begingroup$


I'm a java programmer by trade and starting to learn C/C++. I'd appreciate any pointers on glaring issues with my code style which is acceptable in java but not in C. I feel like open_files is probably bad in C I'm used to that sort of thing in java to make code easier to scan ( setting *src_file on one line and testing the next)



#include <stdio.h>
#include <string.h>

void get_paths(char *src, char *dest, int buf_size)
fprintf(stdout, "Please enter src path:n");
fgets(src, buf_size, stdin);
src[strcspn(src, "rn")] = 0;
fprintf(stdout, "nPlease enter dest path:n");
fgets(dest, buf_size, stdin);
dest[strcspn(dest, "rn")] = 0;


int open_files(char *src, char *dest, FILE **src_file, FILE **dest_file)
*src_file = fopen(src, "r");
if(!*src_file)
fprintf(stderr, "n%s is not a valid file or permission deniedn", src);
return 0;


*dest_file = fopen(dest, "w");
if(!*dest_file)
fprintf(stderr, "ncould not open %s for writing, check path exists and you have write permissionsn", dest);
fclose(*src_file);
return 0;

return 1;


int cpy(char *cpy_buf, FILE *src_file, FILE *dest_file)
size_t num_elements;
while((num_elements = fread(cpy_buf, sizeof(char), sizeof(cpy_buf), src_file)) > 0)
if(fwrite(cpy_buf, sizeof(char), num_elements, dest_file) != num_elements)
fprintf(stderr, "nError while writing to destination nAborting...");
return 0;



return 1;



int main()
const int buf_size = 256;
const int cpy_buf_siz = 64;
char src[buf_size];
char dest[buf_size];
char cpy_buf[cpy_buf_siz];
FILE *src_file;
FILE *dest_file;

get_paths(src, dest, buf_size);
fprintf(stdout, "nAttempting to copy from %s to %s...n", src, dest);

if(!open_files(src, dest, &src_file, &dest_file))
return -1;

fprintf(stdout, "nStarting copy from %s to %s...n", src, dest);
int success = cpy(cpy_buf, src_file, dest_file);
fclose(src_file);
fclose(dest_file);

return success ? 0 : 1;










share|improve this question











$endgroup$







  • 1




    $begingroup$
    You should not use fread() in units of more than one byte. It is designed to allow transfers of any multiple of any length, but it is misdesigned to return the number of transfers rather than the number of bytes, so it cannot report a short transfer. As a consequence it is almost certainly going to misreport the data count at end of file.
    $endgroup$
    – user207421
    Feb 11 at 8:56







  • 2




    $begingroup$
    If you're learning C++, then I don't necessarily recommend going via C. OTOH if you're learning both C and C++, prepare for a bumpy ride!
    $endgroup$
    – Toby Speight
    Feb 11 at 10:58










  • $begingroup$
    Good use of src[strcspn(src, "rn")] = 0;.
    $endgroup$
    – chux
    Feb 14 at 4:41













9












9








9


3



$begingroup$


I'm a java programmer by trade and starting to learn C/C++. I'd appreciate any pointers on glaring issues with my code style which is acceptable in java but not in C. I feel like open_files is probably bad in C I'm used to that sort of thing in java to make code easier to scan ( setting *src_file on one line and testing the next)



#include <stdio.h>
#include <string.h>

void get_paths(char *src, char *dest, int buf_size)
fprintf(stdout, "Please enter src path:n");
fgets(src, buf_size, stdin);
src[strcspn(src, "rn")] = 0;
fprintf(stdout, "nPlease enter dest path:n");
fgets(dest, buf_size, stdin);
dest[strcspn(dest, "rn")] = 0;


int open_files(char *src, char *dest, FILE **src_file, FILE **dest_file)
*src_file = fopen(src, "r");
if(!*src_file)
fprintf(stderr, "n%s is not a valid file or permission deniedn", src);
return 0;


*dest_file = fopen(dest, "w");
if(!*dest_file)
fprintf(stderr, "ncould not open %s for writing, check path exists and you have write permissionsn", dest);
fclose(*src_file);
return 0;

return 1;


int cpy(char *cpy_buf, FILE *src_file, FILE *dest_file)
size_t num_elements;
while((num_elements = fread(cpy_buf, sizeof(char), sizeof(cpy_buf), src_file)) > 0)
if(fwrite(cpy_buf, sizeof(char), num_elements, dest_file) != num_elements)
fprintf(stderr, "nError while writing to destination nAborting...");
return 0;



return 1;



int main()
const int buf_size = 256;
const int cpy_buf_siz = 64;
char src[buf_size];
char dest[buf_size];
char cpy_buf[cpy_buf_siz];
FILE *src_file;
FILE *dest_file;

get_paths(src, dest, buf_size);
fprintf(stdout, "nAttempting to copy from %s to %s...n", src, dest);

if(!open_files(src, dest, &src_file, &dest_file))
return -1;

fprintf(stdout, "nStarting copy from %s to %s...n", src, dest);
int success = cpy(cpy_buf, src_file, dest_file);
fclose(src_file);
fclose(dest_file);

return success ? 0 : 1;










share|improve this question











$endgroup$




I'm a java programmer by trade and starting to learn C/C++. I'd appreciate any pointers on glaring issues with my code style which is acceptable in java but not in C. I feel like open_files is probably bad in C I'm used to that sort of thing in java to make code easier to scan ( setting *src_file on one line and testing the next)



#include <stdio.h>
#include <string.h>

void get_paths(char *src, char *dest, int buf_size)
fprintf(stdout, "Please enter src path:n");
fgets(src, buf_size, stdin);
src[strcspn(src, "rn")] = 0;
fprintf(stdout, "nPlease enter dest path:n");
fgets(dest, buf_size, stdin);
dest[strcspn(dest, "rn")] = 0;


int open_files(char *src, char *dest, FILE **src_file, FILE **dest_file)
*src_file = fopen(src, "r");
if(!*src_file)
fprintf(stderr, "n%s is not a valid file or permission deniedn", src);
return 0;


*dest_file = fopen(dest, "w");
if(!*dest_file)
fprintf(stderr, "ncould not open %s for writing, check path exists and you have write permissionsn", dest);
fclose(*src_file);
return 0;

return 1;


int cpy(char *cpy_buf, FILE *src_file, FILE *dest_file)
size_t num_elements;
while((num_elements = fread(cpy_buf, sizeof(char), sizeof(cpy_buf), src_file)) > 0)
if(fwrite(cpy_buf, sizeof(char), num_elements, dest_file) != num_elements)
fprintf(stderr, "nError while writing to destination nAborting...");
return 0;



return 1;



int main()
const int buf_size = 256;
const int cpy_buf_siz = 64;
char src[buf_size];
char dest[buf_size];
char cpy_buf[cpy_buf_siz];
FILE *src_file;
FILE *dest_file;

get_paths(src, dest, buf_size);
fprintf(stdout, "nAttempting to copy from %s to %s...n", src, dest);

if(!open_files(src, dest, &src_file, &dest_file))
return -1;

fprintf(stdout, "nStarting copy from %s to %s...n", src, dest);
int success = cpy(cpy_buf, src_file, dest_file);
fclose(src_file);
fclose(dest_file);

return success ? 0 : 1;







c file






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Feb 10 at 22:12









200_success

130k16153417




130k16153417










asked Feb 10 at 15:16









SMCSMC

1966




1966







  • 1




    $begingroup$
    You should not use fread() in units of more than one byte. It is designed to allow transfers of any multiple of any length, but it is misdesigned to return the number of transfers rather than the number of bytes, so it cannot report a short transfer. As a consequence it is almost certainly going to misreport the data count at end of file.
    $endgroup$
    – user207421
    Feb 11 at 8:56







  • 2




    $begingroup$
    If you're learning C++, then I don't necessarily recommend going via C. OTOH if you're learning both C and C++, prepare for a bumpy ride!
    $endgroup$
    – Toby Speight
    Feb 11 at 10:58










  • $begingroup$
    Good use of src[strcspn(src, "rn")] = 0;.
    $endgroup$
    – chux
    Feb 14 at 4:41












  • 1




    $begingroup$
    You should not use fread() in units of more than one byte. It is designed to allow transfers of any multiple of any length, but it is misdesigned to return the number of transfers rather than the number of bytes, so it cannot report a short transfer. As a consequence it is almost certainly going to misreport the data count at end of file.
    $endgroup$
    – user207421
    Feb 11 at 8:56







  • 2




    $begingroup$
    If you're learning C++, then I don't necessarily recommend going via C. OTOH if you're learning both C and C++, prepare for a bumpy ride!
    $endgroup$
    – Toby Speight
    Feb 11 at 10:58










  • $begingroup$
    Good use of src[strcspn(src, "rn")] = 0;.
    $endgroup$
    – chux
    Feb 14 at 4:41







1




1




$begingroup$
You should not use fread() in units of more than one byte. It is designed to allow transfers of any multiple of any length, but it is misdesigned to return the number of transfers rather than the number of bytes, so it cannot report a short transfer. As a consequence it is almost certainly going to misreport the data count at end of file.
$endgroup$
– user207421
Feb 11 at 8:56





$begingroup$
You should not use fread() in units of more than one byte. It is designed to allow transfers of any multiple of any length, but it is misdesigned to return the number of transfers rather than the number of bytes, so it cannot report a short transfer. As a consequence it is almost certainly going to misreport the data count at end of file.
$endgroup$
– user207421
Feb 11 at 8:56





2




2




$begingroup$
If you're learning C++, then I don't necessarily recommend going via C. OTOH if you're learning both C and C++, prepare for a bumpy ride!
$endgroup$
– Toby Speight
Feb 11 at 10:58




$begingroup$
If you're learning C++, then I don't necessarily recommend going via C. OTOH if you're learning both C and C++, prepare for a bumpy ride!
$endgroup$
– Toby Speight
Feb 11 at 10:58












$begingroup$
Good use of src[strcspn(src, "rn")] = 0;.
$endgroup$
– chux
Feb 14 at 4:41




$begingroup$
Good use of src[strcspn(src, "rn")] = 0;.
$endgroup$
– chux
Feb 14 at 4:41










5 Answers
5






active

oldest

votes


















10












$begingroup$

Prefer Symbolic Constants Over Magic Numbers



There is a header file that should be included, stdlib.h, that provides some standard symbolic constants such as EXIT_SUCCESS and EXIT_FAILURE. It might also be better to define buf_size and cpy_buf_siz as symbolic constants. If you are using a C compiler use #define to define your constants, if you are using the c++ compiler use the const or constexpr. The open_files function could also return these values.



By using EXIT_SUCCESS and EXIT_FAILURE as the return values in the cpy function when main exists success doesn't need to be tested it can just be returned.



Test for Success or Failure on all User Input



The input value from fgets() is not checked to see if it is a valid value. NULL would not be a valid value in this case. The user could be prompted in a loop for each file name.



DRY Code



The function get_paths() could use a function such as char *get_path(char* prompt) that takes a prompt and returns a single string or NULL on failure. The error handling for fgets() could be in this function.



It might be better if cpy() called open_files(). That would simplify the cpy() interface to



int cpy(char* src, char* dest);


If the C++ compiler is being used, it would probably be better to use std::cin and std::cout rather than fgets().






share|improve this answer











$endgroup$












  • $begingroup$
    I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
    $endgroup$
    – SMC
    Feb 10 at 19:21






  • 1




    $begingroup$
    @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
    $endgroup$
    – pacmaninbw
    Feb 10 at 19:44






  • 3




    $begingroup$
    @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
    $endgroup$
    – Roland Illig
    Feb 11 at 10:16



















11












$begingroup$


  • You test the return value of fwrite, which is good. However, fread may fail as well. Since fread doesn't distinguish error from end of file, you should call ferror:



     while ((num_elements = fread(....)) > 0) 
    ....

    if (ferror(src))
    handle_error



  • fprintf(stdout), while technically valid, looks strange. A printf suffices.


  • cpy_buf doesn't belong to main. Define it directly in cpy.


  • Prefer passing file names via command line arguments.


  • The "Aborting..." message doesn't have a terminating newline, and the next prompt will be printed on the same line (on any shell except cmd.exe). Make a habit to print a newline at the end of the message.






share|improve this answer









$endgroup$




















    5












    $begingroup$

    Regarding these kinds of statements:



    fprintf(stderr, "n%s is not a valid file or permission deniedn", src);


    When the error indication is from a C library function, we should call



    perror( "my error message" );


    That will output to stderr both your error message AND the text reason the system thinks the error occurred.






    share|improve this answer











    $endgroup$




















      4












      $begingroup$

      One additional issue is with sizeof(cpy_buf) in cpy. This will be the size of a pointer (usually 4 or 8 bytes) and not the size of the buffer that cpy_buf points to. If you follow the advice given in other answers to define cpy_buf within cpy, rathar than pass it in as a parameter, this won't be a problem.



      Also a small read/write buffer is inefficient. You should use at least 512 bytes, and preferably something much larger (like 2048 or 16384 bytes).






      share|improve this answer









      $endgroup$




















        1












        $begingroup$

        In your functions, you're outputting a string showing an error occurred, then returning 0, which usually means "Success!". Then in your main() function, you're checking whether or not the functions succeed or fail. Have you tested the return codes and whether you're getting the correct return values?






        share|improve this answer









        $endgroup$












        • $begingroup$
          This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
          $endgroup$
          – SMC
          Feb 12 at 16:49










        • $begingroup$
          And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
          $endgroup$
          – Canadian Luke
          Feb 12 at 17:23










        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',
        autoActivateHeartbeat: false,
        convertImagesToLinks: false,
        noModals: true,
        showLowRepImageUploadWarning: true,
        reputationToPostImages: null,
        bindNavPrevention: true,
        postfix: "",
        imageUploader:
        brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
        contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
        allowUrls: true
        ,
        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%2f213193%2fbasic-c-copy-file-implementation%23new-answer', 'question_page');

        );

        Post as a guest















        Required, but never shown

























        5 Answers
        5






        active

        oldest

        votes








        5 Answers
        5






        active

        oldest

        votes









        active

        oldest

        votes






        active

        oldest

        votes









        10












        $begingroup$

        Prefer Symbolic Constants Over Magic Numbers



        There is a header file that should be included, stdlib.h, that provides some standard symbolic constants such as EXIT_SUCCESS and EXIT_FAILURE. It might also be better to define buf_size and cpy_buf_siz as symbolic constants. If you are using a C compiler use #define to define your constants, if you are using the c++ compiler use the const or constexpr. The open_files function could also return these values.



        By using EXIT_SUCCESS and EXIT_FAILURE as the return values in the cpy function when main exists success doesn't need to be tested it can just be returned.



        Test for Success or Failure on all User Input



        The input value from fgets() is not checked to see if it is a valid value. NULL would not be a valid value in this case. The user could be prompted in a loop for each file name.



        DRY Code



        The function get_paths() could use a function such as char *get_path(char* prompt) that takes a prompt and returns a single string or NULL on failure. The error handling for fgets() could be in this function.



        It might be better if cpy() called open_files(). That would simplify the cpy() interface to



        int cpy(char* src, char* dest);


        If the C++ compiler is being used, it would probably be better to use std::cin and std::cout rather than fgets().






        share|improve this answer











        $endgroup$












        • $begingroup$
          I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
          $endgroup$
          – SMC
          Feb 10 at 19:21






        • 1




          $begingroup$
          @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
          $endgroup$
          – pacmaninbw
          Feb 10 at 19:44






        • 3




          $begingroup$
          @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
          $endgroup$
          – Roland Illig
          Feb 11 at 10:16
















        10












        $begingroup$

        Prefer Symbolic Constants Over Magic Numbers



        There is a header file that should be included, stdlib.h, that provides some standard symbolic constants such as EXIT_SUCCESS and EXIT_FAILURE. It might also be better to define buf_size and cpy_buf_siz as symbolic constants. If you are using a C compiler use #define to define your constants, if you are using the c++ compiler use the const or constexpr. The open_files function could also return these values.



        By using EXIT_SUCCESS and EXIT_FAILURE as the return values in the cpy function when main exists success doesn't need to be tested it can just be returned.



        Test for Success or Failure on all User Input



        The input value from fgets() is not checked to see if it is a valid value. NULL would not be a valid value in this case. The user could be prompted in a loop for each file name.



        DRY Code



        The function get_paths() could use a function such as char *get_path(char* prompt) that takes a prompt and returns a single string or NULL on failure. The error handling for fgets() could be in this function.



        It might be better if cpy() called open_files(). That would simplify the cpy() interface to



        int cpy(char* src, char* dest);


        If the C++ compiler is being used, it would probably be better to use std::cin and std::cout rather than fgets().






        share|improve this answer











        $endgroup$












        • $begingroup$
          I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
          $endgroup$
          – SMC
          Feb 10 at 19:21






        • 1




          $begingroup$
          @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
          $endgroup$
          – pacmaninbw
          Feb 10 at 19:44






        • 3




          $begingroup$
          @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
          $endgroup$
          – Roland Illig
          Feb 11 at 10:16














        10












        10








        10





        $begingroup$

        Prefer Symbolic Constants Over Magic Numbers



        There is a header file that should be included, stdlib.h, that provides some standard symbolic constants such as EXIT_SUCCESS and EXIT_FAILURE. It might also be better to define buf_size and cpy_buf_siz as symbolic constants. If you are using a C compiler use #define to define your constants, if you are using the c++ compiler use the const or constexpr. The open_files function could also return these values.



        By using EXIT_SUCCESS and EXIT_FAILURE as the return values in the cpy function when main exists success doesn't need to be tested it can just be returned.



        Test for Success or Failure on all User Input



        The input value from fgets() is not checked to see if it is a valid value. NULL would not be a valid value in this case. The user could be prompted in a loop for each file name.



        DRY Code



        The function get_paths() could use a function such as char *get_path(char* prompt) that takes a prompt and returns a single string or NULL on failure. The error handling for fgets() could be in this function.



        It might be better if cpy() called open_files(). That would simplify the cpy() interface to



        int cpy(char* src, char* dest);


        If the C++ compiler is being used, it would probably be better to use std::cin and std::cout rather than fgets().






        share|improve this answer











        $endgroup$



        Prefer Symbolic Constants Over Magic Numbers



        There is a header file that should be included, stdlib.h, that provides some standard symbolic constants such as EXIT_SUCCESS and EXIT_FAILURE. It might also be better to define buf_size and cpy_buf_siz as symbolic constants. If you are using a C compiler use #define to define your constants, if you are using the c++ compiler use the const or constexpr. The open_files function could also return these values.



        By using EXIT_SUCCESS and EXIT_FAILURE as the return values in the cpy function when main exists success doesn't need to be tested it can just be returned.



        Test for Success or Failure on all User Input



        The input value from fgets() is not checked to see if it is a valid value. NULL would not be a valid value in this case. The user could be prompted in a loop for each file name.



        DRY Code



        The function get_paths() could use a function such as char *get_path(char* prompt) that takes a prompt and returns a single string or NULL on failure. The error handling for fgets() could be in this function.



        It might be better if cpy() called open_files(). That would simplify the cpy() interface to



        int cpy(char* src, char* dest);


        If the C++ compiler is being used, it would probably be better to use std::cin and std::cout rather than fgets().







        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited Feb 10 at 23:43









        Clonkex

        1033




        1033










        answered Feb 10 at 16:23









        pacmaninbwpacmaninbw

        5,19321537




        5,19321537











        • $begingroup$
          I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
          $endgroup$
          – SMC
          Feb 10 at 19:21






        • 1




          $begingroup$
          @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
          $endgroup$
          – pacmaninbw
          Feb 10 at 19:44






        • 3




          $begingroup$
          @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
          $endgroup$
          – Roland Illig
          Feb 11 at 10:16

















        • $begingroup$
          I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
          $endgroup$
          – SMC
          Feb 10 at 19:21






        • 1




          $begingroup$
          @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
          $endgroup$
          – pacmaninbw
          Feb 10 at 19:44






        • 3




          $begingroup$
          @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
          $endgroup$
          – Roland Illig
          Feb 11 at 10:16
















        $begingroup$
        I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
        $endgroup$
        – SMC
        Feb 10 at 19:21




        $begingroup$
        I assume it's acceptable to keep using 0 and 1 for return values for other functions like open_files or should the code be refactored to also use EXIT_SUCCESS/FAILURE instead?
        $endgroup$
        – SMC
        Feb 10 at 19:21




        1




        1




        $begingroup$
        @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
        $endgroup$
        – pacmaninbw
        Feb 10 at 19:44




        $begingroup$
        @SMC generally functions can return anything, but if I were writing the code for open_files I'd use EXIT_SUCCESS/FAILURE because that's what the code is doing.
        $endgroup$
        – pacmaninbw
        Feb 10 at 19:44




        3




        3




        $begingroup$
        @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
        $endgroup$
        – Roland Illig
        Feb 11 at 10:16





        $begingroup$
        @pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.
        $endgroup$
        – Roland Illig
        Feb 11 at 10:16














        11












        $begingroup$


        • You test the return value of fwrite, which is good. However, fread may fail as well. Since fread doesn't distinguish error from end of file, you should call ferror:



           while ((num_elements = fread(....)) > 0) 
          ....

          if (ferror(src))
          handle_error



        • fprintf(stdout), while technically valid, looks strange. A printf suffices.


        • cpy_buf doesn't belong to main. Define it directly in cpy.


        • Prefer passing file names via command line arguments.


        • The "Aborting..." message doesn't have a terminating newline, and the next prompt will be printed on the same line (on any shell except cmd.exe). Make a habit to print a newline at the end of the message.






        share|improve this answer









        $endgroup$

















          11












          $begingroup$


          • You test the return value of fwrite, which is good. However, fread may fail as well. Since fread doesn't distinguish error from end of file, you should call ferror:



             while ((num_elements = fread(....)) > 0) 
            ....

            if (ferror(src))
            handle_error



          • fprintf(stdout), while technically valid, looks strange. A printf suffices.


          • cpy_buf doesn't belong to main. Define it directly in cpy.


          • Prefer passing file names via command line arguments.


          • The "Aborting..." message doesn't have a terminating newline, and the next prompt will be printed on the same line (on any shell except cmd.exe). Make a habit to print a newline at the end of the message.






          share|improve this answer









          $endgroup$















            11












            11








            11





            $begingroup$


            • You test the return value of fwrite, which is good. However, fread may fail as well. Since fread doesn't distinguish error from end of file, you should call ferror:



               while ((num_elements = fread(....)) > 0) 
              ....

              if (ferror(src))
              handle_error



            • fprintf(stdout), while technically valid, looks strange. A printf suffices.


            • cpy_buf doesn't belong to main. Define it directly in cpy.


            • Prefer passing file names via command line arguments.


            • The "Aborting..." message doesn't have a terminating newline, and the next prompt will be printed on the same line (on any shell except cmd.exe). Make a habit to print a newline at the end of the message.






            share|improve this answer









            $endgroup$




            • You test the return value of fwrite, which is good. However, fread may fail as well. Since fread doesn't distinguish error from end of file, you should call ferror:



               while ((num_elements = fread(....)) > 0) 
              ....

              if (ferror(src))
              handle_error



            • fprintf(stdout), while technically valid, looks strange. A printf suffices.


            • cpy_buf doesn't belong to main. Define it directly in cpy.


            • Prefer passing file names via command line arguments.


            • The "Aborting..." message doesn't have a terminating newline, and the next prompt will be printed on the same line (on any shell except cmd.exe). Make a habit to print a newline at the end of the message.







            share|improve this answer












            share|improve this answer



            share|improve this answer










            answered Feb 10 at 18:09









            vnpvnp

            40k232102




            40k232102





















                5












                $begingroup$

                Regarding these kinds of statements:



                fprintf(stderr, "n%s is not a valid file or permission deniedn", src);


                When the error indication is from a C library function, we should call



                perror( "my error message" );


                That will output to stderr both your error message AND the text reason the system thinks the error occurred.






                share|improve this answer











                $endgroup$

















                  5












                  $begingroup$

                  Regarding these kinds of statements:



                  fprintf(stderr, "n%s is not a valid file or permission deniedn", src);


                  When the error indication is from a C library function, we should call



                  perror( "my error message" );


                  That will output to stderr both your error message AND the text reason the system thinks the error occurred.






                  share|improve this answer











                  $endgroup$















                    5












                    5








                    5





                    $begingroup$

                    Regarding these kinds of statements:



                    fprintf(stderr, "n%s is not a valid file or permission deniedn", src);


                    When the error indication is from a C library function, we should call



                    perror( "my error message" );


                    That will output to stderr both your error message AND the text reason the system thinks the error occurred.






                    share|improve this answer











                    $endgroup$



                    Regarding these kinds of statements:



                    fprintf(stderr, "n%s is not a valid file or permission deniedn", src);


                    When the error indication is from a C library function, we should call



                    perror( "my error message" );


                    That will output to stderr both your error message AND the text reason the system thinks the error occurred.







                    share|improve this answer














                    share|improve this answer



                    share|improve this answer








                    edited Feb 11 at 11:00









                    Toby Speight

                    25.6k742116




                    25.6k742116










                    answered Feb 10 at 17:36









                    user3629249user3629249

                    1,73759




                    1,73759





















                        4












                        $begingroup$

                        One additional issue is with sizeof(cpy_buf) in cpy. This will be the size of a pointer (usually 4 or 8 bytes) and not the size of the buffer that cpy_buf points to. If you follow the advice given in other answers to define cpy_buf within cpy, rathar than pass it in as a parameter, this won't be a problem.



                        Also a small read/write buffer is inefficient. You should use at least 512 bytes, and preferably something much larger (like 2048 or 16384 bytes).






                        share|improve this answer









                        $endgroup$

















                          4












                          $begingroup$

                          One additional issue is with sizeof(cpy_buf) in cpy. This will be the size of a pointer (usually 4 or 8 bytes) and not the size of the buffer that cpy_buf points to. If you follow the advice given in other answers to define cpy_buf within cpy, rathar than pass it in as a parameter, this won't be a problem.



                          Also a small read/write buffer is inefficient. You should use at least 512 bytes, and preferably something much larger (like 2048 or 16384 bytes).






                          share|improve this answer









                          $endgroup$















                            4












                            4








                            4





                            $begingroup$

                            One additional issue is with sizeof(cpy_buf) in cpy. This will be the size of a pointer (usually 4 or 8 bytes) and not the size of the buffer that cpy_buf points to. If you follow the advice given in other answers to define cpy_buf within cpy, rathar than pass it in as a parameter, this won't be a problem.



                            Also a small read/write buffer is inefficient. You should use at least 512 bytes, and preferably something much larger (like 2048 or 16384 bytes).






                            share|improve this answer









                            $endgroup$



                            One additional issue is with sizeof(cpy_buf) in cpy. This will be the size of a pointer (usually 4 or 8 bytes) and not the size of the buffer that cpy_buf points to. If you follow the advice given in other answers to define cpy_buf within cpy, rathar than pass it in as a parameter, this won't be a problem.



                            Also a small read/write buffer is inefficient. You should use at least 512 bytes, and preferably something much larger (like 2048 or 16384 bytes).







                            share|improve this answer












                            share|improve this answer



                            share|improve this answer










                            answered Feb 11 at 2:02









                            1201ProgramAlarm1201ProgramAlarm

                            3,2832923




                            3,2832923





















                                1












                                $begingroup$

                                In your functions, you're outputting a string showing an error occurred, then returning 0, which usually means "Success!". Then in your main() function, you're checking whether or not the functions succeed or fail. Have you tested the return codes and whether you're getting the correct return values?






                                share|improve this answer









                                $endgroup$












                                • $begingroup$
                                  This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
                                  $endgroup$
                                  – SMC
                                  Feb 12 at 16:49










                                • $begingroup$
                                  And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
                                  $endgroup$
                                  – Canadian Luke
                                  Feb 12 at 17:23















                                1












                                $begingroup$

                                In your functions, you're outputting a string showing an error occurred, then returning 0, which usually means "Success!". Then in your main() function, you're checking whether or not the functions succeed or fail. Have you tested the return codes and whether you're getting the correct return values?






                                share|improve this answer









                                $endgroup$












                                • $begingroup$
                                  This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
                                  $endgroup$
                                  – SMC
                                  Feb 12 at 16:49










                                • $begingroup$
                                  And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
                                  $endgroup$
                                  – Canadian Luke
                                  Feb 12 at 17:23













                                1












                                1








                                1





                                $begingroup$

                                In your functions, you're outputting a string showing an error occurred, then returning 0, which usually means "Success!". Then in your main() function, you're checking whether or not the functions succeed or fail. Have you tested the return codes and whether you're getting the correct return values?






                                share|improve this answer









                                $endgroup$



                                In your functions, you're outputting a string showing an error occurred, then returning 0, which usually means "Success!". Then in your main() function, you're checking whether or not the functions succeed or fail. Have you tested the return codes and whether you're getting the correct return values?







                                share|improve this answer












                                share|improve this answer



                                share|improve this answer










                                answered Feb 11 at 3:16









                                Canadian LukeCanadian Luke

                                4811418




                                4811418











                                • $begingroup$
                                  This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
                                  $endgroup$
                                  – SMC
                                  Feb 12 at 16:49










                                • $begingroup$
                                  And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
                                  $endgroup$
                                  – Canadian Luke
                                  Feb 12 at 17:23
















                                • $begingroup$
                                  This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
                                  $endgroup$
                                  – SMC
                                  Feb 12 at 16:49










                                • $begingroup$
                                  And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
                                  $endgroup$
                                  – Canadian Luke
                                  Feb 12 at 17:23















                                $begingroup$
                                This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
                                $endgroup$
                                – SMC
                                Feb 12 at 16:49




                                $begingroup$
                                This is probably one of those java things. In my mind 0 = false = failed but I know that the opposite is true for main exit codes since you can return specific ints to indicate what type of error occurred. It doesn't confuse me but that's why I'm asking for reviews from experienced C devs
                                $endgroup$
                                – SMC
                                Feb 12 at 16:49












                                $begingroup$
                                And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
                                $endgroup$
                                – Canadian Luke
                                Feb 12 at 17:23




                                $begingroup$
                                And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.
                                $endgroup$
                                – Canadian Luke
                                Feb 12 at 17:23

















                                draft saved

                                draft discarded
















































                                Thanks for contributing an answer to Code Review Stack Exchange!


                                • Please be sure to answer the question. Provide details and share your research!

                                But avoid


                                • Asking for help, clarification, or responding to other answers.

                                • Making statements based on opinion; back them up with references or personal experience.

                                Use MathJax to format equations. MathJax reference.


                                To learn more, see our tips on writing great answers.




                                draft saved


                                draft discarded














                                StackExchange.ready(
                                function ()
                                StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f213193%2fbasic-c-copy-file-implementation%23new-answer', 'question_page');

                                );

                                Post as a guest















                                Required, but never shown





















































                                Required, but never shown














                                Required, but never shown












                                Required, but never shown







                                Required, but never shown

































                                Required, but never shown














                                Required, but never shown












                                Required, but never shown







                                Required, but never shown






                                Popular posts from this blog

                                How to check contact read email or not when send email to Individual?

                                Displaying single band from multi-band raster using QGIS

                                How many registers does an x86_64 CPU actually have?