Basic C copy file implementation

Question

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, "\r\n")] = 0;
    fprintf(stdout, "\nPlease enter dest path:\n");
    fgets(dest, buf_size, stdin);
    dest[strcspn(dest, "\r\n")] = 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 denied\n", 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 permissions\n", 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;
}

Answer 1

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().

Answer 2

• 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.

Answer 3

Regarding these kinds of statements:

fprintf(stderr, "\n%s is not a valid file or permission denied\n", 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.

Answer 4

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).

Answer 5

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?