By SMC


2019-02-10 15:16:49 8 Comments

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;
}

5 comments

@user3629249 2019-02-10 17:36:21

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.

@Canadian Luke 2019-02-11 03:16:21

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?

@SMC 2019-02-12 16:49:16

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

@Canadian Luke 2019-02-12 17:23:57

And I've never programmed in Java, so I had no idea about that, lol. Glad it helped though.

@1201ProgramAlarm 2019-02-11 02:02:33

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

@pacmaninbw 2019-02-10 16:23:17

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

@SMC 2019-02-10 19:21:28

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?

@pacmaninbw 2019-02-10 19:44:07

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

@Roland Illig 2019-02-11 10:16:24

@pacmaninbw since open_files does not exit at all, using the EXIT_* constants is completely wrong for that function.

@vnp 2019-02-10 18:09:04

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

Related Questions

Sponsored Content

2 Answered Questions

[SOLVED] Copy certain lines from one text file to another

  • 2018-09-17 14:48:32
  • FabioEnne
  • 510 View
  • 6 Score
  • 2 Answer
  • Tags:   c# file

2 Answered Questions

[SOLVED] Function to copy a file using sendfile

  • 2018-07-20 01:07:49
  • H. Jang
  • 65 View
  • 0 Score
  • 2 Answer
  • Tags:   beginner c

1 Answered Questions

[SOLVED] Follow-up 2: Copy File, remove spaces in specific lines

0 Answered Questions

TI-Basic file manager

  • 2018-03-02 01:10:35
  • Solomon Ucko
  • 87 View
  • 2 Score
  • 0 Answer
  • Tags:   file ti-basic

1 Answered Questions

[SOLVED] Basic File Browse

2 Answered Questions

[SOLVED] Read file into char*

  • 2017-05-16 00:29:30
  • Conor O'Brien
  • 1352 View
  • 7 Score
  • 2 Answer
  • Tags:   c file

2 Answered Questions

[SOLVED] Follow-up 1: Copy File, remove spaces in specific lines

1 Answered Questions

[SOLVED] Copy File, remove spaces in specific lines

  • 2017-05-17 15:55:24
  • Isofruit
  • 345 View
  • 4 Score
  • 1 Answer
  • Tags:   java file

2 Answered Questions

[SOLVED] Basic shell implementation

  • 2016-01-17 12:20:21
  • jarr
  • 869 View
  • 3 Score
  • 2 Answer
  • Tags:   c linux shell

2 Answered Questions

[SOLVED] Very basic Java file reader/writer

Sponsored Content