Review implementation of stack by using array in C [closed]

Question

Closed. This question needs to be more focused. It is not currently accepting answers.

---

Want to improve this question? Guide the asker to update the question so it focuses on a single, specific problem. Narrowing the question will help others answer the question concisely. You may edit the question if you feel you can improve it yourself. If edited, the question will be reviewed and might be reopened.

Closed 10 years ago.

Improve this question

This question has become long after many updates. Click here to go down.

---

I have implemented stack using arrays in C. Please give me suggestions on how to improve it. The purpose of writing it is practice-only.

I'll be implementing it by using pointers soon so please leave the part about using pointers instead of arrays for implementing stack.

I'll keep adding updated code so please review the latest one.

#include<stdio.h>
#include<stdlib.h>
#define MAX_SIZE 100
#define MIN 1
#define MAX 3
void intro();
void push(int *arr, int *length, int data);
int pop(int* arr, int *length);
int main()
{
 int arr[MAX_SIZE];
 int length = 0;
 int choice = MAX + 1;
 int data;
 while (1)
 {
 while (MAX < choice || choice < MIN)
 {
 system("cls");
 intro();
 printf("Enter your choice -> ");
 scanf("%d", &choice);
 }
 system("cls");
 switch (choice)
 {
 case 1:
 printf("\nEnter data to be pushed -> ");
 scanf("%d", &data);
 push(arr, &length, data);
 printf("\nData pushed");
 break;
 case 2:
 printf("\nPopped Data is %d", pop(arr, &length));
 break;
 case 3:
 return 0;
 }
 printf("\nLength is %d", length);
 getchar();
 getchar();
 choice = MAX + 1;
 }
}
void intro()
{
 printf("1 Push data\n");
 printf("2 Pop Data\n");
 printf("3 Exit this program\n\n");
}
void push(int *arr, int *length, int data)
{
 if (*length == MAX_SIZE){
 printf("Stack Overflow\n");
 exit(1);
 }
 arr[(*length)++] = data;
}
int pop(int *arr, int *length)
{
 if (*length == 0){
 printf("Stack Underflow\n");
 exit(2);
 }
 return arr[--(*length)];
}

Update 1 after Lstor gave suggestions

#include<stdio.h>
#include<stdlib.h>
#include<errno.h>
static const int MAX_SIZE = 100;
static const int MIN = 1;
static const int MAX = 4;
void intro();
void push(int *, int *, int);
int pop(int *, int *);
int top(int *, int *);
int main()
{
 int arr[MAX_SIZE];
 int length = 0;
 for (;;)
 {
 int choice = MAX + 1;
 while (MAX < choice || choice < MIN)
 {
 system("cls");
 intro();
 printf("Enter your choice -> ");
 scanf("%d", &choice);
 }
 system("cls");
 int data;
 errno = 0;
 switch (choice)
 {
 case 1:
 printf("\nEnter data to be pushed -> ");
 scanf("%d", &data);
 push(arr, &length, data);
 if (errno == 1){
 printf("\nStack overflow");
 }
 break;
 case 2:
 data = pop(arr, &length);
 if (errno == 2){
 printf("\nStack underflow");
 }
 else{
 printf("\nThe data is %d", data);
 }
 break;
 case 3:
 data = top(arr, &length);
 if (errno == 1){
 printf("\nStack overflow");
 }
 else if (errno == 2){
 printf("\nStack underflow");
 }
 else{
 printf("\nThe data at top is %d", data);
 }
 break;
 case 4:
 return 0;
 }
 printf("\nLength is %d", length);
 getchar();
 getchar();
 }
}
void intro()
{
 printf("1 Push data\n");
 printf("2 Pop Data\n");
 printf("3 See the top of the stack\n");
 printf("4 Exit this program\n\n");
}
void push(int *arr, int *length, int data)
{
 if (*length == MAX_SIZE){
 errno = 1;
 return;
 }
 arr[(*length)++] = data;
}
int pop(int *arr, int *length)
{
 if (*length == 0){
 errno = 2;
 return -1;
 }
 return arr[--(*length)];
}
int top(int *arr, int *length)
{
 if (*length == 0){
 errno = 2;
 return -1;
 }
 else if (*length == MAX_SIZE){
 errno = 1;
 return -1;
 }
 return arr[*length - 1];
}

