Bash command helper in C

Question

Previously, I had written this program in C++ and asked about it here on Code Review. After spending some time with C++, I desired to become more familiar with its namesake. This is my first C program.

The purpose of the program is to help with remembering complex bash commands. It's invoked with arguments describing the desired operation and displays the commands matching the input ordered by closest match.

I'm trying to stick as close as possible to ANSI C, and I'm still not clear on when to use char [] or char *.

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

#define MAX_TEXT_LENGTH 100
#define MAX_DESC_LENGTH 100
#define MAX_KEYWORDS_LENGTH 100
#define NUM_COMMANDS 8
#define NUM_RESULTS 3

struct command {
    char text[MAX_TEXT_LENGTH];
    char description[MAX_DESC_LENGTH];
    char keywords[MAX_KEYWORDS_LENGTH];
};
struct command commands[NUM_COMMANDS] = {
    { "amixer -Mq set Master 1%-", "decrease volume", "lower" },
    { "amixer -Mq set Master 1%+", "increase volume", "raise" },
    { "makepkg -sri", "build and install a package using a PKGBUILD" },
    { "makepkg -efi", "rebuild and reinstall a package using a PKGBUILD" },
    { "pacman -Qdt", "list orphaned packages", "find" },
    { "pacman -Qe", "list explicitly installed packages", "find" },
    { "pacman -Ql [package]", "list files owned by package", "find" },
    { "pacman -Qo [file]", "list packages that own file", "find owned" }
};
struct result {
    int found;
    char text[MAX_TEXT_LENGTH];
    char description[MAX_DESC_LENGTH];
};
struct result results[NUM_COMMANDS];

int compare_results(const void *, const void *);
void convert_lower(char []);

int main(int argc, char *argv[])
{
    char current[MAX_TEXT_LENGTH + MAX_DESC_LENGTH + MAX_KEYWORDS_LENGTH];
    int a, c, found, r;

    /* convert argv to lowercase */
    for (a = 1; a < argc; ++a)
        convert_lower(argv[a]);
    for (c = 0; c < NUM_COMMANDS; ++c) {
        strcpy(current, commands[c].text);
        strcat(current, commands[c].description);
        strcat(current, commands[c].keywords);
        /* convert current to lowercase */
        convert_lower(current);
        found = 0;
        /* count words in both argv and current */
        for (a = 1; a < argc; ++a)
            if (strstr(current, argv[a]) != NULL)
                ++found;
        if (found == 0)
            continue;
        results[c].found = found;
        strcpy(results[c].text, commands[c].text);
        strcpy(results[c].description, commands[c].description);
    }
    /* sort commands with most words in common with argv at top */
    qsort(results, NUM_COMMANDS, sizeof(*results), compare_results);
    /* only print NUM_RESULTS results */
    for (r = 0; r < NUM_RESULTS && results[r].found > 0; ++r) {
        printf("%s\n", results[r].text);
        printf("    %s\n", results[r].description);
    }
    return 0;
}

int compare_results(const void *a, const void *b)
{
    struct result *r1 = (struct result *)a;
    struct result *r2 = (struct result *)b;

    return r2->found - r1->found;
}

void convert_lower(char string[])
{
    int c;

    for (c = 0; string[c] != '\0'; ++c)
        string[c] = tolower(string[c]);
}

Answer 1

* or []

There isn't really a definitive time when you should and shouldn't use pointers or arrays (at least not that I know of). However, I generally only use arrays for:

• Large buffers. For example, if you were reading a file.
• A definitive size array of a simple type, such as an int or a char.
• A group of constants all sharing the same type (your commands array, for example).

The two things are very similar. However, pointers are more idiomatic and C and I find are actually a lot easier to use.

---

Here is a great example of using pointers or arrays:

for (c = 0; string[c] != '\0'; ++c)
    string[c] = tolower(string[c]);

If instead of receiving an array through an argument you received a char *, you could do this:

for(; *string != '\0'; string++) {
    *string = tolower(*string);
}

This completely eliminates the need for c, and is a lot cleaner. Here, in the above snippet, we are taking the pointer string and increment through it each iteration.

Note that you still might be able to use an array in the arguments, it would just look a lot cleaner if you wrote it like this:

char *string

instead. I think it would still work either way (I did not test it), which is one of those gray areas between arrays and pointers.

---

const vs #define

Your #defines as the top should not be #defines:

const int MAX_TEXT_LENGTH = 100
const int MAX_DESC_LENGTH = 100
const int MAX_KEYWORDS_LENGTH = 100
const int NUM_COMMANDS = 8
const int NUM_RESULTS = 3

This is better practice than using #defines as those do not specify a direct type for the value; the pre-processor simply substitutes that number as a string into your code and it is up to the compiler to determine a type.

---

sprintf

    strcpy(current, commands[c].text);
    strcat(current, commands[c].description);
    strcat(current, commands[c].keywords);

Three separate function calls to concatenate three strings? That's not very efficient; and it's quite ugly. It would be better to use sprintf:

sprintf(current, "%s%s%s", commands[c].text, commands[c].description, commands[c].keywords);

Actually, this looks like a toString method for a command struct. Time for a refactor!

void buffer_tostring(char *buf, struct command);

Simply move that code (with slight adjustments) into a method like the one above; it is good to split your code into as many different (and logical) methods as you can so that everything follows the single responsibility principle, overall making your code more maintainable.

You could also do the same thing for your results struct regarding here:

    strcpy(results[c].text, commands[c].text);
    strcpy(results[c].description, commands[c].description);

---