K&r 1-17 copying strings

[AaronP]

I'm going through the K&R examples, and I've run into a snag on 1-17.

My current code:

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

const int MAX_ARRAY_SIZE = 1000;

int
main(void)
{
    char longLines[MAX_ARRAY_SIZE][MAX_ARRAY_SIZE];
    char currentLine[MAX_ARRAY_SIZE];
    int  linesMaxIndex = 0;  
    int  currentChar;
    int  currentIndex  = 0;

    while((currentChar = getchar()) != EOF)
    {
        if((char)currentChar == '\n')
        {
            if(currentIndex >= 59)
            {
                longLines[linesMaxIndex] = strcpy(longLines[linesMaxIndex],
                        currentLine);
                linesMaxIndex++;
            }
            currentLine  = NULL;
            currentIndex = 0;
        }
        currentLine[currentIndex] = (char)currentChar;
        currentIndex++;
    }

    for(int i = 0; i <= linesMaxIndex; i++)
    {
        printf("%s\n", longLines[i]);
    }


    return 0;
}

The goal is to pipe a file to stdin and print out any line with 60 (actually, I'm reading the text now and it's 80, but that's a minor detail) characters. I get a compile error on line 21 where I try to copy a string into an array of strings. Can someone give me pointers on the correct way to do this in C?

[CommonTater]

Ok... lots of problems...

line 9 ... are you really sure you want to create an array of 1 megabyte on the program stack? It's likely too big for most OS/Compiler setups, you should be creating that monster on the heap with malloc().

line 19 ... will only store lines longer than 59 characters (is that what you want?)

line 25 ... setting a stack variable = NULL is bad juju... if you're just looking to reuse the same buffer lose this line.

line 21... you are calling strcpy() on a string that is not null terminated, you are very likely to get a lot of garbage in your strings. The trick here would be to add a line of code to replace the \n with a 0... so that your strings are correctly terminated.

if( currentChar == '\n')  
  {  
    currentLine[currentIndex] = 0;
    if (CurrentIndex > 59)
      ...

(that's untested, but it should get you pretty close)


line 32... might be printing one too many lines... test it both with and without the = sign.

Additionally...
You have no guards to stop at MAX_ARRAY_SIZE lines, which would cause a buffer overrun.

You also have no guards to stop at MAX_ARRAY_SIZE characters on a given line, which would also cause a buffer overrun.

The size of your array would be far easier to adjust if you were to split your MAX_ARRAY_SIZE into two defines, one for the MAX_LINES and one for MAX_CHARS ... this would let you trim the array to a more reasonable size...

#define MAX_LINES 1000
#define MAX_CHARS 100

char array[MAX_LINES][MAX_CHARS]

Hope this helps...

[KCfromNC]

Remove the assignment to longLines[lineMaxIndex] and just do strcpy(longLines[linesMaxIndex], currentLine). It's almost never the right idea to use the return value of strcpy,strcat or similar functions, except maybe as arguments to another call to the same function(*).

Also, rethink how you're doing this. What happens when you have more than a thousand lines longer than 80 characters? Why the need to make copies of the long lines and then print them out later?

* - mainly to do really ugly things like strcat(strcat(strcat(foo, "a"), "b"), "c") for people who just couldn't get enough of Lisp.

[stahta01]

* - mainly to do really ugly things like strcat(strcat(strcat(foo, "a"), "b"), "c") for people who just couldn't get enough of Lisp.

I am glad that I most likely will never have to code with Lisp.
I just "cdr" and "car" it anymore.

Tim S.

[AaronP]

Ah, thanks to both Tater and KC, I've got it working. Yeah I know, it's not the best way to do things, but I've already put off enough exercises for until I review pointers that I want to make sure I can at least do the basic string manipulation stuff.