Update 2 - Includes the suggestions by Lstor given in comments and William Morris's suggestions. I changed a few things and didn't implement William Morris's suggestion about user experience.

#include<assert.h>
#include<errno.h>
#include<stdio.h>
#include<stdlib.h>
static const int MAX_SIZE = 100;
enum action {PUSH = 1, POP, TOP, QUIT};
void clear_screen(void)
{
 system("cls");
}
static enum action get_user_action(void)
{
 int choice = 0;
 do
 {
 clear_screen();
 printf("%d Push data\n"
 "%d Pop Data\n"
 "%d See the top of the stack\n"
 "%d Exit\n\n"
 "Enter your choice -> ", PUSH, POP, TOP, QUIT);
 scanf("%d", &choice);
 } while (choice != PUSH && choice != POP && choice != TOP && choice != QUIT);
 return (enum action) choice;
}
void push(int *arr, int *length, int data)
{
 if (*length == MAX_SIZE){
 errno = PUSH;
 return;
 }
 arr[(*length)++] = data;
}
int pop(int *arr, int *length)
{
 if (*length == 0){
 errno = POP;
 return -1;
 }
 return arr[--(*length)];
}
int top(int *arr, int *length)
{
 if (*length == 0){
 errno = POP;
 return -1;
 }
 else if (*length == MAX_SIZE){
 errno = PUSH;
 return -1;
 }
 return arr[*length - 1];
}
int main(void)
{
 int arr[MAX_SIZE];
 int length = 0;
 enum action choice;
 while ((choice = get_user_action()) != QUIT)
 {
 clear_screen();
 int data;
 errno = 0;
 switch (choice)
 {
 case PUSH:
 printf("Enter data to be pushed -> ");
 scanf("%d", &data);
 push(arr, &length, data);
 if (errno == PUSH){
 printf("Stack overflow\n");
 }
 break;
 case POP:
 data = pop(arr, &length);
 if (errno == POP){
 printf("Stack underflow\n");
 }
 else{
 printf("The data is %d\n", data);
 }
 break;
 case TOP:
 data = top(arr, &length);
 switch (errno)
 {
 case PUSH:
 printf("Stack overflow\n");
 break;
 case POP:
 printf("Stack underflow\n");
 break;
 default:
 printf("The data at top is %d\n", data);
 }
 break;
 default:
 assert(!"You should not have reached this.");
 }
 printf("Length is %d\n", length);
 getchar();
 getchar();
 }
}

Update 3 Includes William Morris's suggestions to Update 2

#include<assert.h>
#include<stdio.h>
#include<stdlib.h>
static const int MAX_SIZE = 100;
enum action {PUSH = 1, POP, TOP, QUIT};
void clear_screen(void)
{
 system("cls");
}
static enum action get_user_action(void)
{
 int choice = PUSH - 1;
 do
 {
 clear_screen();
 printf("%d Push data\n"
 "%d Pop Data\n"
 "%d See the top of the stack\n"
 "%d Exit\n\n"
 "Enter your choice -> ", PUSH, POP, TOP, QUIT);
 scanf("%d", &choice);
 } while (choice != PUSH && choice != POP && choice != TOP && choice != QUIT);
 return (enum action) choice;
}
void push(int *arr, int *length, int *status, int data)
{
 *status = PUSH - 1;
 if (*length == MAX_SIZE){
 *status = PUSH;
 return;
 }
 arr[(*length)++] = data;
}
int pop(int *arr, int *length, int *status)
{
 *status = PUSH - 1;
 if (*length == 0){
 *status = POP;
 return -1;
 }
 return arr[--(*length)];
}
int see_top(int *arr, int *length, int *status)
{
 *status = PUSH - 1;
 if (*length == 0){
 *status = POP;
 return -1;
 }
 return arr[*length - 1];
}
int main(void)
{
 int arr[MAX_SIZE];
 int length = 0;
 enum action choice;
 while ((choice = get_user_action()) != QUIT)
 {
 clear_screen();
 int status;
 int data;
 switch (choice)
 {
 case PUSH:
 printf("Enter data to be pushed -> ");
 scanf("%d", &data);
 push(arr, &length, &status, data);
 if (status == PUSH){
 printf("Stack overflow\n");
 }
 else{
 printf("%d pushed onto the stack\n", data);
 }
 break;
 case POP:
 data = pop(arr, &length, &status);
 if (status == POP){
 printf("Stack underflow\n");
 }
 else{
 printf("The data is %d\n", data);
 }
 break;
 case TOP:
 data = see_top(arr, &length, &status);
 switch (status)
 {
 case POP:
 printf("Nothing in the stack\n");
 break;
 default:
 printf("The data at top is %d\n", data);
 }
 break;
 default:
 assert(!"You should not have reached this.");
 }
 printf("Length is %d\n", length);
 getchar();
 getchar();
 }
}

