Use the C standard library
Your code could be greatly simplified if you used the C standard library functions fopen
, fclose
, fread
, fwrite
and fprintf
instead of the low-level POSIX system calls. The standard libraray also provides strlen
which obsoletes your ft_len
function but if you use the functions mentioned earlier, you won't have to count the length of a string any more anyway. If these standard library functions are verboten in your assignment, please state this in your question.
Be prepared for read
and write
to return early
read
and write
are not guaranteed to read or write all bytes they were asked to. The proper way to use them is therefore to call them in a loop until either all bytes are read / written or an error is encountered.
Be prepared for read
and write
to fail
read
and write
may fail in which case they return
a negative value and set errno
. You should check this and act upon the error appropriately.
Be prepared for files larger than 4095 bytes
You only call read
a single time per file. The concerns about read
returning early aside, what if the file is larger than your buffer?
Get rid of useless NUL termination
You stick a '\0'
byte at the end of your buffer but nothing expects this. You're already passing around an explicit length (which is good, because the buffer may contain embedded NUL bytes). And if you do it anyway, it should be buf[size] = '\0';
.
Don't guess the reason for an error
If you cannot open a file, you always report “No such file or directory”. But that isn't the only reason a call to open
can fail. You should inspect the value of errno
to find out why the operation failed. If you cannot find out the reason, reporting a generic error like “Cannot open file … for reading” is still better than guessing a potentially wrong reason. Reporting a wrong reason is worse than reporting no reason at all because it will mislead your users.
#include
all required headers
I don't see any #include
s of system headers in your code. Your code depends on the following headers.
<unistd.h>
for open
, read
, write
and close
<sys/stat.h>
and <fcntl.h>
for the respective flags
In case you have #include
d them in the header file you didn't show: Don't do that. What functions are needed in the implementation is an implementation detail. You headers should only expose the public API of your module.
Use proper types
The length of an array should be measured in size_t
(defined in <stddef.h>
) instead of int
. The return
type of read
and write
is ssize_t
.
Avoid uninitialized variables
It's not the 1980's any more where we had to write code like this.
int i;
// …
i = 1;
// …
while (i < files)
{
// …
++i;
}
Instead, we can write this.
for (int i = 1; i < files; ++i)
{
// …
}
The more you limit the scope of a variable, the lesser the risk for accidentally misusing it.
The variables size
and fd
could be declared only inside the body of the loop. fd
could be declared const
then.
const int fd = open( … );
It would avoid certain mistakes and you wouldn't be tempted to initialize it with a meaningless value.
Avoid magic numbers
Your choice for a buffer size of 4096 is reasonable but still arbitrary. One day, you might decide to chose a different size. Yet, the constant – or, even worse, constants derived from it, which you cannot search for – is repeated several times in the code. Instead of hard-coding such magic numbers, use a named constant.
#define BUFFER_SIZE 4096
char buffer[BUFFER_SIZE];
const ssize_t count = read(fd, buffer, BUFFER_SIZE - 1);
You could also use sizeof(buffer)
.
Since C now supports variable-length arrays, you could also use a const
variable instead of a macro #define
.
Subtracting 1 isn't really needed because NUL termination isn't needed as mentioned above. But when you do need to derive a size, using an expression like this automatically updates the dependent value if the value it depends on changes.
The line
ft_writeln(1, " No such file or directory", 26);
is another example that is waiting for somebody to alter the text and forgetting to update the number. It's not even clear what the 26 means immediately.
In order to further reduce the amount of magic constants, consider using the macros STDOUT_FILENO
, STDERR_FILENO
and STDIN_FILENO
, which are #define
d in <unistd.h>
to 1, 2 and 0 respectively. The benefit is not so much that these constants would change any time soon or people don't know what file descriptor 1 refers to but that in a function call
print(1, "hello, world\n");
the 1
could mean almost anything whereas in
print(STDOUT_FILENO, "hello, world\n");
the meaning of the first argument is much clearer.
Print error messages to standard error output
Programs should output their results to standard output and any logging, warnings or error messages to standard error output. One reason for this is that standard output is often redirected. If I execute
$ cat bar.c baz.c > foo.c
all standard output will go to foo.c
but I still want to see the error messages. (And even if I don't want to see them, I certainly don't want them to end up in foo.c
.)
Separate concerns
The arguments passed to main
include the program name as argv[0]
. This is fine but your ft_read
function really shouldn't care where its arguments came from. Instead, the call in main
should only pass it the relevant arguments
ft_read(argc - 1, argv + 1);
and ft_read
should consider all elements (starting with index 0) of the array as file names to operate on.
Report errors all the way back
If your program fails at any point, it should report this back via its exit status. Therefore, if you encounter an error you cannot recover from, main
should return
EXIT_FAILURE
(which is #define
d in <stdlib.h>
) or a small positive integer.
Structure the code in the way that minimizes surprises
You have separated the implementation logic from the file that contains the main
function. This is good because it allows unit testing and code re-use. But there isn't really a good reason to put the ft_writeln
function into a separate file. Actually, why do you have this function at all?
Declare functions static
when they need not be externally visible
If a function is only used in the translation unit (the *.c
file) that defines it, declare it static
. This will make it inaccessible from other translation units and therefore improve encapsulation and avoid name-space pollution. It might also help increase performance.
Use names that properly reflect what a function is doing
ft_writeln
sounds as if the function would write a line. But it actually doesn't care about lines. It simply writes out bytes.
Don't confuse return
with a function
The return
statement is not a function call. You don't need parenthesis around the value that is to be return
ed.
Use proper indentation
Your code is formatted correctly most of the time. But at some points, you forgot to indent the body of a loop or misplaced braces. There is no point in engaging in a flame war about how many spaces are the correct choice but there can be no dispute that you should use some indentation and use the same style consistently. A good editor will take care of this for you automatically.
Read the documentation for cat
If your goal is to be compatible to POSIX cat
, read its documentation to find out what it does exactly. Implementing all of its options might be too much work for now but an interesting aspect of its behavior is currently not reflected by your version. If cat
is given zero arguments, it reads from standard input instead. Your program simply does nothing in this case.