Skip to main content
Code Review

Return to Answer

deleted 1 character in body
Source Link
Heslacher
Heslacher
  • 50.4k
  • 5
  • 81
  • 175

There is almost no error checking or handling. It's not hard to haddadd, and it makes the program much more robust. Here's how the start of main might look:

There is almost no error checking or handling. It's not hard to hadd, and it makes the program much more robust. Here's how the start of main might look:

There is almost no error checking or handling. It's not hard to add, and it makes the program much more robust. Here's how the start of main might look:

added example of error handling
Source Link
Edward
Edward
  • 66.6k
  • 4
  • 118
  • 282

There is almost no error checking or handling. It's not hard to hadd, and it makes the program much more robust. Here's how the start of main might look:

int main(int argc, char *argv[]) {
    if (argc != 2) {
        puts("Usage: longword filename");
        return 0;
    }
    FILE* fp;
    fp = fopen(argv[1], "rb");

    if (!fp) {
        perror("couldn't open words file");
        return 3;
    }
    size_t length = 5000000;
    char *buffer = malloc(length);
    if (buffer == NULL) {
        perror("couldn't allocate memory");
        return 2;
    }
    length = fread(buffer, 1, length, fp);
    if (ferror(fp)) {
        perror("couldn't read file");
        free(buffer);
        return 1;
    }
    // rest of program here
    free(buffer);
}

There is almost no error checking or handling. It's not hard to hadd, and it makes the program much more robust.

There is almost no error checking or handling. It's not hard to hadd, and it makes the program much more robust. Here's how the start of main might look:

int main(int argc, char *argv[]) {
    if (argc != 2) {
        puts("Usage: longword filename");
        return 0;
    }
    FILE* fp;
    fp = fopen(argv[1], "rb");

    if (!fp) {
        perror("couldn't open words file");
        return 3;
    }
    size_t length = 5000000;
    char *buffer = malloc(length);
    if (buffer == NULL) {
        perror("couldn't allocate memory");
        return 2;
    }
    length = fread(buffer, 1, length, fp);
    if (ferror(fp)) {
        perror("couldn't read file");
        free(buffer);
        return 1;
    }
    // rest of program here
    free(buffer);
}
Source Link
Edward
Edward
  • 66.6k
  • 4
  • 118
  • 282

The code is not bad as it stands, but I think there are some things that could be improved.

Think of the user

The input file name and unwanted letters are all hardcoded at the moment. It would be nice if the user could specify one or both of these parameters on the command line.

Add error handling

There is almost no error checking or handling. It's not hard to hadd, and it makes the program much more robust.

Consider using standard library functions

At a very small performance penalty (as measured on my machine), one could write a very clean version using only standard functions:

char *longest = NULL;
int longestlen = 0;
char *word = strtok(buffer, "\n");
while (word) {
    const int len = strlen(word);
    if (len > longestlen) {
        if (strpbrk(word, "gkmqvwxzio") == NULL) { 
            longestlen = strlen(word);
            longest = word;
        }
    }
    word = strtok(NULL, "\n");
}
printf("%s\n", longest);

That is the way I'd probably write it unless there were some compelling reason that's not fast enough.

Use functions

Your is_legal function is not bad, but I'd also write a get_word_len function to fetch the length of the next word in the buffer.

static inline int get_word_len(const char *buff, const char *end) {
    int len = 0;
    for ( ; *buff != '\n' && buff < end; ++buff, ++len) 
    {}
    return len;
}

Use const where practical

The is_legal function doesn't alter the passed string, so that parameter should be const.

Think carefully about the problem

The current code might print the word followed by \n, but if the words doesn't happen to be the first in the file, it will also print the \n from the previous word. It's not necessarily wrong, but it's not consistent.

Use bool for boolean values

The implmentation of bool is in <stdbool.h> and should be used as the return type of is_legal.

Use only the required headers

In this program neither <stddef.h> nor <unistd.h> appear to be needed; I'd recommend omitting them and only including headers that are actually needed.

Consider using pointers

There may not be a performance difference in this case, but for problems like these, the use of pointers seems more natural to me. For example:

const char *end = buffer + length;
const char *longest = buffer;
int longestlen = 0;

for (const char *curr=buffer; curr < end;  ) {
    const int wordlen = get_word_len(curr, end);
    if (wordlen > longestlen) {
        if (is_good_word(curr, wordlen)) {
            longestlen = wordlen;
            longest = curr;
        }
    }
    curr += wordlen + 1;
}
printf("%.*s\n", longestlen, longest);

Here, is_good_word is like your is_legal function:

static inline bool is_good_word(const char *curr, int wordlen) {
  static const char* bad = "gkmqvwxzio";
  for ( ; wordlen; --wordlen) {
      char ch = tolower(*curr++);
      for (const char *badptr = bad; *badptr; ++badptr) {
          if (ch == *badptr) {
              return false;
          }
      }
  }
  return true;
}

Don't leak memory

The program allocates but does not free the buffer space. Yes, the operating system will clean up after you, but a free costs very little and allows for better memory leak checking with tools like valgrind.