I have problems implementing a stack

Question

I have a question about my piece of code here: I tried to write a function, its name is take, the function can get only one int parameter and have to return back the middle number that was inserted. The function has to use in, as minimum memory as possible. I tried to use in a stack. Its my implementation. The problem is that the program doesn't return a value after the third insertion.

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

int take (int);

typedef struct stack
{
    int num;
    struct stack *next;
}stack;

stack first;

bool isit = true;
int counter = -1;

int main()
{
    printf("%d",take(5));
    printf("%d", take(6));
    printf("%d", take(7));

    return 0;
}

int take(int value)
{
    if (isit)
    {
        isit = false;
        first.num = value;
        first.next = NULL;
    }
    else
    {
        static stack newone;
        newone.num = value;
        newone.next = NULL;
        stack temp = first;
        while (temp.next != NULL)
        {
            temp = *temp.next;
        }

        temp.next = &newone;

    }

    stack *temp1 = malloc(sizeof(stack));
    *temp1 = first;
    counter++;

    if (counter > 1 && counter % 2 == 0)
    {
        temp1 = temp1->next;
    }

    return (temp1->num);
}

The only memory allocation you have done is for a single `struct`. Perhaps you are unlucky that the second insertion seems to work. I would expect to see either a fixed size stack, or the use of `realloc`. Or is this actually a linked list? — Weather Vane, Feb 04 '18 at 20:23
Here is very good advice on handling problems like that. https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ I would really like to see/read you explain each and every line of your code to yourself or anybody. I expect the result should have a few more lines of comment than your current code. Current count of comment lines: 0. That is too few for any code you have problems understanding/debugging. — Yunnosch, Feb 04 '18 at 20:35
Second good advice (actually part of the first one, but just to stress the point): https://stackoverflow.com/questions/2069367/how-to-debug-using-gdb — Yunnosch, Feb 04 '18 at 20:36

score 2 · Accepted Answer · answered Feb 04 '18 at 20:40

A big problem in your code is that you use global variables where you don't need them. This creates problems that don't expect, like this:

int take(int value)
{
    ...
        static stack newone;
        newone.num = value;
        newone.next = NULL;
        stack temp = first;
        while (temp.next != NULL)
        {
            temp = *temp.next;
        }

        temp.next = &newone;

The static stack newone is a static variable, it means it will be always the same every time you call take, you are overwriting the values all the time, specially the next pointer.

For this reason, avoid using global variables when you can perfectly declare them in the main function and pass them to the other functions.

Also you malloc part doesn't make any sense. You want minimal memory footprint but you allocate memory which is lost after temp1 = temp1->next;.

If you want a minimal memory footprint and not having to allocate memory with malloc, then you can declare an array of fixed length and use it as a stack, something like this:

typedef struct stack
{
    int stack[20];
    size_t len;
    size_t size;
} Stack;

void stack_init(Stack *stack)
{
    if(stack == NULL)
        return;

    stack->size = sizeof stack->stack / sizeof stack->stack[0];
    stack->len = 0;
}

int stack_is_empty(Stack *stack)
{
    if(stack == NULL)
        return 1;

    return stack->len == 0;
}

int stack_is_full(Stack *stack)
{
    if(stack == NULL)
        return 0;

    return stack->len == stack->size;
}

int stack_push(Stack *stack, int value)
{
    if(stack == NULL)
        return 0;

    if(stack_is_full(stack))
        return 0;

    stack->stack[stack->len++] = value;
    return 1;
}

int stack_pop(Stack *stack, int *val)
{
    if(stack == NULL)
        return 0;

    if(stack_is_empty(stack))
        return 0;

    stack->len--;
    if(val)
        *val = stack->stack[stack->len];

    return 1;
}

int take(Stack *stack, int value)
{
    if(stack == NULL)
        return 0;

    if(stack_push(stack, value) == 0)
        fprintf(stderr, "stack is full, cannot push\n");

    return stack->stack[stack->len / 2];
}

int main(void)
{
    Stack stack;

    stack_init(&stack);

    printf("%d", take(5));
    printf("%d", take(6));
    printf("%d", take(7));

    return 0;
}

I have problems implementing a stack

1 Answers1