# Creating a string from command line arguments

I am fairly new to C and am just overcoming my fear of pointers and segmentation faults. C is a strange language to me because I come from more object oriented languages like JavaScript and Java.

When I code in C, everything feels all "one-liney" to me. I'm used to things, for example in Java, being neatly spread out, everything with it's own clean section in the code.

When I see C(ha) that I write, everything is all in one place and nothing has it's own area.

Here is an example piece of code that I recently wrote. It takes all the command line arguments and puts them all into one string with each word separated by a space (dismiss any and all spelling errors):

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

int main(int argc, char *argv[]) {
if(argc == 1) { // if there are too few arguments
printf("ERROR: Expected atleast 1 argument\n");
return 0;
}
int size = 0;
while(argv[size]) { // gets the amount of arguments besides the first argument
size++;
}
size--;
if(size == 1) { // if there is only 1 word
printf("%s\n", argv[1]);
return 0;
}
int i;
int v = strlen(argv[1]) + strlen(argv[2]); // for allocating memroy

char *str = (char *)malloc(v);
strcat(str, argv[1]); // putting the string together
strcat(str, " ");
strcat(str, argv[2]);
strcat(str, " ");
for(i = 3; i <= size; i++) {
str = (char *)realloc(str, (v + strlen(argv[i]))); // reallocates enough memory
strcat(str, argv[i]);
strcat(str, " ");
}
printf("%s\n", str);
return 0;
}

One thing that I read somewhere about C involving convention is that C should be broken up into smaller bits of code (functions?). Is this true, and what else should I work on?

In the last lines of your question, what you mention is C being a procedural language. This means, you should split your semantic logic into reusable smaller logical bits and implement smaller portions of your code and implement your program by using the functions you implemented.

Although, as you come from a object-oriented language, I believe you must be familiar with the concepts I mentioned.

Coming to your example, your program consists of a fairly small codebase and it does these things:

• Iterate through the argv strings
• Combine the argv strings in another string

But if I did not misunderstand, your first iteration is to find the count of the arguments. That is what argc variable, therefore your first while loop is unnecessary, you can simply set size = argc - 1;

You have an unnecessary condition check and this part is unnecessary:

strcat(str, argv[1]); // putting the string together
strcat(str, " ");
strcat(str, argv[2]);
strcat(str, " ");

You should start your for loop from 1 instead of 3, and remove these lines.

Your condition check also seems unnecessary, as your for for loop would produce similar results.

Therefore, I would not say that this code is bad due to not being written in procedural paradigm, but it is bad due to overall unnecessary lines of code.

A shorter approach would be:

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

int main(int argc, char *argv[]) {
if(argc == 1) {
printf("ERROR: Expected at least 1 argument\n");
return 0;
}

int i, v = 0, size = argc - 1;

char *str = (char *)malloc(v);

for(i = 1; i <= size; i++) {
str = (char *)realloc(str, (v + strlen(argv[i])));
strcat(str, argv[i]);
strcat(str, " ");
}

printf("%s\n", str);
return 0;
}
• Thank you for rewriting the code as an exemplar. I will read up on functional based programming languages. If I could +1, I would. Dec 7 '14 at 22:44
• Functional? I think procedural is the standard term. Functional makes me think of languages like Haskell. Dec 8 '14 at 0:27
• @icktoofay you are right, I've written that in a late hour, sorry about that. Dec 8 '14 at 5:54
• You need to add one for the " " in allocated memory. Also strcat will fail the first time because the allocated memory is not zeroed. I'd avoid using strcat and use memcpy instead, keeping a note of the length as you go. Dec 8 '14 at 16:20
• It's worth mentioning that size = argc - 1 subtracts 1 from argc because the first argument (argv[0]) contains the name of the binary being ran (i.e. if we ran ls -la, argv[0] would be ls, so if we wanted just the arguments passed to ls we would subtract 1 from the argument count). Jun 11 '16 at 22:24
• Trust the promise. argc is the Argument Count:

int size = 0;
while(argv[size]) { // gets the amount of arguments besides the first argument
size++;
}

At this point you may assert(size == argc);

• Here be arguments:

int v = strlen(argv[1]) + strlen(argv[2]); // for allocating memroy

We checked argv[1]. However argv[2] may not be there.

int v = strlen(argv[1]) + strlen(argv[2]); // for allocating memroy

char *str = (char *)malloc(v);
strcat(str, argv[1]); // putting the string together
strcat(str, " ");
strcat(str, argv[2]);
strcat(str, " ");

v doesn't have enough space to accomodate both strings and two spaces and a terminating 0.

str = (char *)realloc(str, (v + strlen(argv[i]))); // reallocates enough memory

It doesn't reallocate enough. The subsequent strcat(str, " ") is not accounted for.

• I never thought about allocating space for the space. Thank you for bringing that up. Dec 8 '14 at 1:53

You don't actually need to concatenate everything into one string, as you can just print each argument as you encounter it. However, I'm going to assume that you did this the hard way as a memory management exercise.

The major faults are:

• You haven't allocated quite enough memory to account for the spaces and the terminating NUL character.
• There are too many special cases.
• Reallocating memory is likely to be inefficient. It would probably be better to get the buffer length correct the first time.
• It is customary to print error messages to stderr and exit with a non-zero status if a critical error occurs.

Additionally, I suggest avoiding strcat(), since it has a hidden strlen() operation, which is slightly expensive.

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

int main(int argc, char *argv[]) {
if (argc == 1) {
fprintf(stderr, "No arguments\n");
return 1;
}

// Allocate memory for all the strings, plus one space per argument,
// plus a NUL terminator.
size_t size = 1;
for (int i = 1; i < argc; i++) {
size += strlen(argv[i]) + 1;
}
char *str = malloc(size);
if (!str) {
fprintf(stderr, "No memory\n");
return 2;
}

// Concatenate all arguments with a trailing space after each.
// size keeps a running total of the length of the string being built.
size = 0;
for (int i = 1; i < argc; i++) {
// sprintf() copies the argument to the buffer starting at the
// offset specified as size.  There is also a space appended
// (because it is specified by the format string) and a NUL
// terminator (because that's what sprintf() does).
size += sprintf(str + size, "%s ", argv[i]);

// sprintf() returns the number of bytes written (excluding the
// NUL that it added), which we use to keep track of the offset
// of the NUL terminator.
assert(str[size] == '\0');
}
// Remove trailing space.  The string is now terminated by two
// consecutive NULs.  That's OK: only the first one matters.
str[--size] = '\0';

printf("%s\n", str);
free(str);
}
• Thank you for the tip on the error logging. I was hoping someone would answer that. Also, I appreciate that you explained inefficiencies in my code. Dec 8 '14 at 0:42
• For every argument, you need to allocate enough for the argument itself, plus 1 for the space character. (I screwed that up in Rev 1.) You also need to reserve one byte at the end of everything. Dec 8 '14 at 0:51
• When you say, "reserve one byte at the end", is that what you are doing here: str[--size] = '\0' or here: size += sprintf(str + size, "%", argc[i](I think that should be argv, not argc). And, why do you set size to 0? Dec 8 '14 at 0:56
• You're right, I meant argv. I've added copious comments, which should hopefully help explain the motivation. Dec 8 '14 at 7:37
• Whether you get a segmentation fault depends on what happens to lie in the overrun region. Using unallocated memory results in undefined behaviour. Sometimes you get lucky. Dec 8 '14 at 20:41