2
\$\begingroup\$

I've written a C function to find the program less that the operating system should provide. If lessis not provided, some other pager variable should be used. I compile it with gcc -ansi -pedantic -Wall -O3.

int findless(char *less_token, int ret) {
    int breakloop = 0;
    char *new_str;
    while (less_token != NULL) {
        if ((new_str = malloc(strlen(less_token) + strlen("/less") + 1)) != NULL) {
            new_str[0] = '\0';
            strcat(new_str, less_token);
            strcat(new_str, "/less");
            if (file_exist(new_str)) {
                /* Found less */
                ret = 0;
                breakloop = 1;
            }
            free(new_str);
            if (breakloop) {
                break;
            }
        } else {
            printf("malloc failed!\n");
        }
        less_token = strtok(NULL, ":");
    }
    return ret;
}

Can you please review it? I put in in a separate utilities util.c file and I'm including it with a util.h interface.

\$\endgroup\$
1
\$\begingroup\$

Clearer Naming Needed

In general, I think your variable naming could be better. I would rename less_token to be called path or something to make it clearer what you're doing here. new_str should be called current_path or curr_path. Moreover, I'd make the function name something more like find_less_program.

C Style

If you're going to be changing the ret parameter, it's idiomatic in C to pass in a pointer to ret and modify that. Either that or remove ret as a parameter and just return an int as an error code. Moreover, whenever I see a break in a while loop, I get a small frown on my face as that's usually a sign of bad control flow. See below for (IMO) a better control flow.

Moreover, it's been over a decade since C99 came out and introduced us to the bool type definitions. Prefer using them over the standard int way of signaling true or false. You'll need to include stdbool.h to use bool.

I would also create a small constant buffer containing the name of the program we're looking for (in this case "/less".) This prevents you from having to write "/less" all over the place and instead just use a variable name. There's no need to assign the first character of the allocated buffer to NULL in this case. Just call strcpy to copy the string name into the buffer instead of relying on strcat. This also makes your intent clearer as you're first copying an existing string into a buffer and then appending to it.

Here's a possible implementation taking together all of the above suggestions:

bool find_less_program(char *path) {

    bool found = false;
    char *curr_path;
    const char program[] = "/less";

    while (path && !found) {
        if ((curr_path = malloc(strlen(path) + sizeof(program))) != NULL) {

            strcpy(curr_path, path);
            strcat(curr_path, program);
            if (file_exist(curr_path)) {
                found = true; // we found the program
            }
            free(curr_path);
            path = strtok(NULL, ":");
        } else {
            fprintf(stderr, "malloc failed!\n");
            return false;
        }
    }
    return found;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer. I compile it with gcc -ansi -pedantic -Wall -O3 though that is not final. Maybe I can't use the bool type because I compile using an old standard (C90). \$\endgroup\$ – Niklas Apr 15 '16 at 19:39
  • 1
    \$\begingroup\$ No problem. In that case, it's trivial to switch from bool to int and replace true with 1 and false with 0. :) \$\endgroup\$ – Bizkit Apr 15 '16 at 19:40
  • \$\begingroup\$ Now I also think there could be a typo where you put less_token instead of path in the head of the while loop. \$\endgroup\$ – Niklas Apr 15 '16 at 19:42
  • 1
    \$\begingroup\$ Oops! You're correct. I fixed it in the answer :) \$\endgroup\$ – Bizkit Apr 15 '16 at 19:45
  • 1
    \$\begingroup\$ Now I tested the code and it works perfectly. \$\endgroup\$ – Niklas Apr 15 '16 at 19:45

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.