Update 4 After William Morris's comments on Update 3. I also made the whole thing use consistent bracing style suggested by Lstor in my question about stack implementation by pointers

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
static const int MAX_SIZE = 100;
enum action {START, PUSH, POP, TOP, QUIT, END};
void clear_screen(void)
{
 system("cls");
}
static enum action get_user_action(void)
{
 int choice = START;
 do {
 clear_screen();
 printf("%d Push data\n"
 "%d Pop Data\n"
 "%d See the top of the stack\n"
 "%d Exit\n\n"
 "Enter your choice -> ", PUSH, POP, TOP, QUIT);
 scanf("%d", &choice);
 } while (!(START < choice && choice < END));
 return (enum action) choice;
}
void push(int *arr, int *length, int *status, int data)
{
 *status = START;
 if (*length == MAX_SIZE) {
 *status = PUSH;
 return;
 }
 arr[(*length)++] = data;
}
int pop(int *arr, int *length, int *status)
{
 *status = 0;
 if (*length == 0) {
 *status = 1;
 return -1;
 }
 return arr[--(*length)];
}
int peek(int *arr, int *length, int *status)
{
 *status = 0;
 if (*length == 0) {
 *status = 1;
 return -1;
 }
 return arr[*length - 1];
}
int main(void)
{
 int arr[MAX_SIZE];
 int length = 0;
 enum action choice;
 while ((choice = get_user_action()) != QUIT) {
 clear_screen();
 int status;
 int data;
 switch (choice) {
 case PUSH:
 printf("Enter data to be pushed -> ");
 scanf("%d", &data);
 push(arr, &length, &status, data);
 if (status == 1) {
 printf("Stack overflow\n");
 } else {
 printf("%d pushed onto the stack\n", data);
 }
 break;
 case POP:
 data = pop(arr, &length, &status);
 if (status == 1) {
 printf("Stack underflow\n");
 } else {
 printf("The data is %d\n", data);
 }
 break;
 case TOP:
 data = peek(arr, &length, &status);
 if (status == 1) {
 printf("Nothing in the stack\n");
 } else {
 printf("The data at top is %d\n", data);
 }
 break;
 default:
 assert(!"You should not have reached this.");
 }
 printf("Length is %d\n", length);
 getchar();
 getchar();
 }
}

