11
\$\begingroup\$

I have a school project to create a program that works the same as the Linux cat. The code works, but I just need find out where I can Improve it or handle errors. The program has 3 .c files and 1 header file. I'm also not allowed to use system functions; I have to use read(), close(), open() and write() and while loop only.

ft_read.c

#include "ft_read_file.h"

 int    ft_len(char *str)
 {
     int    i;

     i = 0;
     while (str[i] != '\0')
     i++;
     return (i);
  }

  void  ft_error(char *file_name)
  {
       ft_writeln(1, file_name, ft_len(file_name));               
       ft_writeln(1, ": ", 2);
       ft_writeln(1, file_name, ft_len(file_name));
       ft_writeln(1, ":", 1);
       ft_writeln(1, " No such file or directory", 26);
       ft_writeln(1, ".\n", 2);
   }

   int  ft_read(int files, char **file_name)
   {
       int  fd;
       int  i;
       char buf[4096];
       int  size;

       fd = 1;
       i = 1;
       size = 0;
       while (i < files)
       {
           fd = open(file_name[i], O_RDONLY);
           if (fd == -1)
           {
               ft_error(file_name[i]);
                return (1);
            }
             size = read(fd, buf, 4095);
             buf[4095] = '\0';
             ft_writeln(1, buf, size);
             close(fd);
             i++;
          }
          return (0);
     }

main.c

#include "ft_read_file.h"

int main(int argc, char **argv)
{
    if (argc >= 1)
        ft_read(argc, argv);
    return (0);
 }

ft_writeln.c

#include "ft_read_file.h"

void    ft_writeln(int fd, void *buf, int size)
{
       write(fd, buf, size);
}
\$\endgroup\$
8
\$\begingroup\$

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 #includes 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 #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 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 #defined 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 #defined 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 returned.

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you, I forgot to mention that I'm not allowed to use system functions, I have to use read(), close(), open() and write() and whileloop only. \$\endgroup\$ – Junius L. Apr 15 '16 at 6:42
  • \$\begingroup\$ @TenTen Fair enough then. Pedantic note: These actually are system functions. It seems you're not allowed to use standard library functions. \$\endgroup\$ – 5gon12eder Apr 15 '16 at 6:59

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.