3
\$\begingroup\$

I have to parse a custom configuration file with this structure:

# Begin with core containers.

ID          = 0 # STX container
Name        = "" # No name is necessary for a STX container.
Flags       = {STX}
Attributes  = {Status=0}

So I've written the following code in order to remove those comments (starting with #) and the inline comments. I don't know if I'm doing it the right way, it just seems (for me, not too much experience) a stupid way of doing it with many while loops.

void NextToken() {
   // Let's begin taking comments away!
   FILE* Source = fopen("defs.dat", "r");
   char* Buffer = calloc(MAXBUFLEN, sizeof(char));
   while (fgets(Buffer, MAXBUFLEN, Source)) {
    // If line starts with '#' it is a comment, so it must be ignored. If it's a newine it just has to be skipped.
    if (Buffer[0] == 35 || Buffer[0] == '\n')
        continue;
    // Now we have to delete inline comments
    int Pos = 0;
    while (Pos < strlen(Buffer)) {
        if(Buffer[Pos] == '#') {
            Pos++;
            while(Buffer[Pos] != '\n')  {
                Pos++;
                continue;
            }
            continue;
        }
        //Cool! now just let's print the output.
        printf("%c", Buffer[Pos++]);
    }
   }
}

The max size of the buffer is 256 characters.

\$\endgroup\$
2
\$\begingroup\$

I see some things that I think could help you improve your code.

Allow the user to specify input and output files

The input file name is currently hardcoded which certainly greatly restricts the usefulness of the program. Consider using argc and argv to allow the user to specify file name on the command line. Also, it would be nice to be able to send the output to a file.

Fix memory leaks

The program allocates but never frees memory, leading to a memory leak of MAXBUFLEN bytes every time NextToken() is called. If the value of MAXBUFLEN is really just 256, as suggested in the text, then it might be simplest to declare it on the stack instead and avoid explicit memory allocation.

Eliminate "magic numbers"

Instead of hard-coding the constant 35 in one place and '#' in another, it would be better to use a #define or const and name them.

Simplify the code

The code uses printf("%c",...) where it could simply use putchar() and eliminate the overhead of printf scanning the format string. Also the loops, with three continue statements are not very easy to follow. Here's what I'd propose instead:

void NextToken2(const char *filename) {
    FILE* Source = fopen(filename, "r");
    if (Source == NULL) {
        fprintf(stderr, "Couldn't open file \"%s\"\n", filename);
        return;
    }
    char Buffer[MAXBUFLEN];
    char *ptr;
    while (fgets(Buffer, MAXBUFLEN, Source)) {
        if ((ptr = strpbrk(Buffer, "#\n")) != NULL) {
            *ptr = '\0';
        }
        if (Buffer[0] != '\0') {
            puts(Buffer);
        }
    }
    fclose(Source);
}
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

You do not need calloc() with fgets(), it adds nul terminator by itself: use malloc() instead. Also check for allocation errors and, maybe, use a meaningful name for Buffer (for example line).

char* line= malloc(MAXBUFLEN, sizeof(char));
if (line == NULL) {
    // Oops...
}

Also fopen() may fail then you should also check its return value:

if (fopen(...) == NULL) {
    // Oops
}

You should move the check for a line to ignore to a separate self-explicative function, that's why you felt the need to add a comment:

if (IsBlankOrComment(line))
    continue;

Implementing IsBlankOrComment() you should avoid hard-coded constants like 35, you should add #define and use a character instead of its ASCII code:

#define COMMENT_DELIMITER '#'

bool IsBlankOrComment(char* line) {
    char* trimmedLine = TrimWhiteSpaces(line);

    return trimmedLine[0] != '\0'
        && trimmedLine[0] != COMMENT_DELIMITER;
}

For a possible implementation you can read this SO post. Do not forget to include stdbool.h if you're using bool (instead of _Bool).

You now have to remove in-line comments, what's better than strchr()? No need to manually loop through the string:

char* inlineCommentStart = strchr(line, COMMENT_DELIMITER);
if (inlineCommentStart != NULL) {
    *inlineCommentStart = '\0';
}

Note that it also deals with line with leading and trailing spaces, if you do not need it then you definitely should go with Edward's lovely implementation (which uses strpbrk()).

To print the line (without comment) you then just need:

puts(line);

No need to print character-by-character while searching the delimiter (BTW do not use printf() when what you need to print is just a single character!) Also note that calling strlen() in a loop is (probably) extremely inefficient because, if compile can't determine that string is constant in the loop, it will emit code to call strlen() for each iteration.

Do not forget to close the file when you're finished:

fclose(stream);

And to free allocated memory:

free(line);

In this case you should ask yourself IF you need dynamically allocated memory. If MAXBUFSIZE is a preprocessor constant then you can directly declare a local variable (check also for stack size issues):

char line[MAXBUFSIZE];

If your application isn't multi-threaded and you call this function more than once you may want to also consider to make it static. Finally do not forget to add const here and there, where appropriate.

| improve this answer | |
\$\endgroup\$

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.