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 2 down vote favorite

I'm try to create array of strings from a text file. I need a review from experienced programmers about my program. (MinGW gcc, Win7)

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

main()
{
FILE *file; 
char *fname = "lines.txt";
file = fopen(fname, "r");

if(file == NULL)
    printf("Can't read %s", fname);

char cbuff;

int nlines = 0; //counter of new lines
int chr = 0; //counter of chars (except new line char)

int maxlen = 0;
while( (cbuff = fgetc(file) - 0) != EOF )
{
    if(cbuff == '\n')
    {
        if (chr > maxlen)
        {
            maxlen = chr + 1;
        }

        chr = 0;
        nlines++;
    }

    else
    {
    chr++;
    }
}

printf( "lines: %d\nmax string len: %d\n\n", nlines, maxlen );
rewind(file);

char *list[nlines];

int buffsize = maxlen * sizeof(char);
char buff[buffsize];

int i = 0;
while(fgets(buff, buffsize, file))
{

    list[i] = malloc(strlen(buff) * sizeof(char));
    strcpy(list[i], buff);
    i++;
}

fclose(file);

int c = 0;
for(c; c < nlines; c++)
{
    printf( "%s", list[c] );
}
}//main

I'm trying to control size of array and items in array. Maybe I have errors somewhere, do not judge strictly.

share|improve this question
add comment

2 Answers

up vote 1 down vote

It looks like you're writing in C99. That's okay as long as your compiler supports it, but you should specify it, maybe with

#if __STDC_VERSION__ >= 199901L
// your code
#endif

Your main() routine should declare parameters for the argument count and argument list. My suggestion is that it check if there is an additional parameter and treat it as the filename if it exists, defaulting to "lines.txt" if there isn't one.

Then it should call another function with the string. (That other function opens the file, parses it, and returns a list as a char**, signalling an error [e.g. failure to open file] by returning NULL.) Finally main() should print out an error if the return was NULL, or if not, print the list, ideally by calling a second function.

As for the parsing function: It's not necessary to make two passes over the file. You can start out with your string buffer at a default size, and expand it as necessary (with realloc()) if your fgets() call ever doesn't terminate the string with '\n'. If you do still want to make two passes over the file, the passes should be in separate functions, to encapsulate the functionality (and of course a third function that calls the fopen() and rewind() and fclose() and passes the FILE * as a parameter).

In fact your method is potentially vulnerable to a race condition; if the underlying OS allows another process to modify the file being read at locations previous to the file pointer, you could end up crashing.

int is the wrong data type for bufsize, chr, maxlen. They're always going to be positive. Try size_t. Also, please pick a better variable name than chr.

You most definitely do need to take sizeof(char) when sizing the buffers.

What's the reason for the - 0 in cbuff = fgetc(file) - 0 ?

I suggest using strncpy() instead of strcpy(), especially since you know the max length already.

share|improve this answer
add comment
up vote 1 down vote

If you are truly coding in C then all variables should be declared at the start of the function. This has traditionally been how the C language worked and what C coders expect.

C has also traditionally not allowed for runtime declarations of array sizes. You should malloc the list variable.

If you initialize a loop variable then just omit it in the for loop definition. In other words don't do this:

for(c; c < foo; c++)

do this:

for( ; c < foo; c++)

A char is already a byte, no need to multiple strlen by sizeof(char).

Personally I would move most of the logic into proper functions and out of main. It is just a good habit to get into.

share|improve this answer
    
Many thanks for your answer. –  cyan May 29 at 10:33
    
The only standard of C that guarantees you the ability to have a variable-sized array (array size set at runtime) is C99. The newer standard, C11, allows compilers to implement but does not require it (you can tell by checking if a certain symbol is defined). –  Snowbody Jun 27 at 21:20
    
en.wikipedia.org/wiki/C99 also allows inline variable declarations. I strongly disagree with @Sean; C99 is definitely C and cyan can use it if he wants to. –  Snowbody Jun 27 at 21:33
    
It is not guaranteed that a char is a byte; it is not guaranteed that sizeof(char)==1. If you want your code to be portable you do need to take sizeof(char). –  Snowbody Jun 27 at 21:34
add comment

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.