Linux cat command in C

Question

I wrote my own implementation of the linux cat command in C for my computer laboratory class. We were asked to replicate its functionality with no options passed as arguments or with just the -b, -n and -s options.

I'm posting the code here because, although it works fine, i feel a little bit insecure about. Firstly because i think i "over-coded" it a bit, specially on the option handling part, and also, because i'm still not very confortable working with pointers and so i am not very sure if my approach does not generate memory leaks or malfunctions.

Here it is:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <ctype.h>
#define BUF_SIZE 1024

int readStdin(int index, int bflag, int nflag){
  char buffer[BUF_SIZE];
  while(fgets(buffer, BUF_SIZE, stdin)){ //reads from the standard input and prints the input
    if (nflag){
      printf("     %d  %s", index, buffer);
      index++;
    }
    if (bflag){
      if (*buffer == '\n'){
        printf ("%s", buffer);
      }
      else{
        printf ("     %d  %s", index, buffer);
        index++;
      }
    }
    else {
      printf("%s", buffer);
    }
  }
  return index; //returns the incremented index to perpetuate its use
}

int readFile(char* filename, FILE* fp, int index, int bflag, int nflag){
  char ch;
  char s[BUF_SIZE];
  if (fp==NULL){ //in case the file doesn't exist
    printf("%s: No such file or directory\n", filename);
    exit(1);
  }

  if (bflag){
    while ((fgets(s,BUF_SIZE,fp))){
      if (strcmp(s,"\n") == 0){
        printf (" %s", s);
      }
      else {
        printf ("     %d  %s", index, s);
        index++;
      }
    }
    fclose(fp);
  }
  if (nflag){
    while ((fgets(s,BUF_SIZE,fp))){
      printf ("     %d  %s", index, s);
      index++;
    }
    fclose(fp);
  }
  else{
    while ((ch=fgetc(fp)) != EOF){ //printing loop
      putchar(ch);
    }
    fclose(fp);
  }
  return index;
}

void readArgs(int argc, char* argv[]){
  FILE* fp;
  int index = 1; //line index. to be used in case -b or -n is passed as an argument
  int option; //option passed as argument
  int bflag = 0; //-b option deactivated by default
  int nflag = 0; //-n option deactivated by default
  opterr = 0; //deactivates getopt's default error messages

  //checks if there are options passed as argument and updates their flags
  while ((option = getopt(argc, argv, "bn")) != -1){
    switch (option){
      case 'b':
              bflag = 1;
              break;
      case 'n':
              nflag = 1;
              break;
      case '?': //in case there was some problem
              exit(1);
    }
  }

  if (bflag == 1 && nflag == 1){ //if -b and -n are passed as argument, b overrides n
    nflag = 0;
  }

  for (int i=optind; i<argc; i++){
    if (*argv[i] == '-'){ //in case of '-' in argv[i], reads from stdin and prints
      index = readStdin(index, bflag, nflag);
      clearerr(stdin);
    }
    else { //prints the contents of the file in *argv[i]
      fp = fopen(argv[i], "r");
      index = readFile(argv[i], fp, index, bflag, nflag);
    }
  }
}

int main(int argc, char* argv[]){
  if (argc<2){ //if there are no arguments
    readStdin(1,0,0);
    return 0;
  }
  readArgs(argc, argv); //otherwise
  return 0;
}

I didn't implement the -s option because i cant think of a way to handle it at the same time as the other options. That's why i think that the way i am handling the options part is a bit inefficient.

Any suggestions?

Answer 1

Use Standard Constants When You Can

The stdio.h header file contains a definition of BUFSIZ. This is a good constant to use for input and output buffers.

In the stdlib.h header file are 2 constants that are useful when calling exit or returning from main, these are EXIT_FAILURE and EXIT_SUCCESS. These constants are universal, even if the underlying operating system expects different numbers for success or failure this constants will provide the correct value.

DRY Code
There is a programming principle called the Don't Repeat Yourself principle. The principle is this, if you find yourself repeating code, it might be better to create a function or sub-program and call that function where the code is repeated. This reduces the amount of code in a module and makes it easier to read. What is more beneficial than the readability is the fact that you only have to write and debug that code once. Another benefit of puting repeating code into a function is that if the code needs to be changed, there is only one place to change it. .

Code that repeats a lot in this program:

    printf("     %d  %s", index, buffer);
    index++;

While this is only one line of code, making a function out of it reduces the possibility of errors such as missing spaces or too many spaces.

All of the output could be in their own functions:

void formattedLineOut(int *index, char buffer[])
{
    printf("     %d  %s", *index, buffer);
    *index++;
}

The function formattedLineOut is receiving a reference to index rather than the value. This way the function doesn't have to have a return value for index.

void bprint(int *index, char buffer[])
{
    if (strcmp(s, "\n") == 0) {
        printf(" %s", s);
    }
    else {
        formattedLineOut(index, s);
    }
}

void outputLine(int *index, char buffer[], int nflag, int bflag)
{
    if (nflag)
    {
        formattedLineOut(index, buffer);
    }
    else if (bflag)
    {
        bprint(index, buffer);
    }
    else
    {
        printf("%s", buffer);
    }
}

int readFile(char* filename, FILE* fp, int index, int bflag, int nflag) {
    char ch;
    char s[BUF_SIZE];
    if (fp == NULL) { //in case the file doesn't exist
        printf("%s: No such file or directory\n", filename);
        exit(1);
    }

    while ((fgets(s, BUF_SIZE, fp))) {
        outputLine(&index, s, bflag, nflag)
    }

    return index;
}

Match fclose() With fopen()

The function readFile has multiple calls to fclose(). This can lead to bugs, it might be better to call fclose() after readFile returns in the calling function. This will also allow readFile to handle stdin, since stdin is a file pointer.