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)

\$\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\$
– user207421Feb 11 at 8:56

5 Answers
5

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

\$\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\$
– SMCFeb 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\$
– pacmaninbwFeb 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 IlligFeb 11 at 10:16

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.

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

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?

\$\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\$
– SMCFeb 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 LukeFeb 12 at 17:23