Parse program arguments of the form key=value

Question

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 ...

Answer 1

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

Answer 2

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...)

Answer 3

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.