# Parse program arguments of the form key=value

I have two proposals for parsing arguments:

## Method A

const char *
cmp_skip (const char *key, const char *str)
{
int len = strlen (key);
return strncmp (key, str, len) ? NULL : str + len;
}

void
parser (int argc, const char **argv)
{
for (int i = 0; i < argc; i++) {
const char *str, *arg = argv[i];

if ((str = cmp_skip ("db=", arg)))
db_field = str;
else if ((str = cmp_skip ("table=", arg)))
table_field = str;
else if ((str = cmp_skip ("pass=", arg)))
pass_field = str;
else if ((str = cmp_skip ("type=", arg)))
type_field = str;
else if ((str = cmp_skip ("priv=", arg)))
priv_field = str;
}
}


## Method B

void
parser (int argc, const char **argv)
{
for (int i = 0; i < argc; i++) {
const char *key = argv[i];

if (!strncmp (key, "db=", 3))
db_field = key + 3;
else if (!strncmp (key, "table=", 6))
table_field = key + 6;
else if (!strncmp (key, "pass=", 5))
pass_field = key + 5;
else if (!strncmp (key, "type=", 5))
type_field = key + 5;
else if (!strncmp (key, "priv=", 5))
priv_field = key + 5;
}
}


Anyway we have some hardcoded strings. I think this is inevitable. Our project is limited to specific fields: db, table, pass, type and priv. What is the best choice for assigning their values to our pointers?

I think that B is better because:

• It hasn't any extra abstraction.
• Anyway we hardcoded some strings. Why we can't hardcode their length? Trying to find a known length of a hardcoded string sucks. Doesn't it?
• Anyway in cmp_skip() we implement what we see in this code.
• This code is very smaller than the first code.
• This code doesn't call any functions and doesn't waste resources.
• This code is very simple, readable and optimizable.
• We don't need to extend this structure. But if we to need to, we can refactor this easily.
• YAGNI.

But my friends say that second code is horrible and first code is very good and extensible. But I think actually we don't need flexibility and extendibility for now! Cause YAGNI!

Anyway what is your opinion? I'm really confused ...

• Hardcoding is rarely the right answer, but what kind of project are we talking about? Is this for a study project, an internship or your first job? Code is rarely alone. I'd write this differently for a one-off script than for a production environment, even though I know perfectly well code can move from one environment to another.
– Mast
Jan 15 at 18:52
• I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. Jan 17 at 13:37

I certainly prefer variant A. Despite the extra function call, it's much easier to use correctly. Consider for example that some maintainer (perhaps you) needs to change the name of one of the options. Will you spot that there are two numbers you need to change in addition to the string? The same concern applies to someone who copies an existing option and edits it in order to accept a new option.

Some things that should be fixed regardless:

• cmp_skip() probably ought to have internal (static) linkage.
• The type of len should be size_t, since that's what strlen() returns and strncmp() accepts (have you enabled insufficient compiler warnings?)
• parser() will be more useful if it accepts char** as given to main(). We can comfortably accept char*const* (or char*const*const, which is the same signature), but char const** is inconvenient for users.
• We need some way to indicate invalid use, such as an unrecognised key. Should repeating a key be an error?
• Stop using global variables to communicate between functions.

An alternative you might like to consider is to start by looking for = within the argument just once at the beginning, and use the result to determine key and value parts. That's reasonably easy to follow (easier still if you can choose to overwrite the = with \0, as you can then use plain old strcmp()).

Implementing that approach gives something like:

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct options
{
const char *db_field;
const char *table_field;
const char *pass_field;
const char *type_field;
const char *priv_field;
};

static bool
parse_args(int argc, char **argv,
struct options *opt)
{
for (int i = 0;  i < argc;  ++i) {
const char *const arg = argv[i];
const char *const eq_pos = strchr(arg, '=');
if (eq_pos) {
/* "key=value" */
const size_t key_len = (size_t)(eq_pos - arg);
const char *value = eq_pos + 1;

if (!strncmp("db", arg, key_len)) {
opt->db_field = value;
} else if (!strncmp("table", arg, key_len)) {
opt->table_field = value;
} else if (!strncmp("pass", arg, key_len)) {
opt->pass_field = value;
} else if (!strncmp("type", arg, key_len)) {
opt->type_field = value;
} else if (!strncmp("priv", arg, key_len)) {
opt->priv_field = value;
} else {
/* key not recognised */
return false;
}
} else {
/* no equals sign */
return false;
}
}
/* all arguments successfully processed */
return true;
}

