4
\$\begingroup\$

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.

\$\endgroup\$
1

3 Answers 3

5
\$\begingroup\$
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);
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ This is obviously parsing command line flags. There it is natural to start index from 1 as the argv[0] is the application name. \$\endgroup\$ Jan 14, 2013 at 17:45
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 14, 2013 at 17:46
  • \$\begingroup\$ @LokiAstari, agreed on argv[0] being the application name. But the loop still goes one past the end of the list of arguments. \$\endgroup\$ Jan 14, 2013 at 17:51
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Jan 14, 2013 at 17:51
  • \$\begingroup\$ 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! \$\endgroup\$ Jan 14, 2013 at 18:16
2
\$\begingroup\$

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:

\$\endgroup\$
1
\$\begingroup\$

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)

\$\endgroup\$
4
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Jan 14, 2013 at 17:44
  • \$\begingroup\$ @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. \$\endgroup\$
    – simonc
    Jan 14, 2013 at 17:59
  • \$\begingroup\$ Using a '<' would be much more intuitive and easier to read the intentions, that makes sense. Thanks for the help! \$\endgroup\$ Jan 14, 2013 at 18:18
  • 1
    \$\begingroup\$ 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; ... \$\endgroup\$
    – Lundin
    Jan 15, 2013 at 14:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.