Implementing a stack using a linked-list

up vote
2
down vote

favorite

In my intro CS class we're reviewing data structures. I'm currently working on implementing a stack using a linked list (LIFO) in C. I'd appreciate a review of the implementation as well as of my understanding of how a stack should work.

// This program is implementation of stack data structure
// via linked list (LAST IN FIRST OUT STRUCTURE) LIFO

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

typedef struct node

 char string[20];
 struct node *next;
 node;

node *push(char *element, node *head);
node *pop(node *head);
void destroy_stack(node *p);

int main(void)



 // create new stack
 node *stack = NULL;

 // push 6 "functions" to the stack
 char *function[6] = "first funct", "second funct", "third funct",
 "fourth funct", "fifth funct", "sixt funct"; 
 for (int i = 0; i < 6; i++)
 
 printf("function is : %sn",function[i]);
 stack = push(function[i], stack);
 if (!stack)
 
 fprintf(stderr,"Not enough memory space for new list");
 return 1;
 
 


 // display the stack
 for (node *temp = stack; temp != NULL; temp = temp -> next)
 

 printf("Elements of the stack are: %sn", temp -> string);
 

 // pop the elements from the stack
 while (stack != NULL)
 

 printf("Popped elemet is: %sn", stack -> string);
 stack = pop(stack);


 

 destroy_stack(stack);
 return 0;

 

node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 // if stack is empty
 if (head == NULL)
 
 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;


node *pop(node * head)

 // create a new head
 node *newHead = head->next;

 // pop the element from stack
 free(head);

 return newHead;



void destroy_stack(node *p)

 if ( p == NULL )
 
 return;
 
 else
 
 destroy_stack(p -> next);
 
 free(p);

edited 1 hour ago

Dannnno

5,4321749

asked 3 hours ago

Ivan Stimac

111

New contributor

add a commentÂ |Â

up vote
2
down vote

favorite

// This program is implementation of stack data structure
// via linked list (LAST IN FIRST OUT STRUCTURE) LIFO

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

typedef struct node

 char string[20];
 struct node *next;
 node;

node *push(char *element, node *head);
node *pop(node *head);
void destroy_stack(node *p);

int main(void)



 // create new stack
 node *stack = NULL;

 // push 6 "functions" to the stack
 char *function[6] = "first funct", "second funct", "third funct",
 "fourth funct", "fifth funct", "sixt funct"; 
 for (int i = 0; i < 6; i++)
 
 printf("function is : %sn",function[i]);
 stack = push(function[i], stack);
 if (!stack)
 
 fprintf(stderr,"Not enough memory space for new list");
 return 1;
 
 


 // display the stack
 for (node *temp = stack; temp != NULL; temp = temp -> next)
 

 printf("Elements of the stack are: %sn", temp -> string);
 

 // pop the elements from the stack
 while (stack != NULL)
 

 printf("Popped elemet is: %sn", stack -> string);
 stack = pop(stack);


 

 destroy_stack(stack);
 return 0;

 

node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 // if stack is empty
 if (head == NULL)
 
 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;


node *pop(node * head)

 // create a new head
 node *newHead = head->next;

 // pop the element from stack
 free(head);

 return newHead;



void destroy_stack(node *p)

 if ( p == NULL )
 
 return;
 
 else
 
 destroy_stack(p -> next);
 
 free(p);

edited 1 hour ago

Dannnno

5,4321749

asked 3 hours ago

Ivan Stimac

111

New contributor

add a commentÂ |Â

up vote
2
down vote

favorite

// This program is implementation of stack data structure
// via linked list (LAST IN FIRST OUT STRUCTURE) LIFO

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

typedef struct node

 char string[20];
 struct node *next;
 node;

node *push(char *element, node *head);
node *pop(node *head);
void destroy_stack(node *p);

int main(void)



 // create new stack
 node *stack = NULL;

 // push 6 "functions" to the stack
 char *function[6] = "first funct", "second funct", "third funct",
 "fourth funct", "fifth funct", "sixt funct"; 
 for (int i = 0; i < 6; i++)
 
 printf("function is : %sn",function[i]);
 stack = push(function[i], stack);
 if (!stack)
 
 fprintf(stderr,"Not enough memory space for new list");
 return 1;
 
 


 // display the stack
 for (node *temp = stack; temp != NULL; temp = temp -> next)
 

 printf("Elements of the stack are: %sn", temp -> string);
 

 // pop the elements from the stack
 while (stack != NULL)
 

 printf("Popped elemet is: %sn", stack -> string);
 stack = pop(stack);


 

 destroy_stack(stack);
 return 0;

 

