Use the C standard library
Your code could be greatly simplified if you used the C standard library functions
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
write to return early
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
write to fail
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
#includes of system headers in your code. Your code depends on the following headers.
<fcntl.h> for the respective flags
In case you have
#included 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
return type of
Avoid uninitialized variables
It's not the 1980's any more where we had to write code like this.
i = 1;
while (i < files)
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.
fd could be declared only inside the body of the loop.
fd could be declared
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
const ssize_t count = read(fd, buffer, BUFFER_SIZE - 1);
You could also use
Since C now supports variable-length arrays, you could also use a
const variable instead of a macro
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.
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
STDIN_FILENO, which are
<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");
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
The arguments passed to
main include the program name as
argv. 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);
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,
EXIT_FAILURE (which is
<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?
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.
return with a function
return statement is not a function call. You don't need parenthesis around the value that is to be
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
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.