Simple program to split arguments up

Question

I am writing this small tester program to be used as part of a larger project, and I am quite unsure about the strtok function, which I (think) I need to use to split some character arrays up on a certain character (in my case this is a colon). For each character array (which is passed in as an argument), it should split the array into two. Apologies if it's hard to follow, I'm bad at explaining things.

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

int main(int argc, char **argv)
{
    char *pt;

    for (int i = 1; i < argc; i++)
    {
        pt = strtok(argv[i], ":");

        int j = 0;

        while (pt != NULL || j < 2)
        {
            j++;

            if (j == 1)
            {
                printf("item: ");
            } else if (j == 2)
            {
                printf("quantity: ");
            }

            printf("%s\n", pt);
            pt = strtok(NULL, ":");
        }

        j = 0;
    }

    return 0;
}

Yes, I know the program will crash if not enough arguments are supplied, but I just quickly threw this together to try and learn more about strtok. Now although this code works, I feel it is not the most efficient. I have not been programming C for very long, so any help for how I could improve this loop would be greatly appreciated.

Answer 1

I see some things that may help you improve your program.

Break up the code into smaller functions

This is a very short program, so it's not critical here, but generally, rather than having everything in the main() function, it would be easier to read and maintain if each discrete step were its own function. For instance, I'd separate out the token splitting function so that main looks like this:

int main(int argc, char **argv) {
    for (int i = 1; i < argc; i++) {
        split(argv[i]);
    }
}

Eliminate work that does not need to be done

The final j = 0 has no practical effect and should be eliminated. Think carefully about what your program is doing so that you don't have the computer do more work than is necessary.

Eliminate "magic numbers"

The code is mostly free of "magic numbers' (that is, hard-coded values for which there is no obvious meaning), but I'd suggest that it would make sense to have the delimter string be an assigned variable.

Use a for loop instead of while

Because both pt and the j variables are only used within the while loop, consider again reducing scope and turning that into a for loop with one of those variables.

Use a single call to printf where practical

Instead of calling printf twice for each parsed value, it would simplify the program to simply call printf once instead of twice.

Sanitize user input better

The code doesn't quite work as posted, as you apparently already know. It doesn't really take much more time or code to make the program not crash with malformed input, so I would strongly suggest doing so to make your program robust.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Results

Here's a reworked version of your program incorporating all of these suggestions:

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

void split(char *arg) {
    static const char* label[] = { "item", "quantity", NULL };
    static const char *delim = ":";
    const char **lbl = label;
    for (char *pt = strtok(arg, delim); pt != NULL && *lbl != NULL; ++lbl) {
        printf("\t%s: %s\n", *lbl, pt);
        pt = strtok(NULL, delim);
    }
}

int main(int argc, char **argv) {
    for (int i = 1; i < argc; i++) {
        printf("\nInput: %s\nOutput:\n", argv[i]);
        split(argv[i]);
    }
}

Some notes

The revised code includes two static const variables. This means that they are initialized at compile time and do not change during the course of the program. Further, every invocation of the split function has those variables set to the exact same values. The advantage is that the compiler can turn this into very efficient code requiring little or no runtime execution and minimal code size.

The lbl variable simply points to the first label "item" first, and then steps through the labels in label until it encounters NULL at which point the routine will end whether or not there are more delimiters in the input string.

Finally, I'd recommend creating test code that runs a number of test cases and compares the actual output to the desired output. By creating such a test program, you can be assured that if you change the program later (e.g. for more efficiency or to add additional features), you will be able to assure that the function still operates as intended and that no new unintended behavior (bugs!) have been introduced.

Sample run

./splitter foo:bar toes:10 Edward: nocolon two:colons:here :: : smoo

Input: foo:bar
Output:
        item: foo
        quantity: bar

Input: toes:10
Output:
        item: toes
        quantity: 10

Input: Edward:
Output:
        item: Edward

Input: nocolon
Output:
        item: nocolon

Input: two:colons:here
Output:
        item: two
        quantity: colons

Input: ::
Output:

Input: :
Output:

Input: smoo
Output:
        item: smoo