6
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Are you allowed to use Linux system calls like read(2) and write(2), or are you forced to use the C standard functions? \$\endgroup\$ – esote Apr 7 at 23:41
  • 1
    \$\begingroup\$ @esote there are no objections to that. I might take a look into it \$\endgroup\$ – userUSeruser Apr 8 at 0:01
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ i'm having a problem with the index pointer. it is not updating its value. how can i approach this? \$\endgroup\$ – userUSeruser Apr 8 at 14:51
  • 1
    \$\begingroup\$ @GuilhermeLima try (*index)++; \$\endgroup\$ – pacmaninbw Apr 8 at 15:16
  • \$\begingroup\$ oh right, i forgot the parentheses. another thing, it is no longer reading from stdin if i pass just an option (or more) and nothing else after that. (for example, ./my_cat -n). how can i approach this? i'm really scratching my head \$\endgroup\$ – userUSeruser Apr 8 at 15:18
  • \$\begingroup\$ If you still need to hand in your homework I can't help you, it is beyond the scope of this website. Hand in what you had working. I also can't answer without seeing the code. Since it is broken you might want to post it on stackoverflow.com. \$\endgroup\$ – pacmaninbw Apr 8 at 15:33

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.