patterncMinor
Criticize C argument parsing method
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?
Note:
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.