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.

I implemented generic stack in C. Seems to work fine. It doesn't work for strings, though.

It was compiled using Visual C++.

Implementation:

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

struct node{
    void* value;

    node* next;
};

struct stack{
    node* first;
    int elemSize;
    int length;
};

stack* StackNew(int elemSize)
{
    stack* s = (stack*)malloc(sizeof(stack));
    s->elemSize = elemSize;
    s->length = 0;
    return s;
}

void Push(stack* s, void* value)
{
    node* oldFirst = s->first;
    s->first = (node*)malloc(sizeof(node));
    s->first->value = malloc(s->elemSize);
    memcpy(s->first->value, value, s->elemSize);
    s->first->next = oldFirst;
    s->length++;
}

void* Pop(stack* s)
{
    void* value = malloc(s->elemSize);
    memcpy(value, s->first->value, s->elemSize);
    node* oldFirst = s->first;
    s->first = s->first->next;
    free(oldFirst);
    s->length--;
    return value;
}

void Destory(stack* s)
{
    while (s->length > 0) Pop(s);
    free(s);
}

Test:

struct point{
    int x, y;
};

int main()
{
    stack* s = StackNew(sizeof(point));
    for (int i = 0; i < 100; i++){
        point pt;
        pt.x = i;
        pt.y = i * i;
        Push(s, &pt);
    }

    while (s->length > 0) {
        point pt = *(point*)Pop(s);
        printf("(%d,%d)\n", pt.x, pt.y);
    }
    Destory(s);
    return 0;
}

Output:

(99,9801)
(98,9604)
(97,9409)
(96,9216)
(95,9025)
(94,8836)
(93,8649)
(92,8464)
(91,8281)
(90,8100)
(89,7921)
(88,7744)
(87,7569)
(86,7396)
...

as expected.

Please tell me what I did wrong and how I can improve things.

share|improve this question
    
If this is C, why are you including iostream? –  EngieOP 13 hours ago
    
@EngieOP I wrote it in C++ but i'm using the C subset of C++. –  Rezo Megrelidze 13 hours ago
    
Okay, just wondering. I remember there being a Meta post about C/C++ tag mixing. –  EngieOP 13 hours ago
2  
If it compiles as C++, but would fail to compile as C, then it's still C++ code and should be tagged as such. (Someone will probably say "Why don't you just go all the way with C++?" in an answer, but that's just how it has to be.) –  200_success 12 hours ago
1  
Alternatively, just remove #include <iostream> and retag as c. –  200_success 12 hours ago

1 Answer 1

  • Naming seems inconsistent.

    Consider prefixing all exported names with Stack, e.g. StackPush, StackPop, StackDestroy (Destory is definitely a typo).

    The entity named first in a stack context is traditionally referred to as top.

  • Memory management is overengineered.

    Every time a value is pushed memory is allocated. That's justifiable for ownership reasons (make sure to document it). However every time a value is popped memory is allocated again. This is very strange. A simple

    return s->first->value;
    

    would suffice.

    Along the same line Destroy doesn't free memory (re)allocated for individual stack entries. Consider

    while(s->length > 0)
        free(Pop(s))
    
  • You've mention the problem with strings; I believe it is stemmed from elemSize being a property of the stack itself. Making it a property of an element would solve the problem.

  • As mentioned in the comments, the C code shall not include C++ headers.

share|improve this answer
    
"Making it a property of an element would solve the problem." It does solve the problem. I also had to to rewrite StackPush(Stack* s,void* value) to StackPush(Stack* s,void* value,int elemSize). –  Rezo Megrelidze 12 hours ago

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.