When the entire stack is displayed, only the last elements of the stack are displayed. Why?

51 Views Asked by At

I need to add new items to the end of the list, delete the last one and display the entire list. When displaying the entire list, for some reason, only the last elements of the stack are displayed, by the number of elements in the list. Why?

#include <iostream>
#include <stdio.h>
#include <stack>

using namespace std;

struct Node
{
    char* name = new char[6];
    float sumary;
    int amount;
    Node(char* name, float sumary, int amount) :name(name), sumary(sumary), amount(amount)
    {
    }
    Node() {}
};

int main()
{
    int command;
    stack<Node> node;
    for (;;)
    {
        printf("Input command:\n 1 - add,\n 2 - delete last,\n 3 - show all,\n 4 - exit\n");
        scanf("%d", &command);
        switch (command)
        {
        case 1:
            char name[6];
            float sumary;
            int amount;
            printf("Enter name: ");
            scanf("%s", &name);
            printf("Enter sumary: ");
            scanf("%f", &sumary);
            printf("Enter amount: ");
            scanf("%d", &amount);
            node.push(Node(name, sumary, amount));
            break;
        case 2:
            node.pop();
            printf("The last have been deleted");
            break;
        case 3:
            while (!node.empty())
            {
                Node temp = node.top();
                node.pop();
                cout << temp.name << " " << temp.sumary << " " << temp.amount << endl;
            }
            break;
        case 4:
            return 0;
        default:
            printf("Wrong command...");
            break;
        }
    }
}
2

There are 2 best solutions below

0
bruno On

In the constructor :name(name) does not do what you expect while name is a char*, you do not deeply copy the name but just save the pointer which become invalid out of the switch with an undefined behavior (and lost the allocated array of char producing a memory leak).

Use a std::string rather than a char* and you will have the expected behavior. That also simplify a lot the management when a Node is copied, assigned, deleted because you have nothing to do, which is not the case using a pointer.

Out of that having char name[6]; if a word having more than 5 characters is available for scanf("%s", &name); it writes out of your array with an undefined behavior.

You do not also check the calls of scanf success checking they return 1, you are not protected if an input is invalid.

Note rather than to use C functions to read/write you can use iostream features (also checking the readings success anyway).

The choice of a std::stack rather than for instance a std::vector is strange because to write its content is less practical.

A way to do can be where the stack is not emptied when writing its contains :

#include <iostream>
#include <string.h>
#include <stack>
#include <limits>

struct Node
{
  std::string name;
  float sumary;
  int amount;

  Node(std::string name, float sumary, int amount) 
    :name(name), sumary(sumary), amount(amount) {
 }
 Node() {}
};

void inputError(const char * msg)
{
  if (std::cin.rdstate() & std::istream::eofbit)
  {
    // EOF
    std::cerr << "EOF, abort" << std::endl;
    exit(-1);
  }

  std::cerr << msg << std::endl;

  std::cin.clear();
  // flush all the line, you can just read a word
  // with "string s; cin >> s;" if you prefer
  std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}

int main()
{
    int command;
    std::stack<Node> node;

    for (;;)
    {
        std::cout << "Input command:\n 1 - add,\n 2 - delete last,\n 3 - show all,\n 4 - exit" << std::endl;

        if (!(std::cin >> command))
          inputError("invalid command, not an integer");
        else
        {
          switch (command)
          {
          case 1:
            {
              std::string name;
              float sumary;
              int amount;

              if (!(std::cout << "Enter name: ", std::cin >> name) ||
                  !(std::cout << "Enter sumary: ", std::cin >> sumary) ||
                  !(std::cout << "Enter amount: ", std::cin >> amount))
                inputError("invalid value");
              else
                node.push(Node(name, sumary, amount));
            }
            break;
          case 2:
            if (node.empty())
              std::cerr << "no node to delete" << std::endl;
            else 
            {
              node.pop();
              std::cout << "The last have been deleted" << std::endl;
            }
            break;
          case 3:
            {
              std::stack<Node> temp = node;

              while (!temp.empty())
              {
                const Node & n = temp.top();

                temp.pop();
                std::cout << n.name << " " << n.sumary << " " << n.amount << std::endl;
              }
            }
            break;
          case 4:
            return 0;
          default:
            std::cerr << "Wrong command number" << std::endl;
            break;
          }
        }
    }
}

Compilation and execution :

pi@raspberrypi:/tmp $ g++ -Wall c.cc
pi@raspberrypi:/tmp $ ./a.out
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
7
Wrong command number
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
z
invalid command, not an integer
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
1
Enter name: aze
Enter sumary: a
invalid value
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
1
Enter name: aze
Enter sumary: 1.1
Enter amount: 2
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
1
Enter name: qsd
Enter sumary: 2.2
Enter amount: 3
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
3
qsd 2.2 3
aze 1.1 2
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
3
qsd 2.2 3
aze 1.1 2
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
2
The last have been deleted
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
3
aze 1.1 2
Input command:
 1 - add,
 2 - delete last,
 3 - show all,
 4 - exit
4
pi@raspberrypi:/tmp $ 

It can be also a good idea to define the operator<< in Node rather than to put its members public to be able to access them in main to write them

0
Vlad from Moscow On

This constructor

Node(char* name, float sumary, int amount) :name(name), sumary(sumary), amount(amount)
{
}

does not make sense because this initialization of the data member name within the class definition

char* name = new char[6];

is ignored. From the C++ 17 Standard (12.6.2 Initializing bases and members)

10 If a given non-static data member has both a brace-or-equal-initializer and a mem-initializer, the initialization specified by the mem-initializer is performed, and the non-static data member’s brace-or-equal-initializer is ignored.

So the data member name is always initialized by the address of the firs element of the local array name with automatic storage duration

    switch (command)
    {
    case 1:
        char name[6];
        //...
        node.push(Node(name, sumary, amount));
        //...

So the program has undefined behavior. As a result as you pointed out yourself you get that all nodes output the same string.

Pay attention to that this call of scanf

scanf("%s", &name);

is incorrect. You have to write

scanf("%s", name);

but in any case it is much better to use C++ stream functions. And instead of the dynamically allocated array you should use the standard class std::string.

And moreover your class does not have an explicit destructor that shall free the allocated memory if you are using dynamic memory allocation.

So substitute the pointer declaration for declaration of a data member of the type std::string.