HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Criticize C argument parsing method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
parsingcriticizeargumentmethod

Problem

On an IRC channel I'm a member of, one particular person mocks every piece of code I post, even if it's a rough draft or if I haven't coded in months. I am pretty well versed with computer theory and knowledge, but my skills with programming lack due to little practice.
This particular code was from a project I was starting and I was putting down some ideas/structure on GitHub, when he pointed out this particular snippet.

So, after a couple months of trying to get the discipline to start coding again, I coded this and my question is: If indenting like this is bad, what other things did I do wrong here, assuming the logic is sound (I never tested it, I went on hiatus)? Also, what would you do to avoid the indenting?

void parse_arguments(int count, char * arguments[]){//probably add file pointer here  to pass to load, change return type when an API is decided.
    int currentArg, paramIndex;

    for(currentArg = 1; currentArg <= count; currentArg++){
        if(arguments[currentArg][0] == '-'){
            for(paramIndex = 0; paramIndex < PARAMMAX; paramIndex++){
                if(strcmp(arguments[currentArg][1], arglist[paramIndex]) == 0){
                    switch(paramIndex){
                        case 0:
                            //load(currentArg + 1);
                            currentArg++;
                            break;
                        default:
                            break;
                    }
                    break;
                }
            }
        }
    }
}


Note: arglist is a statically declared array of chars.

Solution

void parse_arguments(int count, char * arguments[]){//probably add file pointer here  to pass to load, change return type when an API is decided.
    int currentArg, paramIndex;

    for(currentArg = 1; currentArg <= count; currentArg++){


Why are you counting from zero? In C arrays start from zero, but you are starting from 1. As it stands it looks like you skip the first argument and then read one past the end

if(arguments[currentArg][0] == '-'){
            for(paramIndex = 0; paramIndex < PARAMMAX; paramIndex++){
                if(strcmp(arguments[currentArg][1], arglist[paramIndex]) == 0){


Isn't arguments[currentArg][1] a char? How are you passing it to strcmp?

switch(paramIndex){
                        case 0:
                            //load(currentArg + 1);
                            currentArg++;


I recommend against manipulating loop counters inside your loop. It makes it harder to follow whats going on.

break;
                        default:
                            break;
                    }
                    break;
                }
            }
        }
    }
}


So much indentation suggests that you should break this down into smaller functions. I'd also suggest that you store the results of the arguments into a struct.

A general sketch of how I'd do this:

struct options
{
    char * filename;
    int numberOfRabbits;
};

struct parameter
{
    char * name; // 'x' for option '-x'
    bool requiresOption; // whether or not we should have an option
    void (*handler)(struct options * options, char * argument);
};

void parse_arguments(int count, char * arguments[], struct options * options)
{
    char * argument;
    int position = 0;
    while(position < count)
    {
         struct parameter * param = determine_parameter(arguments[position++]);
         if(!parameter)
         {
              panic(); // don't know this parameter
         }
         if(param.requiresOption)
         {
               argument = arguments[position++];
         }else{
               argument = NULL;
         }
         param.handler(options, argument);
    }
}

Code Snippets

void parse_arguments(int count, char * arguments[]){//probably add file pointer here  to pass to load, change return type when an API is decided.
    int currentArg, paramIndex;

    for(currentArg = 1; currentArg <= count; currentArg++){
if(arguments[currentArg][0] == '-'){
            for(paramIndex = 0; paramIndex < PARAMMAX; paramIndex++){
                if(strcmp(arguments[currentArg][1], arglist[paramIndex]) == 0){
switch(paramIndex){
                        case 0:
                            //load(currentArg + 1);
                            currentArg++;
break;
                        default:
                            break;
                    }
                    break;
                }
            }
        }
    }
}
struct options
{
    char * filename;
    int numberOfRabbits;
};

struct parameter
{
    char * name; // 'x' for option '-x'
    bool requiresOption; // whether or not we should have an option
    void (*handler)(struct options * options, char * argument);
};

void parse_arguments(int count, char * arguments[], struct options * options)
{
    char * argument;
    int position = 0;
    while(position < count)
    {
         struct parameter * param = determine_parameter(arguments[position++]);
         if(!parameter)
         {
              panic(); // don't know this parameter
         }
         if(param.requiresOption)
         {
               argument = arguments[position++];
         }else{
               argument = NULL;
         }
         param.handler(options, argument);
    }
}

Context

StackExchange Code Review Q#20511, answer score: 5

Revisions (0)

No revisions yet.