3
\$\begingroup\$

I'm working on a school assignment, and it's working fine, but I have a feeling I'm over-using pointers, and I just can't think of a way to not pass a triple pointer to the function readFile.

I'd be more than grateful for your suggestions, and if you have any other suggestions on how to improve my code I'd be glad to hear those too!

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <errno.h>

#define handle_error(message) \
    do { perror(message); exit(EXIT_FAILURE); } while(0)
int readFile(char ***names, FILE *file, int *size);
void *_realloc(void *ptr, size_t size);

int main(void) {
    char **names;
    int names_size;
    FILE *file;
    int chars_read;
    int i;

    names = NULL;
    file = NULL;
    names_size = 0;
    chars_read = 0;

    file = fopen("names", "r");
    readFile(&names, file, &names_size);
    printf("%s\n", names[0]);
    fclose(file);
    return EXIT_SUCCESS;
}

int readFile(char ***names_p, FILE *file, int *size) {
    int chars_read;
    int line;
    int chunk_size;
    int name_size;
    int name_length;
    char cur_char;
    char **names;
    char *name;

    name = NULL;
    names = NULL;
    line = 0;
    name_length = 0;
    name_size = 0;
    chunk_size = 2;
    chars_read = 0;
    while((cur_char = fgetc(file)) != EOF && cur_char != '\r'){
        if(cur_char == '\n') {
            if(name_size > 0) {
                names = _realloc(names, sizeof(char*) * (line+1));
                names[line] = _realloc(name, name_length + 1);
                names[line][name_length] = '\0';
                name_size = 0;
                name_length = 0;
                name = NULL;
                line++;
        } 
    } else {
            if(name_size < name_length + 1) {
                name = _realloc(name, 
                    sizeof(char) * (name_size = 
                    (name_size + 1) * chunk_size));
            }
            name[name_length] = cur_char;
            name_length++;
        }
        chars_read++;
    }
    *names_p = names; 
    *size = line;
    return chars_read;
}

void *_realloc(void *ptr, size_t size) {
    void *temp = realloc(ptr, size);
    if(temp == NULL) {
        int error = errno;
        if(error == ENOMEM) printf("size: %d\n", error);
        handle_error("realloc");
    }
    return temp;
}
\$\endgroup\$
  • \$\begingroup\$ Your tripple pointer is indeed unnecessary - just return the char** from readFile \$\endgroup\$ – William Morris Jun 12 '13 at 13:40
4
\$\begingroup\$

1. Introduction

You'll see that I made a lot of comments below, but if you're just starting to learn C, then your code is not at all bad. C is a tough language to write robust code in: it provides many many ways to go wrong, and even the most expert and experienced programmers don't always succeed in avoiding the traps and pitfalls.

I like your attention to error handling, that you check the result of realloc, that you remember to close your file handle, and that you use the symbols EXIT_SUCCESS and EXIT_FAILURE to make it clear what you mean.

2. Bugs

  1. You don't check whether there were in fact any names in the file before printing the first of them. If the file was empty then this would result in undefined behaviour:

    $ clang cr27281.c
    $ cat /dev/null > names
    $ ./a.out
    Segmentation fault: 11
    
  2. You don't check the result of fopen. If this call failed then you'd end up trying to read from the file handle NULL:

    $ rm names
    $ ./a.out
    Segmentation fault: 11
    

3. Other comments on your code

  1. If you need to return multiple values from a function, then why not use a structure? You could declare a structure like this:

    typedef struct name_array_s {
        char **names;         /* Array of names, allocated with malloc. */
        size_t count;         /* Count of names in the 'name' array. */
    } *name_array_t;
    

    and then readFile could take a name_array_t as its first argument.

  2. There's no comment or documentation explaining the interface to readFile. What is this function supposed to do and how are you supposed to call it? Imagine coming back to this code in a couple of years' time when you have forgotten all about it. Do you really want to have to reverse-engineer it to figure out what it does?

  3. The variable names_size is an int, but it counts the number of entries in the names array and it can never legitimately be negative. And there are common platforms where INT_MAX is 2147483647, but where you have enough memory to store many more objects than that. So why not make names_size a size_t?

  4. Similarly for the result of the function readFile.

  5. Consider using abort() in your error handler instead of exit(EXIT_FAILURE). When running under the debugger, abort() drops you straight into the debugger prompt with a good chance of being able to find useful information in the backtrace, but exit() just quits the program, leaving you none the wiser.

  6. It's a bit restrictive that the file has to be named names. Why not take the name on the command line in argv[1]?

  7. This line:

    if(error == ENOMEM) printf("size: %d\n", error);
    

    has four problems! First, error messages should go to standard error, not (as here) standard output, so you need fprintf(stderr, ...). Second, you want size instead of error in the call to printf. Third, size is a size_t but you are using a %d format specifier which takes an int, so this results in undefined behaviour. Fourth, by putting the if and the printf on the same line, you've made it impossible (in most debuggers) to set a breakpoint on the printf. I would write:

    if(error == ENOMEM)
        fprintf(stderr, "size: %lu\n", (unsigned long)size);
    
  8. The C language allows you to combine the declaration of a variable and its initialization, so instead of writing the repetitive

    char **names;
    int names_size;
    FILE *file;
    int chars_read;
    int i;
    
    names = NULL;
    file = NULL;
    names_size = 0;
    chars_read = 0;
    

    you can write

    char **names = NULL;
    int names_size = 0;
    FILE *file = NULL;
    int chars_read = 0;
    int i;
    
  9. After making the change above, it becomes obvious that the variable i is unused. But shouldn't your compiler have complained about that? When writing a new program it's well worth turning on as many error-detecting features as you can, for example if you're using GCC it's a good idea to use a comprehensive set of flags like -ansi -pedantic -std=c99 -Wall -Werror.

  10. The variable chars_read is also unused.

  11. Having an assignment buried inside an expression seems error-prone and hard to understand (especially when broken across lines like this):

    name = _realloc(name, 
        sizeof(char) * (name_size = 
        (name_size + 1) * chunk_size));
    

    I prefer to keep the code simple:

    name_size = (name_size + 1) * chunk_size;
    name = _realloc(name, sizeof(char) * name_size));
    
  12. sizeof(char) is defined to be 1 by the C standard, so you don't need it here (it doesn't do any harm, though).

    §6.5.3.4.3 When [sizeof is] applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1.

  13. chunk_size is poorly named: it's not a size, it's an exponent that controls the exponential growth of the name buffer.

4. Performance

Your algorithm for allocation involves reallocating the names array each time a new line is reached in the file. This is wasteful, because each time this array is reallocated, the contents may have to be copied across to the new region of memory, potentially making it O(n2).

If your files are short then you may prefer not to worry about this, but after I did a simple rewrite of your readFile function to do less reallocation, I was able to get a speedup of more than 3× in test cases with many lines, like this one:

$ yes '' | head -10000000 | nl -ba > names
$ time ./names-old > /dev/null
real    0m4.136s
user    0m3.777s
sys     0m0.333s
$ time ./names-new > /dev/null
real    0m1.248s
user    0m0.946s
sys     0m0.287s
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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