Skip to main content
edited tags
Link
200_success
  • 144.2k
  • 22
  • 188
  • 473
Source Link

Finding the longest word without these characters follow-up

Here I asked this question:

My goal is to go through the list of all English words (separated by '\n' characters) and find the longest word which doesn't have any of these characters: "gkmqvwxz". And I want to optimize it as much as possible

I updated the code with the help of suggestions from answers, but I still need comments on this updated version

Changes:

  1. Name of the file and the forbidden characters are no longer hard-coded. They are passed by arguments.
  2. Added several error checks.
  3. Used pointers instead of indexes.
  4. buffer is freed when we're done with it.
  5. Used bool instead of int for the return type of is_legal.
  6. Parameters to is_legal are made const since we don't change them.
  7. Skip next lines ('\n') remaining from previous lines.
  8. Added some functions to keep main simple.
  9. Removed superfluous headers (#include <string.h>, #include <stddef.h>, #include <unistd.h>).
  10. is_legal need not know about the entire buffer. Just the relevant pointers are now sent.
  11. length is no longer fixed. We get the size of the array at runtime
  12. buffer is terminated with null.

Updated code:

#include <ctype.h>
#include <stdlib.h>
#include <stdio.h>
#include <stdbool.h>


static inline bool is_legal(const char* beg, size_t size, const char* bad)
{
    for (; size-- !=0 ; ++beg) {                                 /* go through current word */
        char ch = tolower(*beg);                                /* The char might be upper case */
        for (const char* bad_ptr = bad; *bad_ptr; ++bad_ptr)
            if (ch == *bad_ptr)                                 /* If it is found, return false */
                return false;
    }

    return true;                                                /* else return true */
}

static inline size_t get_next_word_size(const char* beg)
{
    size_t size = 0; /* resulting size */
    for (; beg[size] && beg[size] != '\n'; ++size) /* read the next word */
    { } /* for loop doesn't have a body */
    return size;
}

static inline char* get_buffer(const char* filename)
{
    char *buffer = NULL;                     /* contents of the text file */

    size_t length;                 /* maximum size */
    FILE* fp;
    fp = fopen(filename, "rb");

    if (!fp) {                               /* checking if file is properly opened */
        perror("Couldn't open the file\n");
        return NULL;
    }

    if (fseek(fp, 0, SEEK_END)) {
        perror("Failed reading");
        return NULL;
    }

    length = ftell(fp);

    if (fseek(fp, 0, SEEK_SET)) {
        perror("Failed reading");
        return NULL;
    }

    buffer = malloc(length + 1); /* +1 for null terminator */

    if (buffer == NULL) {                   /* checking if memory is allocated properly */
        perror("Failed to allocate memory\n");
        free(buffer);
        return NULL;
    }

    fread(buffer, 1, length, fp);           /* read it all */
    fclose(fp);

    buffer[length] = '\0';                  /* terminate the string with null*/
    return buffer;
}


int main(int argc, char **argv)
{
    if (argc < 3) {
        printf("Usage: FileName BadChars");
        return 0;
    }

    char* filename = argv[1];
    char* badchars = argv[2];

    char *buffer = get_buffer(filename);

    if (buffer == NULL) {
        return -1;
    }

    const char *beg = buffer;               /* current word boundaries */
    size_t size = 0;

    const char *mbeg = beg;                 /* result word */
    size_t msize = 0;

    while (beg[size]) {
        beg += size + 1;                 /* +1 to skip the '\n' */
        size = get_next_word_size(beg);  /* get the size of the next word */

        if (size > msize && is_legal(beg, size, badchars)) { /* if it is a fit, save it */
            mbeg = beg;
            msize = size;
        }
    }

    printf("%.*s\n", msize, mbeg);  /* print the output */

    free(buffer);
    return 0;
}

I would especially appreciate comments regarding the way the code reads the entire file into a single dynamically allocated array. About if and how it could be improved. I wouldn't like to sacrifice the performance, but some "best practices" especially about this part are very welcome.