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.

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.

share|improve this question

migrated from stackoverflow.com Jan 14 '13 at 16:50

This question came from our site for professional and enthusiast programmers.

3 Answers 3

up vote 5 down vote accepted
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);
    }
}
share|improve this answer
    
This is obviously parsing command line flags. There it is natural to start index from 1 as the argv[0] is the application name. –  Loki Astari Jan 14 '13 at 17:45
    
Since flags usually have parameters incrementing the loop counter in the loop here is also relatively standard. The problem is actually the lack of checks to make sure there is an extra parameter. –  Loki Astari Jan 14 '13 at 17:46
    
@LokiAstari, agreed on argv[0] being the application name. But the loop still goes one past the end of the list of arguments. –  Winston Ewert Jan 14 '13 at 17:51
1  
@LokiAstari, I certainly understand why he's incrementing the index there. I just think that if you are going to monkey around with the index like that you should use a while loop to better show your intention. –  Winston Ewert Jan 14 '13 at 17:51
    
Oops! The strcmp was a relic from when I was going to parse strings instead! Thanks for the example and for whatever reason I forgot break it down into smaller functions. This was really helpful, thanks! –  GRAYgoose124 Jan 14 '13 at 18:16

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:

share|improve this answer

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)

share|improve this answer
1  
That's probably as a result of passing argc,argv where argv[0] is the application. All other arguments are flags. So I don't agree that this is a problem or unexpected. –  Loki Astari Jan 14 '13 at 17:44
    
@LokiAstari Thanks. I've updated my answer to take account of your comment. I agree that starting the loop from index 1 is likely to be correct but an exit condition of currentArg <= count still seems questionable. At the very least, this shows that the function would benefit from some comments about its arguments. –  simonc Jan 14 '13 at 17:59
    
Using a '<' would be much more intuitive and easier to read the intentions, that makes sense. Thanks for the help! –  GRAYgoose124 Jan 14 '13 at 18:18
1  
I'd fix the out-of-bounds bug, keep the 1 but avoid "magic numbers". Instead, do something like #define FIRST_CMDLINE_ARG 1 and then for(currentArg = FIRST_CMDLINE_ARG; currentArg < count; ... –  Lundin Jan 15 '13 at 14:27

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.