int main(int argc, char **argv)
{
struct options options = {0, 0, 0, 0, 0};
if (argc >= 1 && !parse_args(argc - 1, argv + 1, &options)) {
fputs("Invalid arguments\n", stderr);
return EXIT_FAILURE;
}
}

• That could indeed be a problem, but I changed to checking argc before calling, so as not to invalidate existing uses that are not from main(), such as the unit tests. Reconsidering that interface is definitely something I'd recommend, though. Jan 18 at 8:07
• @chux, please edit for minor typos/thinkos such as std:: in C posts - that's a much more efficient means of correction than commenting! Thanks. Jan 18 at 8:12

As with many things in C, the answer is "use a macro."

Hard-coding values is error-prone because there is the risk you will either (a) get a value wrong; (b) change one of the values but not the other; or (c) copy and paste a function, then (b).

The answer, of course, is to compute the length as part of the comparison. But how can you possibly compute the length of a literal string known at compile time? If only there was a way...

... and there is. Use sizeof on the literal string and it will have type char[], including the trailing zero. Subtract one and you have the value you want:

strncmp(key, "db=", sizeof("db=") - 1)


Then wrap that in a macro:

#define EQ_FIELD(key, literal)  (strncmp((key), (literal), sizeof (literal) - 1) == 0)


Then rewrite your code to use the macro:

const char *key = argv[i];

if (EQ_FIELD(key, "db="))
db_field = key + 3;
else if (EQ_FIELD(key, "table="))
table_field = key + 6;
else if (EQ_FIELD(key, "pass="))
pass_field = key + 5;


But I notice you use the same magic numbers in the computation of the field pointer offset. So maybe the macro needs to be expanded to include the assignment:

SET_FIELD_IF_MATCH(db_field, key, "db=")
else
SET_FIELD_IF_MATCH(table_field, key, "table=")
else
SET_FIELD_IF_MATCH(pass_field, key, "pass=")
...
else REPORT_ERROR("WTF field is this supposed to be: %s", key);


Note: If you are truly convinced of the perfect and unchanging nature of your fields list, have a look at a topic called "perfect hashing". Basically, if the list of field names is unchanging, you can generate a hash function that perfectly maps strings into index numbers. (This is widely underused, mainly because the lists of keywords are never as static and unchanging as people claim they are, so when they do stop changing, nobody remembers to go back and perfect-hash them...)

• I agree that hashing or table lookup might be better, but I don't think a macro is a good idea. I'm not a big fan of hiding code. Jan 15 at 19:27
• I think making bugs or mistake on implementing this macro will be greater that typing 5 field len. Ok using macro make it optimizer. What about simplisity ? Is using macro easy to understand and debug ? Project is a small part of production. But it is actually small and fixed. Why we should refactor this magic numbers ? Jan 16 at 3:08
• The macro is somewhat risky if a future user doesn't realise that a string literal must be passed, and uses the macro with a char* instead. Then the sizeof operator will happily determine the size of the pointer, rather than giving us a nice compilation error. So it's a bit too fragile for my liking. Jan 16 at 15:58

Method A

• Pro: Only 1 usage of key needed in caller

• Con: Wrong length type. Easy to fix. Use size_t.

Method B

• Con: Obliges coordinating 3 objects/constants based on key target. 2 are magic numbers.
    //                        1       2
else if (!strncmp (key, "table=", 6))
table_field = key + 6;
//                    3


Instead to avoid naked magic numbers, could use

#define KEY_TABLE "table="
#define KEY_TABLE_LEN (sizeof (KEY_TABLE) - 1)

else if (!strncmp (key, KEY_TABLE, KEY_TABLE_LEN))
table_field = key + KEY_TABLE_LEN;

• Con: arg string is called key - yet it is not the key. Simple to fix: use better name.

I find Method A easier to maintain and so the preferred choice.

I would even consider:

bool cmp_skip(const char *key, const char *str, char **dest)


And let the helper function conditionally assign *dest.