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:
- Name of the file and the forbidden characters are no longer hard-coded. They are passed by arguments.
- Added several error checks.
- Used pointers instead of indexes.
buffer
is freed when we're done with it.- Used
bool
instead ofint
for the return type ofis_legal
. - Parameters to
is_legal
are madeconst
since we don't change them. - Skip next lines (
'\n'
) remaining from previous lines. - Added some functions to keep main simple.
- Removed superfluous headers (
#include <string.h>
,#include <stddef.h>
,#include <unistd.h>
). is_legal
need not know about the entirebuffer
. Just the relevant pointers are now sent.length
is no longer fixed. We get the size of the array at runtimebuffer
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.