current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I am writing a Unix shell in C, which uses a rather large switch in the main loop.

It is used to handle built-in commands like 'cd' or'help'. The way it works is that it parses the user input and return a 'comand_t' struct, with 'name' being the first word of the input.

The in order to switch on a string, I translate it into an int by reading this initialized struct:

built_in_commands my_commands[CMD_NBR] = { 
    {""     , EMPTY_CMD         , ""},
    {"cd"   , CD_CMD            , "change directory"},
    {"env"  , ENV_CMD           , "print enviroment. Can show single variable"},
    {"exit" , EXIT_CMD          , "exit from AGROS"},
    {"help" , HELP_CMD          , "print help"},
    {"?"    , SHORTHELP_CMD     , "print short help"}
};

The macros are defined in the header file:

#define EMPTY_CMD           0

#define CD_CMD              1
#define ENV_CMD             2
#define EXIT_CMD            3
#define HELP_CMD            4
#define SHORTHELP_CMD       5
#define SETENV_CMD          6
#define GETENV_CMD          7

#define OTHER_CMD           8

#define CMD_NBR             9

the main loop of the program is this:

while (AG_TRUE){

    /* Set the prompt */
    get_prompt(prompt, MAX_LINE_LEN, username);

    /* 
     * Read a line of input 
     * commandline should be deallocated with free() 
     */
    commandline = read_input (prompt); 

    parse_command (commandline, &cmd);

    switch (get_cmd_code (cmd.name)){ //returns an int 
        case EMPTY_CMD:
            break;

        case CD_CMD:
            change_directory (cmd.argv[1], ag_config.loglevel);
            break;

        case HELP_CMD:
            print_help(&ag_config, cmd.argv[1]);
            break;

        case SHORTHELP_CMD:
            print_help(&ag_config, "-s");
            break;

        case ENV_CMD:
            print_env (cmd.argv[1]);
            break;

        case EXIT_CMD:
            free (commandline);
            commandline = (char *)NULL;
            closelog ();
            return 0;

        case OTHER_CMD:
            fork_and_exec();
            break;
    }
}

Every time I add a new builtin function, I wonder if there isn't a better way to do this. What would it be? How can I call a function dynamically?

share|improve this question

2 Answers 2

up vote 5 down vote accepted

In addition to @Winston notes on function pointers.

I would not do this:

built_in_commands my_commands[CMD_NBR] = { 
                   ///        ^^^^^^^  This makes mainting the code harder.
                   ///                 If you add commands you now need to modify the code
                   ///                 in multiple places.

It is easier to do it like this:

built_in_commands my_commands[] = { 
                   ///       ^^        By leaving it blank the compiler works out the size.


int const CMD_NBR = sizeof(my_commands)/sizeof(my_commands[0]);
                   /// Now the compiler works out your constant of the size of the array.
                   /// Note: This also works for empty arrays as sizeof() 
                   ///       is done at compile time and thus works even if
                   ///       my_commands[0] is not technically valid at
                   ///       run-time.
share|improve this answer
    
Thank you very much for your remarks! –  rahmu Oct 22 '11 at 15:30
up vote 5 down vote

Function pointers

You want to use function pointers: http://www.newty.de/fpt/fpt.html

typedef void (*Command)(char const* args);  // Modify as required
                                            // But something like this
struct built_in_commands
{
    char const*   command;
    // int        commandID;   // Remove this you don't need it.
    char const*   man;
    Command       function;    // Add this: A pointer to a function
};

Then your switch becomes:

parse_command (commandline, &cmd);
if (cmd != NULL)
{    (*cmd->function)(commandline);      // Call the function associated with
}                                        // command (assuming you find it in the list)

Other Notes:

Basically, all of your function would have the same signature, and then be referred to inside of your struct instead of the number. Then you could call the function pointer from the struct and away you'd go.

        case EXIT_CMD:
            free (commandline);
            commandline = (char *)NULL;
            closelog ();
            return 0;

This appears to be the only place you free commandline. But you allocate it everytime you get a command. This means that you have a leak. There actually isn't much of a point in freeing the memory here because it will all be free as soon as the process terminates anyways.

share|improve this answer
2  
Seemed pointless writing the same thing as you so just added an example. –  Loki Astari Oct 15 '11 at 18:45

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.