Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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]);
}
share|improve this question
    
Have you considered using Bash Programmable Completion? You could start typing a command, then hit TAB to get suggestions for flags and filenames. – 200_success Jan 11 at 1:39
    
@200_success I know about tab completion, and I use bash history quite a lot. But some commands aren't easy to search for, like pacman -Qdt, so I have to read the man page again or ask Google. It's much easier to type at the command line bh orphan. – Dave Kennedy Jan 11 at 2:43

* 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);

share|improve this answer
    
When I change those defines to const ints I get compiler errors: variably modified ‘text’ at file scope: char text[MAX_TEXT_LENGTH];. – Dave Kennedy Jan 11 at 3:06
    
It doesn't seem that I can have a function inside of a struct either: expected ‘:’, ‘,’, ‘;’, ‘}’ or ‘__attribute__’ before ‘{’ token. – Dave Kennedy Jan 11 at 3:18
1  
1. In C, const does not actually mean constant :) c-faq.com/ansi/constasconst.html So you can't use it as a static array length. 2. Functions cannot go inside structs. That's the realm of OOP. Treat the buffer_tostring function as you would any other function. – gardenhead Jan 11 at 5:07

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.