node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 // if stack is empty
 if (head == NULL)
 
 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;


node *pop(node * head)

 // create a new head
 node *newHead = head->next;

 // pop the element from stack
 free(head);

 return newHead;



void destroy_stack(node *p)

 if ( p == NULL )
 
 return;
 
 else
 
 destroy_stack(p -> next);
 
 free(p);

edited 1 hour ago

Dannnno

5,4321749

asked 3 hours ago

Ivan Stimac

111

New contributor

// This program is implementation of stack data structure
// via linked list (LAST IN FIRST OUT STRUCTURE) LIFO

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

typedef struct node

 char string[20];
 struct node *next;
 node;

node *push(char *element, node *head);
node *pop(node *head);
void destroy_stack(node *p);

int main(void)



 // create new stack
 node *stack = NULL;

 // push 6 "functions" to the stack
 char *function[6] = "first funct", "second funct", "third funct",
 "fourth funct", "fifth funct", "sixt funct"; 
 for (int i = 0; i < 6; i++)
 
 printf("function is : %sn",function[i]);
 stack = push(function[i], stack);
 if (!stack)
 
 fprintf(stderr,"Not enough memory space for new list");
 return 1;
 
 


 // display the stack
 for (node *temp = stack; temp != NULL; temp = temp -> next)
 

 printf("Elements of the stack are: %sn", temp -> string);
 

 // pop the elements from the stack
 while (stack != NULL)
 

 printf("Popped elemet is: %sn", stack -> string);
 stack = pop(stack);


 

 destroy_stack(stack);
 return 0;

 

node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 // if stack is empty
 if (head == NULL)
 
 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;


node *pop(node * head)

 // create a new head
 node *newHead = head->next;

 // pop the element from stack
 free(head);

 return newHead;



void destroy_stack(node *p)

 if ( p == NULL )
 
 return;
 
 else
 
 destroy_stack(p -> next);
 
 free(p);

c linked-list stack

edited 1 hour ago

Dannnno

5,4321749

asked 3 hours ago

Ivan Stimac

111

New contributor

edited 1 hour ago

Dannnno

5,4321749

asked 3 hours ago

Ivan Stimac

111

New contributor

edited 1 hour ago

Dannnno

5,4321749

edited 1 hour ago

Dannnno

5,4321749

edited 1 hour ago

Dannnno

5,4321749

asked 3 hours ago

Ivan Stimac

111

New contributor

asked 3 hours ago

Ivan Stimac

111

asked 3 hours ago

Ivan Stimac

111

New contributor

Ivan Stimac is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.

add a commentÂ |Â

3 Answers
3

active

oldest

votes

up vote
1
down vote

Redundant Code

In your push() function, you have these two cases which are redundant:

// if stack is empty
if (head == NULL)

 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;


strcpy(temp -> string, element);
temp -> next = head;
return temp;

You can just remove the special case for head == NULL and be left with this, which handles both cases:

strcpy(temp -> string, element);
temp -> next = head;
return temp;

Better to iterate than recurse

Your destroy_stack() function is recursive, and that could be a problem if the stack has millions of elements because it could cause your program stack to overflow. It is easy enough to write an iterative version of that function instead, like this:

void destroy_stack(node *p)

 while (p != NULL)
 
 node *next = p->next;
 free(p);
 p = next;

Minor issues

In push(), using strcpy() is unsafe because element might exceed 20 characters and you will overflow your buffer.

In push(), if malloc() returns NULL, you return NULL and thus destroy/leak the rest of the stack. Perhaps you should just return head instead, to preserve the rest of the stack.

In pop(), you might want to check for head == NULL, otherwise you must depend on the caller to never pass NULL to your function.

answered 2 hours ago

JS1

27.1k32876

Stop typing that fast!
â€“Â vnp
2 hours ago

add a commentÂ |Â

up vote
1
down vote

push handles both branches in a curiously similar way. The only difference is that temp->next becomes NULL when head is NULL, and head otherwise. You should see the redundancy. It always becomes head no matter what head has been before. Consolidate the branches:
```
node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;
```

Failure to allocate a node results in the entire list being lost. It seems a bit too drastic.

I do not endorse a recursive solution when a straightforward iterative one is available. Consider rewriting destroy_stack iteratively.

edited 12 mins ago

answered 2 hours ago

vnp

37.4k12993

Shouldn't temp->next = head not NULL?
â€“Â Dannnno
13 mins ago

@Dannnno Whoops. Thank you. Fixed.
â€“Â vnp
12 mins ago

add a commentÂ |Â

up vote
0
down vote

