current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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?

share|improve this question

3 Answers 3

up vote 5 down vote accepted

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;
}
share|improve this answer
    
Thank you for rewriting the code as an exemplar. I will read up on functional based programming languages. If I could +1, I would. –  SirPython Dec 7 '14 at 22:44
1  
Functional? I think procedural is the standard term. Functional makes me think of languages like Haskell. –  icktoofay Dec 8 '14 at 0:27
    
@icktoofay you are right, I've written that in a late hour, sorry about that. –  Fatih BAKIR 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. –  William Morris Dec 8 '14 at 16:20
up vote 4 down vote

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

share|improve this answer
    
I never thought about allocating space for the space. Thank you for bringing that up. –  SirPython Dec 8 '14 at 1:53
up vote 1 down vote

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);
}
share|improve this answer
    
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. –  SirPython 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. –  200_success♦ 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? –  SirPython Dec 8 '14 at 0:56
    
You're right, I meant argv. I've added copious comments, which should hopefully help explain the motivation. –  200_success♦ Dec 8 '14 at 7:37
1  
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. –  200_success♦ Dec 8 '14 at 20:41

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.