No, your implementation is broken: You forgot to check whether allocation succeeded. --- Reviewing your code, you are going way overboard following the "small methods"-mantra in your library-code: While the [Single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle) means every function should do one thing, and only one thing, the size of each task should be properly chosen. Too little, and the function isn't a useful abstraction, using, writing and remembering it takes more effort than it saves. (The performance-cost may be negligible (or even non-existent), if it is inlined. Consider marking it `static` to promote inlining and avoid exporting.) Too much, and reusability and composability suffer. --- Header: 1. I would avoid a typedef for the element-type. What does it buy you? 1. Using an opaque pointer is a good idea allowing you to replace the implementation with something more performant later. So, well done there. 1. Consider leaving the parameter-name out of the declaration when it doesn't make it more descriptive. 1. You should add a variation on `stack_push` which signals success, so a caller can opt in to handling failure gracefully: bool stack_tryPush(Stack, StackElement); 1. Some more useful additions: StackElement stack_peek(Stack); StackElement stack_replace(Stack, StackElement); Sourcefile: 6. You should re-order your includes: Always include the header you are implementing first, then all other headers, sorted for consistency (use the IDE's sorting-macro). That way, you will know that your header is self-sufficient and you didn't forget a crucial dependency. 2. There's `abort()` for abnormal termination. Consider writing a helper-function if you need to output an error-message and abort in multiple places: static void die(const char* message); 2. All your internal functions should not have external linkage, for performance and correctness reasons: Mark them `inline`. 2. Putting `next` first is more common, and possibly slightly more efficient. Also, for a single value `value` is more idiomatic than `data`. 2. Is there any reason why your typedef-name and struct-tag aren't *quite* equal? That's ittitating. 2. `node_create` is a waste: It's literally more work to read, write, and use it than leaving the single use inline. As a rule of thumb, extracting 2 lines into their own function is a net loss of readability. Exceptons are often-used pieces of code. Actually, that can be generalized to all your internal functions, with the possible exception of `node_delete`, which is marginal. 2. [There are](http://programmers.stackexchange.com/questions/15515/using-assertions-versus-throwing-exceptions) two [kinds of](http://programmers.stackexchange.com/questions/116576/how-can-i-improve-my-error-checking-and-handling) precondition-checks: 1. Sanity checks, verifying that the programs logic is sane. These are normally disabled in a release build for performance reasons, and use `assert()`. 2. Tests of untrusted input. How far you can/should/must trust an API's user must be decided in each case. It's your task as API-designer to decide how much you should penalize your users to catch egregious abuse even in release-mode, and whether there's also a debug-mode. But there's no excuse for making your internal helper-functions re-assert preconditions even in release-mode. 2. As you are using C99 (see `<stdbool.h>`), you can use compound-literals to set all members of a struct-type comfortably even after initialization. void stack_push(StackCDT *s, StackElement value) { Node *new = malloc(sizeof *new); *new = (Node){s->top, value}; if(!new) die("Failed to allocate a new node."); s->top = new; } Testprogram: 14. `return 0;` is implicit in `main` since C99.