You really need to create a walk function. The code you have in main to display the contents 'knows' the internal structure of your stack. Instead you need a function that returns the next item on the stack given a current item

edited 12 mins ago

Dannnno

5,4321749

answered 2 hours ago

pm100

41625

add a commentÂ |Â

Your Answer

StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);

);

Ivan Stimac is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f205176%2fimplementing-a-stack-using-a-linked-list%23new-answer', 'question_page');

);

Post as a guest

Name

3 Answers
3

active

oldest

votes

3 Answers
3

active

oldest

votes

up vote
1
down vote

Redundant Code

In your push() function, you have these two cases which are redundant:

// if stack is empty
if (head == NULL)

 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;


strcpy(temp -> string, element);
temp -> next = head;
return temp;

You can just remove the special case for head == NULL and be left with this, which handles both cases:

strcpy(temp -> string, element);
temp -> next = head;
return temp;

Better to iterate than recurse

void destroy_stack(node *p)

 while (p != NULL)
 
 node *next = p->next;
 free(p);
 p = next;

Minor issues

In push(), using strcpy() is unsafe because element might exceed 20 characters and you will overflow your buffer.

In push(), if malloc() returns NULL, you return NULL and thus destroy/leak the rest of the stack. Perhaps you should just return head instead, to preserve the rest of the stack.

In pop(), you might want to check for head == NULL, otherwise you must depend on the caller to never pass NULL to your function.

answered 2 hours ago

JS1

27.1k32876

Stop typing that fast!
â€“Â vnp
2 hours ago

add a commentÂ |Â

up vote
1
down vote

Redundant Code

In your push() function, you have these two cases which are redundant:

// if stack is empty
if (head == NULL)

 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;


strcpy(temp -> string, element);
temp -> next = head;
return temp;

You can just remove the special case for head == NULL and be left with this, which handles both cases:

strcpy(temp -> string, element);
temp -> next = head;
return temp;

Better to iterate than recurse

void destroy_stack(node *p)

 while (p != NULL)
 
 node *next = p->next;
 free(p);
 p = next;

Minor issues

In push(), using strcpy() is unsafe because element might exceed 20 characters and you will overflow your buffer.

In push(), if malloc() returns NULL, you return NULL and thus destroy/leak the rest of the stack. Perhaps you should just return head instead, to preserve the rest of the stack.

In pop(), you might want to check for head == NULL, otherwise you must depend on the caller to never pass NULL to your function.

answered 2 hours ago

JS1

27.1k32876

Stop typing that fast!
â€“Â vnp
2 hours ago

add a commentÂ |Â

up vote
1
down vote

Redundant Code

In your push() function, you have these two cases which are redundant:

// if stack is empty
if (head == NULL)

 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;


strcpy(temp -> string, element);
temp -> next = head;
return temp;

You can just remove the special case for head == NULL and be left with this, which handles both cases:

strcpy(temp -> string, element);
temp -> next = head;
return temp;

Better to iterate than recurse

void destroy_stack(node *p)

 while (p != NULL)
 
 node *next = p->next;
 free(p);
 p = next;

Minor issues

In push(), using strcpy() is unsafe because element might exceed 20 characters and you will overflow your buffer.

In push(), if malloc() returns NULL, you return NULL and thus destroy/leak the rest of the stack. Perhaps you should just return head instead, to preserve the rest of the stack.

In pop(), you might want to check for head == NULL, otherwise you must depend on the caller to never pass NULL to your function.

answered 2 hours ago

JS1

27.1k32876

Redundant Code

In your push() function, you have these two cases which are redundant:

// if stack is empty
if (head == NULL)

 strcpy(temp -> string, element);
 temp -> next = NULL;
 return temp;


strcpy(temp -> string, element);
temp -> next = head;
return temp;

You can just remove the special case for head == NULL and be left with this, which handles both cases:

strcpy(temp -> string, element);
temp -> next = head;
return temp;

Better to iterate than recurse

void destroy_stack(node *p)

 while (p != NULL)
 
 node *next = p->next;
 free(p);
 p = next;

Minor issues

In push(), using strcpy() is unsafe because element might exceed 20 characters and you will overflow your buffer.

In push(), if malloc() returns NULL, you return NULL and thus destroy/leak the rest of the stack. Perhaps you should just return head instead, to preserve the rest of the stack.

In pop(), you might want to check for head == NULL, otherwise you must depend on the caller to never pass NULL to your function.

answered 2 hours ago

JS1

27.1k32876

answered 2 hours ago

JS1

27.1k32876

answered 2 hours ago

JS1

27.1k32876

answered 2 hours ago

JS1

27.1k32876

Stop typing that fast!
â€“Â vnp
2 hours ago

