3
\$\begingroup\$

I work in a project based on C language. I want to read a file that contains two lines of numeric characters.

Example

9
8 9 5456 32 2 45 34 98 5

I want to pass the elements of the second line into an array. As you saw the first line tell us how big needs to be the array. In the above example we want to create an array with size of 9 to hold 9 numbers.

I created a code that does that exactly but it's too complicated and I want to make it more flexible.

The logic of my code it the following:

I try to read the entire file in one go into a single char*, then splits that by line into an array of char* and then lastly converts each line to an int by calling atoi.

The function count_lines() counts the number of words and enter key(new line). The function is_linebreaks(p) checks if the character is a space (' '), an enter key (\n) or a carriage return (\r). If this condition is true, it increment rows counter and eats the other breaks. Eating other breaks means, for example, that if you had multiple spaces (' ') it would skip them until the next valid character (or one of the other breaks).

So, if number_of_rows (in the main function) is not actually rows, but number of words, each line will have one word!

I created the code just to hold 2.000.000 numbers as fast as possible so i want to keep the same logic of the code. But if you have a better idea to store so many numbers so fast please proceed to expain.

The problem is that in the second linespace, every element is one space separated so I don't need to kill all the whitespaces. Also I have only two line in the file.

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

int is_end(char* input) {
    return *input == 0;
}

int is_linebreak(char* input) {
    return *input == '\r' || *input == '\n' || *input == ' ';
}

char* eat_linebreaks(char* input) {
    while (is_linebreak(input))
        ++input;

    return input;
}

size_t count_lines(char* input) {
    char* p = input;
    size_t rows = 1;

    if (is_end(p))
        return 0;

    while (!is_end(p)) {
        if (is_linebreak(p)) {
            ++rows;
            p = eat_linebreaks(p);
        }
        else {
            ++p;
        }
    }
    return rows;
}

/* split string by lines */
char** get_lines(char* input, size_t line_count) {
    char* p = input;
    char* from = input;
    size_t length = 0;
    size_t line = 0;
        int i;
    char** lines = (char**)malloc(line_count * sizeof(char*));

    do {
        if (is_end(p) || is_linebreak(p)) {
            lines[line] = (char*)malloc(length + 1);
            for (i = 0; i < length; ++i)
                lines[line][i] = *(from + i);

            lines[line][length] = 0;
            length = 0;
            ++line;
            p = eat_linebreaks(p);
            from = p;

        }
        else {
            ++length;
            ++p;
        }
    } while (!is_end(p));

    // Copy the last line as well in case the input doesn't end in line-break
    lines[line] = (char*)malloc(length + 1);
    for (i = 0; i < length; ++i)
        lines[line][i] = *(from + i);

    lines[line][length] = 0;
    ++line;


    return lines;
}

int main(int argc, char* argv[]) {
    clock_t start;
    unsigned long microseconds;
    float seconds;
    char** lines;
    size_t size;
    size_t number_of_rows;
    int count;
    int* my_array;
    start = clock();

    FILE *stream;
    char *contents;
    int fileSize = 0;
        int i;

    // Open file, find the size of it
    stream = fopen(argv[1], "rb");
    fseek(stream, 0L, SEEK_END);
    fileSize = ftell(stream);
    fseek(stream, 0L, SEEK_SET);

    // Allocate space for the entire file content
    contents = (char*)malloc(fileSize + 1);

    // Stream file into memory
    size = fread(contents, 1, fileSize, stream);
    contents[size] = 0; 
    fclose(stream);

    // Count rows in content
    number_of_rows = count_lines(contents);

    // Get array of char*, one for each line
    lines = get_lines(contents, number_of_rows);

    // Get the numbers out of the lines
    count = atoi(lines[0]); // First row has count
    my_array = (int*)malloc(count * sizeof(int));
    for (i = 0; i < count; ++i) {
        my_array[i] = atoi(lines[i + 1]);
    }

    microseconds = clock() - start;
    seconds = microseconds / 1000000.0f;
    printf("Took %fs", seconds);


    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Why do you want to know the number of rows? You are just reading one line of numbers. So you really only care about the first two lines. \$\endgroup\$ – Martin York Dec 5 '14 at 3:18
  • \$\begingroup\$ thats my problem. I can't update the my code making it to read only the first two lines! (upvoted comment) \$\endgroup\$ – Nikos KLon Dec 5 '14 at 11:56
1
\$\begingroup\$

Here are some things that may help you improve your code:

Be careful with signed and unsigned

In two cases, the code compares an int i with an unsigned size_t length. It would be better to declare i to also be size_t.

Think of the user

What happens when the user does not specify a file name on the command line? The answer on my machine is that I get a segmentation fault and a crash. Not nice. It's easily solved by checking argc to make sure that the user has specified at least one argument and by checking to make sure that fopen has not returned NULL.

Free memory that you allocate

This program leaks memory because you have allocated space using malloc but never released with free. That's bad practice.

Carefully consider whether memory allocation is required

Reading the entire file into memory is not really a necessary step for this algorithm. Better would be to read from the file using something like fscanf since the file is probably buffered in memory anyway. This would make your code more flexible and eliminate the need for both memory allocation for the file contents and the count_lines routine.

Don't use unnecessary #includes

The code has #include <assert.h> but nothing from that include file is actually in the code. For that reason, that #include should be eliminated.

Handle error conditions

Whenever one works with file I/O or memory allocation, there is a chance these operations can fail. For that reason, all such operations should not only be checked for errors, but also the program should handle those errors in some appropriate way. For instance, what if the input file claims there are 9 numbers but there are actually on 4 in the file? Your program will need to handle that.

\$\endgroup\$
  • \$\begingroup\$ i understand your point. First this is a project and the employe told us not to take any restrictions or error conditions. So don't worry about that. Second, I didn't use fscanf cause I want the procidure to be done in less than 1.5 seconds and with fscanf I would need aproximately 3 - 4 seconds. I want to keep the structure of the code but to make it cleaner so the user can understand what is going on! :) \$\endgroup\$ – Nikos KLon Dec 3 '14 at 14:50
  • \$\begingroup\$ there are parts in my code that I don't need. So I wrote a code that isn't tided up. That's why i post it here. I don't have any other idea to think. \$\endgroup\$ – Nikos KLon Dec 3 '14 at 14:52
  • \$\begingroup\$ @Opo: you should test your assumption about fscanf being "too slow." On my machine, using an input file with 10 million random numbers, your program takes 1.49 seconds and leaks 210 megabytes. My modified version, using fscanf and with all error handling, takes 0.99 seconds and leaks no memory. \$\endgroup\$ – Edward Dec 3 '14 at 15:15
  • \$\begingroup\$ so what have yout try? Your modified version? Also notice that the maximum memory I can use is 64 MB \$\endgroup\$ – Nikos KLon Dec 3 '14 at 15:31

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.