Update 5 After discussing things in chat William Morris suggested other things. I have included Lstor's suggestion about using enum for status and suggestion in the chat.

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
static const int MAX_SIZE = 100;
enum action {START, PUSH, POP, TOP, QUIT, END};
enum status {SUCCESS, FAILURE};
void clear_screen(void)
{
 system("cls");
}
static enum action get_user_action(void)
{
 int choice = START;
 do {
 clear_screen();
 printf("%d Push data\n"
 "%d Pop Data\n"
 "%d See the top of the stack\n"
 "%d Exit\n\n"
 "Enter your choice -> ", PUSH, POP, TOP, QUIT);
 scanf("%d", &choice);
 } while (!(START < choice && choice < END));
 return (enum action) choice;
}
enum status push(int *arr, int *length, int data)
{
 if (*length == MAX_SIZE) {
 return FAILURE;
 }
 arr[(*length)++] = data;
 return SUCCESS;
}
enum status pop(int *arr, int *length, int *data)
{
 if (*length == 0) {
 return FAILURE;
 }
 *data = arr[--(*length)];
 return SUCCESS;
}
enum status peek(int *arr, int *length, int *data)
{
 if (*length == 0) {
 return FAILURE;
 }
 *data = arr[*length - 1];
 return SUCCESS;
}
int main(void)
{
 int arr[MAX_SIZE];
 int length = 0;
 enum action choice;
 while ((choice = get_user_action()) != QUIT) {
 clear_screen();
 int data;
 switch (choice) {
 case PUSH:
 printf("Enter data to be pushed -> ");
 scanf("%d", &data);
 if (push(arr, &length, data) == SUCCESS) {
 printf("%d pushed onto the stack\n", data);
 } else {
 printf("Stack overflow\n");
 }
 break;
 case POP:
 if (pop(arr, &length, &data) == SUCCESS) {
 printf("The data is %d\n", data);
 } else {
 printf("Stack underflow\n");
 }
 break;
 case TOP:
 if (peek(arr, &length, &data) == SUCCESS) {
 printf("The data at top is %d\n", data);
 } else {
 printf("Nothing in the stack\n");
 }
 break;
 default:
 assert(!"You should not have reached this.");
 }
 printf("Length is %d\n", length);
 getchar();
 getchar();
 }
}

Answer 1

In a simple practice program like this, the user experience is perhaps not something you considered much (more interesting is getting it to work :-) But you should think about it in general. Try using your program and consider whether it is comfortable to use. For example, if I want to enter many numbers into the array, I have to repeatedly select 1 then enter the number, select 1 then enter the number, etc. This is tedious. Added to that your getchar calls at the end of each loop mean the user must type enter again, without any purpose, the "Data pushed" message seems like noise, and if the user has the patience to enter 100 numbers and dares enter another, the program exits!

A better approach might be to allow several numbers to be entered at a time. Or perhaps change the interface so that if the user types a number it is pushed, if 'p' is typed, a number is popped and if 'q' the program exits. If the array is full, print a warning but don't exit. These are just examples, the point being that you should always think of the user.

---

One criticism I have of the coding is that your input choices are spread throughout the file. From MIN/MAX at the top through the choice-entry loop and the switch statement to the intro function at the bottom. There is nothing tying these together and so if a change is made in one place you have to remember to change every other location.

