Gfrktyl

I'm working on my implementaion of the linux wc command. I finally have something that is working properly (with no options yet) but i think it needs a lot of "cleaning". I mean, i highly disrespect the "do not repeat yourself rule" and i want to hear some ideas on how can i polish this up a bit.

Here 's the code:

#include <stdio.h>

#include <stdlib.h>

#include <string.h>

#include <unistd.h>

#include <getopt.h>

#define BUF_SIZE 1024



typedef struct Total{ //stores the Total count of lines words and bytes

  int lines;

  int words;

  int bytes;

} Total;



Total* new_Total(int lines, int words, int bytes){ //constructor function for Total

  Total* t = (Total*)malloc(sizeof(Total));

  t->lines = lines;

  t->words = words;

  t->bytes = bytes;

  return t;

}



void updateTotal(Total *t, int lines, int words, int bytes){ //updates Total counters

  t->lines += lines;

  t->words += words;

  t->bytes += bytes;

}



void readStdin(Total* t, char c){ //reads from stdin, counts and prints

  int lines = 0, words = 0, bytes = 0, startWord = 0;

  char ch;

  while((ch=fgetc(stdin)) != EOF){//count the bytes, lines and words

    bytes++;

    if (ch == 'n'){

      lines++;

    }

    if (ch != ' ' && ch != 'n' && ch != 'r' && ch != 't'){ //if there's a visible char, there's a word

      startWord = 1;

    }

    if ((ch == ' ' || ch == 'n' || ch == 'r' || ch == 't') && startWord == 1){ //when the word ends

      words++; //increment the word coutner

      startWord = 0; //reset word finder

    }

  }

  updateTotal(t, lines, words, bytes);

  if (c == ''){ //if the command was called with no arguments

    printf ("%d %d %dn", lines, words, bytes);

  }

  else{

    printf ("%d %d %d %cn", lines, words, bytes, c);

  }

}



void readFile(Total *t, char* filename, FILE* fp){

  int lines = 0, words = 0, bytes = 0, startWord = 0;

  char ch;

  if (fp == NULL){

    printf("%s: No such file or drectoryn", filename);

    exit(1);

  }

  while ((ch=fgetc(fp)) != EOF){ //count the bytes, lines and words

    bytes++;

    if (ch == 'n'){

      lines++;

    }

    if (ch != ' ' && ch != 'n' && ch != 'r' && ch != 't'){

      startWord = 1;

    }

    if ((ch == ' ' || ch == 'n' || ch == 'r' || ch == 't') && startWord == 1){

      words++;

      startWord = 0;

    }

  }

  updateTotal(t, lines, words, bytes);

  printf("%d %d %d %sn", lines, words, bytes, filename);

}



void readArgs(Total* t, int argc, char* argv){

  FILE* fp;

  for (int i=1; i<argc; i++){

    if (*argv[i] == '-'){//if a '-' is found, read from stdin

      readStdin(t, '-');

      clearerr(stdin);

    }

    else { //tries to read the file in argv[i]

      fp = fopen(argv[i], "r");

      readFile(t, argv[i], fp);

      fclose(fp);

    }

  }

  if (argc > 2){ //if there's more than one file, print the total in the end

    printf("%d %d %d totaln", t->lines, t->words, t->bytes);

  }

  else {

    exit(0);

  }

}



int main(int argc, char* argv){

  Total* t = new_Total(0,0,0);

  if (argc<2){ //no arguments

    readStdin(t,''); //pass /0 to readstin because as it is the only time it is called, there's no need to append the name of the argument

    return 0;

  }

  readArgs(t, argc, argv);

  free(t);

  return 0;

}

As you can see, both in readStdin() and in readFile() i have the same piece of code that handles the counting. I've tried to make a function out of this using pointers but i gave up. Any suggestion would be much appreciated. Also, if you have any hint that has nothing to do with the problem that i mentioned, shoot it up!

A standard way to do this is to break the function up into smaller 'private' (static) functions, each with one thing to do. One can pass the default to the counting function in case we are not provided the argument. This allows separation of the code which counts and the UI.

Total is always going to be called with (0, 0, 0), it's unnecessary to provide these arguments. In fact, the storage of Total in dynamic memory is probably unnecessary. No need to updateTotal, use of the counters directly is fine. In fact, there is going to be 1 total, might as well make it static.

fgetc returns an int, char ch is not enough to tell EOF, which is -1. You've defined BUF_SIZE, an excellent optimization, but never use it. I'd go from fgetc to fgets. Note that this assumes that the file is text and doesn't contain a '' inside, (eg, modified UTF-8.)

You've included getopt and unistd, making your code POSIX C99, but you're not using those. Removing them makes your code more general C99 (now your code will compile on more compilers.)

#include <stdio.h>

#include <stdlib.h>

#include <string.h>

#include <ctype.h>

#include <errno.h>



//stores the Total count of lines words and bytes

struct Total {

    size_t lines, words, bytes;

} total;



//count the bytes, lines and words

//gives incorrect results on binary files with embedded 0

//a word is delimited by everything that {isgraph}

//using the unix definition of a text file, {lines = count('n')}

