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
strtok
. Thanks for answering! \$\endgroup\$