C - should I use a variable array of function pointers

Question

This question is about how to solve my problem on the level of how I design my program. For a school project, I'm building a shell, which has several built-in functions. One of these function's purpose (cmd_type) is to check to see if the argument provided is in that list of functions. Here is a partial implementation of it:

int cmd_type(int argc, char *argv[]) {
    if (argc == 2) {
        for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) {
            if (strcmp(cmds_name[i], argv[1]) == 0) {
                printf("%s is a shell builtin\n", argv[1]);
                return 0; // found it
            }
        }

        // still need to search path, call stat(path/cmd)
        errmsg("not implemented! type", 1);
    } else {
        err_msg("type", 1);
    }
}

Defining manual if statements for every function my shell supports sounds like a bad choice because the list might expand over time, and I need to store the list of function names anyway. So originally, I planned to define an array of function names and an array of their pointers, like so:

char cmds_name[BUILTIN_FUNC_COUNT-1][16];
char (*cmds_ptr)(int,*char[])[BUILTIN_FUNC_COUNT-1];
// make list of built-in funcs
strcpy(cmds_name[0], "exit");
strcpy(cmds_name[1], "cd");
// make list of func pointers
cmds_ptr[0] = &cmd_exit;
cmds_ptr[1] = &cmd_cd;

They're accessed like so:

// try builtin cmds
for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) {
    if (strcmp(cmds_name[i], argv[0]) == 0) {
        last_cmd_err = (*cmds_ptr[i])(argc, argv);
        continue; // we found it, so next loop
    }
}

Then they'd each happily take (int argc, char *argv[]) as arguments. But the cmd_path() needs access to the list in addition to those arguments, so I'd have to define it as a global, or define a global pointer to it... In the process of researching this, I found this answer, saying a similar approach was really bad style: https://stackoverflow.com/a/41425477/5537652

So my questions are: Is this a good way to solve this problem, or should I just do if/else statements/is there a better way? Would you recommend a global pointer to the array of function names?

Answer 1

I am going to propose a structure of cmd_name and function pointer like this:

typedef struct{
  char cmds_name[16];
  char (*cmds_ptr)(int,*char[]);
} cmd_type;

Now define a static table of this type for all your cmds:

static const cmd_type cmd_table[] = {
  {"exit", &cmd_exit},
  {"cd", &cmd_cd},
  .......
  .......
};

Finally access it like this:

for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) {
  if (strcmp(cmd_table[i].cmds_name, argv[0]) == 0) {
    last_cmd_err = (*cmd_table[i].cmds_ptr)(argc, argv);
    continue; // we found it, so next loop
  }
}

The decision to choose between if-else vs a global table is a matter of personal taste and coding style. I would prefer the above solution simply because it improves ** code readability** and reduces clutter. There may be other constraints in your environment that can influence your decision - like if the no of table entries is huge and there is a limitation on global memory space - the if-else route would be a better choice..

HTH!

Answer 2

I would not go with if-else statements. There is nothing wrong with solution (2) proposed in https://stackoverflow.com/a/41425477/5537652.

You could have a table with a string and a function to service an entry:

typedef struct cmd_desc
{
  char cmd[80];
  int builtin_cmd(int argc, char **argv, void *extra);
} CMD_DESC;

static CMD_DESC descTable[] =
{
  { "exit",                 cmd_exit      },      
  { "cd",                   cmd_cd        },   
  { "$ON_OPEN_CMD",         OnOpenCmd     },
  { "$OPEN_EXTRA_CMD",      OpenExtraCmd  },
  { "$AC",                  ActionCmd     },
  { "$AD",                  ActionDataCmd },
  { "$EC",                  ExtraCmd      },
  { "$TC",                  TextCmd       },
  { "",                     NULL          }
};

int cmd_exit (int argc, char **argv, void *extra)
{
  //...
}

Access/execution:

for (int tokenIndex=0; strcmp(descTable[tokenIndex].cmd,""); tokenIndex++) //search table 
{
    if ( strcmp( (descTable[tokenIndex]).cmd, argv[0] ) == 0 )
    { 
        int ret = (*(descTable[tokenIndex]).builtin_cmd( argc, argv, extra);
    }
}

I used the above approach in a my applications and it worked well for me.

The table can be easily expanded and the readability of the table is better than if/else chain.