Adding an enum (or #defines) and using it everywhere would help:

enum action {PUSH, POP, QUIT};
static enum action get_user_action(void)
{
 int choice = 0;
 do {
 printf("%d Push data\n"
 "%d Pop Data\n"
 "%d Exit\n\n"
 "Enter your choice -> ", PUSH, POP, QUIT);
 scanf("%d", &choice);
 } while (choice != PUSH && choice != POP && choice != QUIT);
 return (enum action) choice;
}

and in main

enum action choice;
while ((choice = get_user_action()) != QUIT) {
 if (choice == PUSH) {
 // do the push
 } else {
 // do the pop
 }
}

---

Some minor points:

• Your printf statements often put a '\n' at the beginning of the text to be printed. This is unusual - it would be better if you put the \n at the end.

• Your switch statement has no default case, which is generally bad practice. Always use a default. In this case the switch is arguably not needed.

• Move main to the end to avoid the need for prototypes. Although at first it might seem odd, this is a common pattern.

• Both main and intro need void parameter list

---

Comments on your 2nd update

I'm not keen on your setting errno as an error value. And certainly not with the PUSH/POP values you have used. Values written to errno are defined in errno.h - or more probably sys/errno.h and you should never conflict with those numbers. Moreover, errno is used by system and library functions and I'm not happy extending that for your own private use.

Error handling is often the most difficult part of C (which is perhaps why exceptions seem at first glance to be such a boon in C++ - although they seem to cause more problems than they cure from my ignorant perspective). The general pattern for library and system functions is to return -1 or NULL to the caller and maybe set errno to show the reason. Often -1/NULL is returned in place of the normal return value, which is not ideal. In your case, the reason for failure is clear and errno is not needed. As you have used it, the caller must be sure to set errno to zero before calling one of your functions, as the functions do not indicate failure (even pop and top don't, as -1 is a valid number to find on the stack).

So since we don't have exceptions and I'm saying not to use errno as you have, what should you do?

Since you cannot mix the return value and the data (-1 is a valid number for the stack), there are two alternatives. Return status and add a return parameter for the value (pop/top).

int push(int *arr, int *length, int data) 
{
 if (*length == MAX_SIZE) {
 return -1;
 }
 arr[(*length)++] = data;
 return 0;
}
int pop(int *arr, int *length, int *data)
{
 if (*length == 0){
 return -1;
 }
 *data = arr[--(*length)];
 return 0;
}

Or return the data (pop/top) and add a return parameter for the status:

void push(int *arr, int *length, int data, int *status) 
{
 if (*length == MAX_SIZE) {
 *status -1;
 }
 arr[(*length)++] = data;
 *status = 0;
}
int pop(int *arr, int *length, int *status)
{
 if (*length == 0){
 *status -1;
 }
 *status = 0;
 return arr[--(*length)];
}

Take your pick and be consistent within the app. In this case I think I'd go for returning status.

In the case of your top function, you don't need to pass length as a pointer, arr could and should be const and it is surely not an error if length == MAX_SIZE.

---

Comments on 3rd update.

No, as I said before in relation to errno, don't use PUSH and POP as error status. And using PUSH-1 as a synonym for 0 is horribly wrong. When you have defined some constants (with enum or #define etc) you must treat their value as unknown - nothing is permitted to assume the value of an enum or define - which is what you have done in using PUSH-1. If you assume the value of such a constant you negate the purpose of using the constant instead of the raw value.

As I said before, you don't need different status values. Just use 0 for good and -1 for bad (or in other circumstances, a valid pointer and NULL, or 1 for true and 0 for false etc). If at some point you did need to differentiate between errors, you could define some error constants. PUSH is not an error constant it is one of your actions. STACK_FULL or STACK_EMPTY would be meaningful. But you don't need that here. Your statements in main should just check for if (status < 0) {...} or if (status != 0) {...} or even if (status) {...} .

Comment on 5th update

That looks much better, don't you think? I hope you agree because a lot of the process of improving code is self criticism. Once you get a feeling for what looks good and what is just not quite right (the 'yuk' feeling), you have made the biggest leap to improving your code.

When I look at your latest update, I now have only minor 'yuk' moments. One is the START/END tags of the enum, which stick out somehow. I have trouble explaining why - I think it is just at the level of "I wouldn't do that", which is a danger when reviewing - separating objective comments from personal preference.

Another minor point is in your use of START/END.

...
} while (!(START < choice && choice < END));

This would be more naturally expressed as

...
} while (choice <= START || choice >= END);

The peek function still needs a const on arr and length should not be a pointer.

A final minor point is the return of enum status by the functions. This is perfectly ok. Nothing wrong at all. It is just unusual to see functions returning such an enum rather than just an int (0/-1). Arguably what you have is "better" in that it is clear from the enum what is good/bad. It is just not normally done :-)

