typedef struct Stack { char v; int x; int y; struct Stack* next; } Stack;Above is my stack struct.
Stack* helper (char val, int x, int y) { Stack* node = (Stack*)malloc(sizeof(Stack)); node->v = val; node->x = x; node->y = y; return node; } void push (Stack** top,char val,int x,int y) { Stack* node = helper(val,x,y); node->next = *top; *top = node; printf("%c pushed to stack\n",val); } void pop (Stack** top,char *val,int *w,int *h) { if (checkempty(*top)) return; Stack* temp = *top; *top = (*top)->next; *val = temp->v; *w = temp->x; *h = temp->y; free(temp); } int checkempty(Stack* top) { return !top; }Above are my push and pop functions.I was wondering if there are better ways to do this ? Or is what I am doing the standard way ?
- 2\$\begingroup\$It would be better if your whole program was provided to allow us to test the code as you tested the code. It also isn't clear what the variables
xandymean in the structure.\$\endgroup\$2020-03-08 19:49:36 +00:00CommentedMar 8, 2020 at 19:49
3 Answers3
Stack* helper (char val, int x, int y) {Your indentation and whitespace are kind of funky; I recommend looking at what some popular open-source code on GitHub does, and trying to copy them as closely as possible. Or, just run your code through a formatter such asclang-format.
Stack *helper(char val, int x, int y){This function is only ever used in one place, so why do you have it? Right now it's kind of unsafe, becausehelper returns aStack * which:
must be freed by the caller to avoid memory leaks, and
has uninitialized garbage in its
nextfield.
If you inlined it intopush, you'd have a complete unit of work, without either of those dangerous sharp edges.
Look into proper library organization. You should have a.h file that definesstruct Stack and declarespush andpop, and a.c file with the implementations.
You should probably name the functions something likestack_push andstack_pop, sincepush andpop are likely to be common names; e.g. you might later want to write astruct Queue with aqueue_push andqueue_pop. This kind of "manual namespacing" is common in C libraries.
Of coursepush shouldn't callprintf (there's no "I" in "TEAM" and there's no "printf" in "push data onto a stack").
Inpop's argument list, you usew andh where you meantx andy.
pop has interesting behavior on an empty stack: it just returns without initializing*x and*y. This is a problem for the caller, because the caller doesn't have any indication that anything went wrong! You should probably makepop return abool to indicate whether it was successful.
Compare a pointer for null with(top == NULL), not!top. Clarity counts!
Here's an idea if you want to push yourself harder: Right now you're passingx,y, andvalue as individual arguments topush, and retrieving them individually frompop. Package them up into a struct!
Here's the struct definitions and function declarations for a different stack implementation. Can you see how to implement these functions?
struct Data { int x, y, value; };struct DataStack { struct Data data; struct DataStack *next; }struct DataStack *datastack_push(struct DataStack *, struct Data);struct DataStack *datastack_pop(struct DataStack *);struct Data datastack_top(const struct DataStack *);bool datastack_empty(const struct DataStack *);Consider separating the stack from the stack entries. Right now, you have to pass a pointer to a pointer to the stack to your push & pop routines.Quuxplusone’s solution requires the caller to do the work of assigning the return value to the stack pointer. Both of these are harder to use.
Moreover, with Quuxplusone’s solution, you can’t pass an empty stack to another function, and have it fill in entries for the caller.
Separating the stack from the stack entries allows the stack to store additional meta data about the stack. Eg)
typedef struct StackEntry { char v; int x, y; struct StackEntry *next;} StackEntry;typedef struct Stack { StackEntry *top; int num_entries;} Stack;Now,push,pop &check_empty would all just take aStack*, which is easier to remember. Stacks can be passed & shared, even if they are empty, since they wouldn’t just be a null pointer. And you can determine the depth of a stack in\$O(1)\$ time.
Stack* helper (char val, int x, int y) { Stack* node = (Stack*)malloc(sizeof(Stack)); node->v = val; node->x = x; node->y = y; return node; }
That's not a very descriptive name - something in keeping with the existing scheme might bestack_node_create().
Sincemalloc() returns avoid*, there's no need to scare readers by inserting a cast operator.
We must check thatmalloc() didn't give us a null pointer before we dereference it.
Fixing those issues gives us:
#include <stdlib.h>/** * Return a newly-created node, or a null pointer on failure. */Stack *stack_node_create(char val, int x, int y){ Stack *const node = malloc(sizeof *node); if (!node) { return node; } node->v = val; node->x = x; node->y = y; return node;}When we call this, we do need to remember that it can return a null pointer, and handle that accordingly.
You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.
