0

I have the following code:

typedef struct element{
    int value;
    int numberOfValues;
    struct element * next;
} Element;

static int setnum = 0; // this is a key that identifies the set
int s, i; // s is input by the user to identify a set | i is a loop counter
Element * prev, * curr;

Element * larray[MAXSETS];

And then, a function that allows the user to create a set of elements:

int create(void)
{
    if (setnum > MAXSETS)
        printf("Error. Maximum value of sets reached.\n");
    else
        {
            setnum++;
            larray[setnum] = (Element *) malloc(sizeof(Element));
            larray[setnum]->next = NULL;
            larray[setnum]->numberOfValues = 0;
            // larray[setnum] practically becomes the head
        }

    return setnum;

}

And an add function:

void add(int s, int x)
{
static Element * hold = NULL;

printf("Hello!\n");

if (larray[s]->numberOfValues >= MAXELEMENTS) // the set must be finite
{
    printf("Error. Set is full.\n");
}
else
    if (larray[s]->numberOfValues == 0)
    {
        larray[s]->value = x;
        larray[s]->numberOfValues++;
        hold = larray[s];
    }
    else
    {
        prev = (Element *) malloc(sizeof(Element));
        prev = hold;
        prev->next = hold;
        hold->next = NULL;
        hold->value = x;
        printf("Done!\n");
        larray[s]->numberOfValues++;
    }
printf("Bye!\n");
}

And finally, a display function:

void display(int s)
{
    curr = larray[s];
    for (i = 0; larray[s]->numberOfValues; i++)
    {
        printf("%d\n", larray[s]->value);
        larray[s] = larray[s]->next;
    }
}

However, when I pass these functions in a drive, only 1 number is displayed. Either the display function is wrong, or the add function is wrong, but I can't seem to get the problem? Any ideas?

Thanks.

3
  • 2
    prev = malloc(...) followed by prev = hold is very sospicious Commented Jan 12, 2014 at 12:42
  • hold is declared as static in the function? are you sure you want it that way?? it will not initialized for every function call? Commented Jan 12, 2014 at 13:01
  • Well, I'm trying to make hold remember the contents of the last added element, that's why. Commented Jan 12, 2014 at 13:12

3 Answers 3

1

The pointer assignments are incorrect.

prev is assigned to a malloc of memory. Then prev is assigned to hold. So the newly allocated memory is lost.

prev = (Element *) malloc(sizeof(Element));
prev = hold;
Sign up to request clarification or add additional context in comments.

Comments

1

Your code has more than one problem. First of all, note the create function:

else
    {
        setnum++;
        larray[setnum] = (Element *) malloc(sizeof(Element));
        larray[setnum]->next = NULL;
        larray[setnum]->numberOfValues = 0;
        // larray[setnum] practically becomes the head
    }

I'd say you want to increment setnum only after assigning to larray[setnum], otherwise, you will never use position larray[0]. Also, when setnum == MAXSETS, you will be accessing an out-of-bounds position (this is an off-by-one error). Thus, the test should check for setnum >= MAXSETS. create should look like this instead:

int create(void)
{
    if (setnum >= MAXSETS)
        printf("Error. Maximum value of sets reached.\n");
    else
        {
            larray[setnum] = (Element *) malloc(sizeof(Element));
            larray[setnum]->next = NULL;
            larray[setnum]->numberOfValues = 0;
            setnum++;
        }

    return setnum;
}

The add function is wrong. This piece of code is, for sure, not what you want:

else
{
    prev = (Element *) malloc(sizeof(Element));
    prev = hold;
    prev->next = hold;
    hold->next = NULL;
    hold->value = x;
    printf("Done!\n");
    larray[s]->numberOfValues++;
}

At this point, hold will point to NULL, after the assignment prev = hold;, you leak memory, because you lose reference to the memory you allocated immediately before, and, worse than that, you dereference a null pointer in the following statements. I suggest rewriting this function from scratch.

The display function won't work the way you expect. The for loop leaks memory in every iteration because of this assignment:

larray[s] = larray[s]->next;

Do you really want to destroy the set after displaying it? Also, the loop condition is wrong. When you add an element, you always increment numberOfValues in the first element on that array position, and you don't use numberOfValues in the other elements past the first. However, the loop condition to stop is that larray[s]->numberOfValues == 0, which, combined with the assignment on the previous iteration, makes your program behave completely arbitrarily, since larray[s]->numberOfValues was never initialized and will have some garbage value.

1 Comment

Thanks for the create() problem, I'll fix that. As regards add(), I'll try working sth out again though not sure really, I'd like to add elements to the set but it's like they're being replaced.
1

this code has too much problem ,  I am not sure what you are suppose to do with this code,but I change part of your code, and test it with simple case, it works , hope it will be useful here

#define MAXSETS 5
#define MAXELEMENTS 3
#include <stdio.h>
#include <stdlib.h>
typedef struct element{
    int value;
    int numberOfValues;
    struct element * next;
} Element;


static int setnum = 0; // this is a key that identifies the set
int s, i; // s is input by the user to identify a set | i is a loop counter
Element * prev, * curr;

Element * larray[MAXSETS];

int create(void)
{
    while (setnum < MAXSETS)
        {

            larray[setnum] = (Element *) malloc(sizeof(Element));
            larray[setnum]->next = NULL;
            larray[setnum]->numberOfValues = 0;
            setnum++;
            // larray[setnum] practically becomes the head
        }
     printf("%d\n", setnum);
    return setnum;
}


void add(int s, int x)
{
static Element * hold = NULL;


if (larray[s]->numberOfValues >= MAXELEMENTS) // the set must be finite
{
    printf("Error. Set is full.\n");
}
else
    if (larray[s]->numberOfValues == 0)
    {
        larray[s]->value = x;
        larray[s]->numberOfValues++;
        printf("numberOfValues: %d\n", larray[s]->numberOfValues);
        hold = larray[s];
    }
    else
    {
        hold = larray[s];
        for(int i = 1; i < larray[s]->numberOfValues;i++)
        {
            hold = hold->next;
        }
        prev = (Element *) malloc(sizeof(Element));
        prev->next = hold;
        prev->value = x;
        larray[s]->numberOfValues++;
        printf("numberOfValues: %d\n", larray[s]->numberOfValues);
        hold->next = prev;
    }

}


void display(int s)
{
    curr = larray[s];
    for (i = 0; i < larray[s]->numberOfValues; i++)
    {
        printf("%d\n", curr->value);
        curr = curr->next;
    }
}

int main()
{
    create();
    add(0,0);
    add(0,1);
    add(0,2);
    display(0);

}

1 Comment

I basically want the user to create a set of elements, then, using linked lists, add elements to the set. Thanks for the code, I'll look into it!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.