• Aseem Bansal: Many thanks for the suggestion of enum. I always knew that this structure, which I used regularly, was bad but didn't knew any remedy or what to ask for. I'll update my code and post the updated one.
• Aseem Bansal: I added the updated code. Update 2. I haven't taken care of the user experience but I think other things should be good. One thing that I am not sure is the while in get_user_input. If the things in enum become large would checking individually be a good idea? Same goes for using if...else structure in the main function. Wouldn't switch work? After all enum have int associated with them. Please comment.
• Aseem Bansal: I hope you didn't have the time to read the last comment because I tried and the switch version worked. I tweaked some more checks and I think the readability increased. Any suggestions?
• Aseem Bansal: About length == MAX_SIZE not being an error, in case we are pushing and stack is filled then isn't that an error? I was mistaken in case of function top but in case of push it is an error. Also I am doing Update 3. Please comment.
• Lstor: Although I missed the PUSH - 1 part when I read your code, @WilliamMorris is making a very important point here. enums provide an abstraction, and you should not break that abstraction by assuming values. However, as long as the values are contiguous I think it's fine to test if a value is inside the range or not.

Answer 2

Most of the code is a driver program. The only really interesting part here is

arr[(*length)++] = data;

and

return arr[--(*length)];

as well as the range-checking. I won't look too closely at the driver code.

My key advice is: Make your data structure reusable. Put it in a header and implementation file, and use that. The next step is to dynamically allocate the array in a create type of function and use that instead.

Some general style notes:

• Use (static) const int instead of #define for constants.
• In C99 and C11, you don't have to declare variables at the beginning of a block. Use C99 or C11 and declare variables as late as possible. (You are already using one of them, otherwise your main would require a return 0; at the end.)
• Don't pass length as a pointer. There's no need to, and makes the code more error-prone.
• I like for (;;) better than while (1). Think "forever". It's just a matter of taste, though.
• Avoid platform-dependent code. If you must, at least wrap it up in a function.
• Your push and pop should neither do IO nor exit the program. The C way is to return a status code (or set errno).
• Your interface is incomplete: It does not provide a way to peek at the top of the stack without popping it.
• In terms of variable names, indentation, whitespace and so on your program is pretty okay. That is good! (But I would have written the extra ay.)

These points are mostly nitpicking. Don't bother to improve your program. Write a new one, and take it all the way: Call malloc.

Update: Key points from comments.

• Use a default branch in your switch. If you don't expect the default to ever be entered, then put an assertion in it:

  switch (condition) {
  case firstCase:
   // ...
  default:
   assert(!"Should never be reached.");
  }
  

  String literals are always truthy. By reversing the value (with !), the assertion will always trigger if the line is executed.

• see_top is often called peek.

• The condition in the do...while in get_user_action can be choice < PUSH && choice > QUIT. If you add dummy elements at the beginning and end of the enum and test against them instead, you won't need to update the code when you add more options:

  enum action { BEGIN, PUSH, /* ... */ END };
  do {
   // ...
  } while (choice <= BEGIN && choice >= END);
  

• I'd put an empty line below each case block, and a space before < in the #includes.

• Sort your #includes alphabetically.

• Aseem Bansal: I am going to make it reusable by using pointers. Just wanted to get some basic things out of the way first. If I shouldn't pass the length as pointer then how do you propose to update that in the functions? About platform-dependent code, which one? system('cls')? The extra ay? I am ignoring your advice about malloc for now and updating my question with updated code. Please comment.
• Aseem Bansal: I am ignoring the malloc part only because I'll be implementing that after I understand completely how to implement stack by arrays. I have been told that my basic concepts are bad so just trying to think everything again.
• Lstor: I don't have anything further to add to the updated code, except to put #include <errno.h> at the top (to keep them sorted alphabetically). Also, I recommend having a default branch in your switch. In C89, there is no implicit return from main(), and therefore you need it right before the end of the scope, unless that is unreachable. But you should prefer C99 or C11 anyway.
• Lstor: Exactly. Put assert(!"Should never be reached."); (or something like that) there, so you will know if you have made a mistake.
• Lstor: Nothing really substantial. see_top is often called peek. The condition in the do...while in get_user_action can be choice < PUSH && choice > QUIT). If you add dummy elements at the beginning and end of the enum and test against them instead, you won't need to update the code when you add more options. Finally, I'd put an empty line below each case block, and a space before < in the #includes.