10
\$\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, "\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;
}
\$\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, "\r\n")] = 0;. \$\endgroup\$ – chux Feb 14 at 4:41
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().

\$\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
12
\$\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.

\$\endgroup\$
5
\$\begingroup\$

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.

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

\$\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?

\$\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

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

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