Problem with array of pointers

[brimble2010]

Hi (first post :) ),
I have a coursework where we have to read in 50 lines from a file (bubble.txt), sort them into length order and then print them out (its not the whole coursework, just a part). I thought that the only way of holding lines of text in an array is to hold a pointer to each char array in an array called strings.

For some reason (I must admit, pointer do confuse me a bit), whenever I run this code below (just to make sure the lines are in the array) i keep getting the last line (line 50) printed 50 times and I can't tell why.

Any help appreciated.

Kristian Brimble

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

char *strings[50];

void readLines(void);
void printLines(void);

int main()
{
    readLines();
    printLines();
    return 0;
}

void readLines(void)
{
    FILE *inputFilePtr;
    inputFilePtr = fopen("bubble.txt", "r");
    char line[1000];
    
    for(int i = 0; i < 50; i++)
    {
        fgets(line, 1000, inputFilePtr);
        char *linep = malloc(sizeof(char) * 1000);
        linep = (char *)&line;
        strings[i] = linep;
    }
}

void printLines(void)
{
    for(int i = 0; i < 50; i++)
    { 
        printf("%s", strings[i]);
    }
}

[Salem]

> linep = (char *)&line;
Try strcpy() instead.

[brimble2010]

> linep = (char *)&line;
Try strcpy() instead.

worked a treat, thank you.

any chance you could explain what I was doing and what strcpy() does differently, just for future reference?

thanks again

[brimble2010]

also, i have added the bubble sort to the code but it isnt swapping the pointers stored in the array and i know it is something to do with the swap method, but i just cant see what it is.

again any help very much appreciated

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

char *strings[50];

void readLines(void);
void sortLines(void);
void swapLines(int j, int k);
void printLines(void);

int main()
{
    readLines();
    sortLines();
    printLines();
    return 0;
}

void readLines(void)
{
    FILE *inputFilePtr;
    inputFilePtr = fopen("bubble.txt", "r");
    char line[1000];
    
    for(int i = 0; i < 50; i++)
    {
        fgets(line, 1000, inputFilePtr);
        char *linep = malloc(sizeof(char) * 1000);
        strcpy(linep, line);
        strings[i] = linep;
    }
}

void sortLines(void)
{
    for(int i = 0; i < 50; i++)
    {
        for(int j = 0, k = 0; k < 50; j++, k++)
        {
            int jLen = strlen(strings[j]);
            int kLen = strlen(strings[k]);
            if(jLen >= kLen)
            {
                swapLines(j, k);
            }
        }
    }
}

void swapLines(int j, int k)
{
    char *temp = malloc(sizeof(char) * 1000);
    
    temp = strings[j];
    strings[j] = strings[k];
    strings[k] = temp;
}   

void printLines(void)
{
    for(int i = 0; i < 50; i++)
    { 
        printf("%s", strings[i]);
    }
}

[Aia]

>any chance you could explain what I was doing and what strcpy() does differently, just for future reference?

char *linep = malloc(sizeof(char) * 1000);

malloc sets apart the memory you specified. And you keep track of that memory via the pointer linep

But that pointer can be reassigned to point to some other piece of memory. That's what you are forcing it to do here.

linep = (char *)&line;

I said forcing it, because you are casting it.
Like this linep = &line ; would have gave you an error.
What where you pointing to? To the memory that fgets() used to store the last read string. line[1000]; And then, you stored the same location address, in fifty different pointers. You accomplished that with strings[i] = linep; .
When you printf'ed each one of these pointers, all of them yielded the same address, therefore, the last string line read was displayed.

Moral of the story? A pointer is not a string. A string is not a variable.
There's not such a thing as string variable in C, you make a string by joining together many single char variables and terminating it with the '\0'. That's all that keeps the string as a string. A fragile single '\0'.
strcpy() knows how to copy each and every one of this single chars terminating it with '\0', into a piece of memory that you have allocated before hand.

[brimble2010]

What where you pointing to? To the memory that fgets() used to store the last read string. line[1000]; And then, you stored the same location address, in fifty different pointers. You accomplished that with strings[i] = linep; .
When you printf'ed each one of these pointers, all of them yielded the same address, therefore, the last string line read were displayed.

Thanks Aia, cleared up a lot for me, esp the quoted part :)

Glad i signed up now :)

[William Hemsworth]

void swapLines(int j, int k)
{
  char *temp = malloc(sizeof(char) * 1000);
  temp = strings[j]; // What happened to the 1000 chars you just allocated?
  strings[j] = strings[k];
  strings[k] = temp;
}

Memory leak!

Remember to free any data you allocate using the free function.

[Aia]

A few other things you need to learn before you can make that piece of code work properly.

Every time you are about to allocate dynamic memory (like with malloc), you need to keep in mind two things. First, you need to check that the setting apart of that memory was successful.

char *linep = malloc(sizeof(char) * 1000);
if ( linep == NULL) {
     /* if you got here, memory was not successfully allocated */
    /* you need to handle the error in a proper way, but you can't */
    /* count with that memory */
}

And second. You need to free that memory when you are finished, using it.

free(linep);

These two things are a must, every time you make use of dynamic memory.

As a bonus. I can let you know about a third concept that you need to be aware.
If you reassign a pointer from dynamic memory like linep = line; (that would have been the correct way of doing linep = (char *)&line; ), that memory previously allocated is lost for you to use. You don't have a way of getting to it, and it will stay allocated to the program until the program is closed, if the operating system is smart to release it, afterward.
As you can see, if you put that in a loop, you could waste a lot of memory pretty easily.

inputFilePtr = fopen("bubble.txt", "r");

When you want to open a file to work on it or read from it, you need to check that it was successful in doing so. Kind of the same that with malloc(). You computer can not work with that file if it could not open it, can't it?

[brimble2010]

thanks for all the advice everyone, helps no end :)