add a commentÂ |Â

Stop typing that fast!
â€“Â vnp
2 hours ago

Stop typing that fast!
â€“Â vnp
2 hours ago

add a commentÂ |Â

up vote
1
down vote

push handles both branches in a curiously similar way. The only difference is that temp->next becomes NULL when head is NULL, and head otherwise. You should see the redundancy. It always becomes head no matter what head has been before. Consolidate the branches:
```
node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;
```

Failure to allocate a node results in the entire list being lost. It seems a bit too drastic.

I do not endorse a recursive solution when a straightforward iterative one is available. Consider rewriting destroy_stack iteratively.

edited 12 mins ago

answered 2 hours ago

vnp

37.4k12993

Shouldn't temp->next = head not NULL?
â€“Â Dannnno
13 mins ago

@Dannnno Whoops. Thank you. Fixed.
â€“Â vnp
12 mins ago

add a commentÂ |Â

up vote
1
down vote

push handles both branches in a curiously similar way. The only difference is that temp->next becomes NULL when head is NULL, and head otherwise. You should see the redundancy. It always becomes head no matter what head has been before. Consolidate the branches:
```
node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;
```

Failure to allocate a node results in the entire list being lost. It seems a bit too drastic.

I do not endorse a recursive solution when a straightforward iterative one is available. Consider rewriting destroy_stack iteratively.

edited 12 mins ago

answered 2 hours ago

vnp

37.4k12993

Shouldn't temp->next = head not NULL?
â€“Â Dannnno
13 mins ago

@Dannnno Whoops. Thank you. Fixed.
â€“Â vnp
12 mins ago

add a commentÂ |Â

up vote
1
down vote

push handles both branches in a curiously similar way. The only difference is that temp->next becomes NULL when head is NULL, and head otherwise. You should see the redundancy. It always becomes head no matter what head has been before. Consolidate the branches:
```
node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;
```

Failure to allocate a node results in the entire list being lost. It seems a bit too drastic.

I do not endorse a recursive solution when a straightforward iterative one is available. Consider rewriting destroy_stack iteratively.

edited 12 mins ago

answered 2 hours ago

vnp

37.4k12993

push handles both branches in a curiously similar way. The only difference is that temp->next becomes NULL when head is NULL, and head otherwise. You should see the redundancy. It always becomes head no matter what head has been before. Consolidate the branches:
```
node *push(char *element, node *head)

 // create space for new element on stack
 node *temp = malloc(sizeof(node));
 if (!temp)
 
 return NULL;
 

 strcpy(temp -> string, element);
 temp -> next = head;
 return temp;
```

Failure to allocate a node results in the entire list being lost. It seems a bit too drastic.

I do not endorse a recursive solution when a straightforward iterative one is available. Consider rewriting destroy_stack iteratively.

edited 12 mins ago

answered 2 hours ago

vnp

37.4k12993

edited 12 mins ago

answered 2 hours ago

vnp

37.4k12993

answered 2 hours ago

vnp

37.4k12993

answered 2 hours ago

vnp

37.4k12993

Shouldn't temp->next = head not NULL?
â€“Â Dannnno
13 mins ago

@Dannnno Whoops. Thank you. Fixed.
â€“Â vnp
12 mins ago

add a commentÂ |Â

Shouldn't temp->next = head not NULL?
â€“Â Dannnno
13 mins ago

@Dannnno Whoops. Thank you. Fixed.
â€“Â vnp
12 mins ago

Shouldn't temp->next = head not NULL?
â€“Â Dannnno
13 mins ago

@Dannnno Whoops. Thank you. Fixed.
â€“Â vnp
12 mins ago

add a commentÂ |Â

up vote
0
down vote

edited 12 mins ago

Dannnno

5,4321749

answered 2 hours ago

pm100

41625

add a commentÂ |Â

up vote
0
down vote

edited 12 mins ago

Dannnno

5,4321749

answered 2 hours ago

pm100

41625

add a commentÂ |Â

up vote
0
down vote

edited 12 mins ago

Dannnno

5,4321749

answered 2 hours ago

pm100

41625

edited 12 mins ago

Dannnno

5,4321749

answered 2 hours ago

pm100

41625

edited 12 mins ago

Dannnno

5,4321749

edited 12 mins ago

Dannnno

5,4321749

edited 12 mins ago

Dannnno

5,4321749

answered 2 hours ago

pm100

41625

answered 2 hours ago

pm100

41625

answered 2 hours ago

pm100

41625

add a commentÂ |Â

Ivan Stimac is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

Ivan Stimac is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

Post as a guest

Name

Search This Blog

Iyfjky