Hi, I'd like to ask for criticisms on a snippet of code

Question

Well, a bit of background, but skip(to the bold) this if you'd just like to see the code to criticize. (Edit: sorry I had no clue code review existed)

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.

Answer 1

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

Answer 2

Parsing arguments to a C program is a problem that has been solved. I understand that you want to flex your C muscles again. I would recommend flexing them by reusing libraries.

This will accomplish a few things:

• Reduce the number of bugs in your code.
• Leverage a more flexible way of parsing arguments.
• Give you more time to work on more interesting parts of your program.
• Give you practice integrating external libraries into your code.
• Eliminate the arrow code.

See also:

• http://www.purposeful.co.uk/software/gopt/
• http://stackoverflow.com/questions/2913767/library-for-parsing-arguments-gnu-style
• http://www.gnu.org/software/libc/manual/html_node/Parsing-Program-Arguments.html#Parsing-Program-Arguments
• http://stackoverflow.com/questions/189972/argument-parsing-helpers-for-c-unix

Answer 3

Your code looks perfectly reasonable and the indentation is very standard. The only issue I can see is that C arrays are zero based so the loop

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

should possibly be

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

If you're parsing a command line and deliberately ignoring arg[0], it'd be worth adding a comment to your code saying this. And whether count is the total number of arguments (meaning that your loop exit condition should be currentArg < count) or the number of arguments to be parsed in this function (meaning that currentArg <= count is a valid exit condition)