static int readFile(FILE *fp) {

    char buffer[1024], b;

    int startWord = 1;

    size_t i;

    while(fgets(buffer, (int)sizeof buffer, fp)) {

        i = 0;

        do {

            b = buffer[i++];

            if(b == 'n') {

                /* This will not recognize 'l' in old Mac text files opened

                 with "wb"? Hmm. */

                total.lines++;

                startWord = 1;

            } else if(b == '') {

                break;

            } else if(isgraph(b)) {

                //if there's a visible char, there's a word

                if(startWord) total.words++, startWord = 0;

            } else {

                startWord = 1;

            }

        } while(1);

        total.bytes += i;

    }

    return ferror(fp) ? 0 : 1;

}



int main(int argc, char *argv) {

    FILE *fp = 0;

    char *fn = "<no file>";

    for(int i = 1; i < argc; i++) {

        fn = argv[i];

        fp = fopen(fn, "r");

        if(!fp || !readFile(fp)) break;

        fclose(fp), fp = 0;

    }

    if(argc <= 1) readFile(stdin);

    if(fp) fclose(fp);

    if(errno) return perror(fn), EXIT_FAILURE;

    /* https://stackoverflow.com/a/2524675/2472827 */

    printf("%zu lines; %zu words; %zu bytes total.n",

        total.lines, total.words, total.bytes);

    return EXIT_SUCCESS;

}

Some see typedef struct as problematic, and, in my opinion, it's not needed here; see Linux kernel coding style. INT_MAX could be as little as 32,767; it's never going to be less than zero, so I'd prefer unsigned, or size_t in this context. What if there are other chars besides 'n', ' ', 'r', 't'? I've included <ctype.h> to get rid of the ambiguity. There was a bug counting the word at the end of the line. Now, a line is defined as the number of 'n' (UNIX style.) A word is delimited by isgraph.

Well, i actually did it on my own so, if anyone's interested, here's the code:

#include <stdio.h>

#include <stdlib.h>

#include <string.h>

#include <unistd.h>

#include <getopt.h>

#define BUF_SIZE 1024



typedef struct Counter{ //stores the count of lines, words, bytes and the value of starword

  int lines;

  int words;

  int bytes;

  int wordFlag; //when counting words, if one is found, set to 1. when digested, reset.

} Counter;



Counter* new_Counter(int lines, int words, int bytes, int wordFlag){ //constructor function for Counter

  Counter* t = (Counter*)malloc(sizeof(Counter));

  t->lines = lines;

  t->words = words;

  t->bytes = bytes;

  t->wordFlag = wordFlag;

  return t;

}



void updateTotal(Counter *t, int lines, int words, int bytes){ //updates the total counter

  t->lines += lines;

  t->words += words;

  t->bytes += bytes;

}



void count(char ch, Counter* cnt){//counts the bytes, lines and words

  cnt->bytes++; //if a char is passed, count it

  if (ch == 'n'){ //count each line

    cnt->lines++;

  }

  if (ch != ' ' && ch != 'n' && ch != 'r' && ch != 't'){ //if there's a visible char, there's a word

    cnt->wordFlag = 1;

  }

  if ((ch == ' ' || ch == 'n' || ch == 'r' || ch == 't') && cnt->wordFlag == 1){ //when the word ends

    cnt->words++; //increment the word counter

    cnt->wordFlag = 0; //reset the flag

  }

}



void readStdin(Counter* t, char c){ //reads from stdin and prints

  Counter* cnt = new_Counter(0,0,0,0);

  char ch;

  while((ch=fgetc(stdin)) != EOF){

    count(ch, cnt);

  }

  updateTotal(t, cnt->lines, cnt->words, cnt->bytes);



  if (c == ''){ //if the command was called with no arguments

    printf ("%d %d %dn", cnt->lines, cnt->words, cnt->bytes);

  }

  else{

    printf ("%d %d %d %cn", cnt->lines, cnt->words, cnt->bytes, c);

  }

  free(cnt);

}



void readFile(Counter *t, char* filename, FILE* fp){

  Counter* cnt = new_Counter(0,0,0,0);

  char ch;

  if (fp == NULL){

    fprintf(stderr, "./my_wc: %s: No such file or directoryn", filename);

    exit(1);

  }

  while ((ch=fgetc(fp)) != EOF){ //count the bytes, lines and words

    count(ch, cnt);

  }

  updateTotal(t, cnt->lines, cnt->words, cnt->bytes);

  printf("%d %d %d %sn", cnt->lines, cnt->words, cnt->bytes, filename);

}



void readArgs(Counter* t, int argc, char* argv){

  FILE* fp;

  for (int i=1; i<argc; i++){

    if (*argv[i] == '-'){//if a '-' is found, read from stdin

      readStdin(t, '-');

      clearerr(stdin);

    }

    else { //tries to read the file in argv[i]

      fp = fopen(argv[i], "r");

      readFile(t, argv[i], fp);

      fclose(fp);

    }

  }

  if (argc > 2){ //if there's more than one file, print the Counter in the end

    printf("%d %d %d totaln", t->lines, t->words, t->bytes);

  }

  else {

    exit(0);

  }

}



int main(int argc, char* argv){

  Counter* t = new_Counter(0,0,0,0); //pointer to the total counter

  if (argc<2){ //no arguments

    readStdin(t,''); //pass /0 to readstin because as it is the only time it is called, there's no need to append the name of the argument

    return 0;

  }

  readArgs(t, argc, argv);

  free(t);

  return 0;

}

As you can see, i renamed the struct to Counter and i used it to store, not only the total counter, but also every temporary counter, then i implemented the function count() that does with the temporary counter values what those if statements in readStdin and readFile do.
If you still have any suggestions, please do not hesitate.