0

I wrote a stack before. Basically, I had two classes, Node and Stack. This stack can just push, pop and check the head of the stack. The code is shown below:

#include <iostream>

using namespace std;

template<class Datatype>
class Node
{
    public:
        Node()
        {
            next = NULL;
            prev = NULL;
        }
        Node* getNext()
        {
            return next;
        }
        Node* getPrev()
        {
            return prev;
        }
        Datatype* getData()
        {
            return &data;
        }
        void changeNext()
        {
            next = NULL;
        }
        void changeNext(Node& nextNode)
        {
            next = &nextNode;
        }
        void changePrev()
        {
            prev = NULL;
        }
        void changePrev(Node& prevNode)
        {
            prev = &prevNode;
        }
        Node* addNext(Node &);
        Node* addPrev(Node &);
        void nodeDel();
        void addData(Datatype &);
    private:
        Node* next;
        Node* prev;
        Datatype data;
};

template<class Datatype>
class Stack
{
    public:
        Stack() : head( NULL )
        {

        }
        int push(Datatype &);
        int pop(Datatype &);
        Datatype* peek();
    private:
        Node<Datatype> *head;
};

template <class Datatype>
Node<Datatype>* Node<Datatype>::addNext(Node<Datatype>& exi_node)
{
    if (exi_node.getNext() == NULL)
    {
        changePrev(exi_node);
        exi_node.changeNext(*this);
    }
    else
    {
        Node* next = exi_node.getNext();
        changePrev(exi_node);
        changeNext(*next);
        exi_node.changeNext(*this);
        next -> changePrev(*this);
    }
    return &exi_node;
}

template <class Datatype>
Node<Datatype>* Node<Datatype>::addPrev(Node<Datatype>& exi_node)
{
    if (exi_node.getPrev() == NULL)
    {
        changeNext(exi_node);
        exi_node.changePrev(*this);
    }
    else
    {
        Node* prev = exi_node.getPrev();
        changePrev(*prev);
        changeNext(exi_node);
        exi_node.changePrev(*this);
        prev -> changeNext(*this);
    }
    return &exi_node;
}

template<class Datatype>
void Node<Datatype>::nodeDel()
{
    if (prev == NULL && next == NULL)
        ;
    else if (prev == NULL)
    {
        Node* next = getNext();
        next -> changePrev();
    }
    else if (next == NULL)
    {
        Node* prev = getPrev();
        prev -> changeNext();
    }
    else
    {
        Node* next = getNext();
        Node* prev = getPrev();
        next -> changePrev(*prev);
        prev -> changeNext(*next);  
    }
    delete this;
    return;
}

template <class Datatype>
void Node<Datatype>::addData(Datatype &new_data)
{
    data = new_data;
}

template <class Datatype>
int Stack<Datatype>::push(Datatype &new_data)
{
    Node<Datatype> *pt_node = new Node<Datatype>;
    if (pt_node == NULL)
        return -1;

    pt_node -> addData(new_data);

    if (head == NULL)
        head = pt_node;
    else
    {
        pt_node -> addPrev(*head);
        head = pt_node;
    }

    cout << *(head -> getData()) << endl;

    return 0;
}

template <class Datatype>
int Stack<Datatype>::pop(Datatype &pop_data)
{
    if (head == NULL)
        return 0;

    pop_data = *(head -> getData());

    if (head -> getNext() == NULL)
    {
        delete head;
        head = NULL;
    }
    else
    {
        Node<Datatype>* temp = head;
        head = head -> getNext();
        delete temp;
    }

    return 1;
}

template <class Datatype>
Datatype* Stack<Datatype>::peek()
{
    return (this->head)->getData();
}

It's a pretty simple stack. The Stack class has some interface for users and also a pointer points to the first Node. Node class has two pointers pointing to both direction and also a data field. Now, I want to add a function to my Stack, which is to find the min value in the stack within O(1). One method is to add a field "local_min" in Node class, which records the minimum value beneath itself. In this way, pop(), push(), find_min() can all be done in O(1). My intuition is to have inheritance of both Node and Stack, because I only need to add a new function. However, I feel this very inconvenient. I have to rewrite almost all the functions. The pointer in both Node and Stack pointed to class Node. If I use a new class, like New_Node, all the thing has to be changed. Are there any good ways for me to do this extension? I really feel like inheritance should be working well here. However, I cannot figure it out.

Tanmay Patil
  • 6,882
  • 2
  • 25
  • 45
Shanpei Zhou
  • 371
  • 1
  • 2
  • 17
  • 3
    And you are not using `std::stack` because...? – Shoe Mar 03 '14 at 19:50
  • I am learning C++ and just try to implement a stack myself and do some implementation. From what I learn, I only know put "using namespace std" at the very beginning. I am not sure this std::stack. The code I provide works well. I met problem when I want to do extension. – Shanpei Zhou Mar 03 '14 at 19:53
  • notice `Stack` != `stack`; c++ is case sensitive – Sebastian Hoffmann Mar 03 '14 at 19:55
  • @user3352668 You are learning it wrong. You should _not_ put `using namespace std;` at the very beginning. See http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice. –  Mar 03 '14 at 20:00
  • @user3352668, may I ask where are you learning C++ exacly? What book/resource? – Shoe Mar 03 '14 at 20:00
  • did you heard about aspect oriented programming? I think this is the way here. Create a new Stack class that inherit your class and add the new feature in each function, then call the father function to do the stack work. – hidrargyro Mar 03 '14 at 20:03
  • *The code I provide works well.* A Stack class with no user-defined destructor to handle the dynamically allocated memory? As soon as you add the destructor, try this: `int main() { Stack s1; int x = 10; s1.push(x); Stack s2 = s1; }` Watch what happens when your program exits... – PaulMcKenzie Mar 03 '14 at 20:47

2 Answers2

2

Your Stack is not written in a way that facilitates clean OO re-use: you haven't got a virtual destructor, and the member functions that derived types may wish to override aren't virtual either (specifically push and pop). So, it is cleaner to write a new class that uses Composition - storing a Stack object as a data member. Then there's no need to change Node - you can just add a Stack<Node*> to your StackWithMin class, and push/pop an extra Node* only when the data Stack pushes/pops a Node that changes the min. For example, after pushing 3, 7, 5, 2, 3, your StackWithMin data members would be:

Stack<Data>   Stack<Node*>
[0] 3         &[0]
[1] 7         &[3]
[2] 5
[3] 2
[4] 3

This encodes that the current min is at &[3], and if a pop removes that element then it should be popped from the Stack<Node*> too.

Separately, Node is a supporting type for Stack, and the definition need not be offered as part of the client-facing interface. To hide it, consider putting it in a namespace called something like Stack_Private, Stack_Support or Stack_Implementation so that clients know not to use, then you can reasonably make it a struct and get/set the data members directly instead of using accessor functions - that will make your code less verbose.

Tony Delroy
  • 102,968
  • 15
  • 177
  • 252
  • 1
    "Composition" should be bold, font size 100. I have already seen too much code trying to be as "object oriented" as possible in my life. Especially when using templates, inheritance is often unnecessary. – Excelcius Mar 03 '14 at 20:55
  • Why do I need to virtual those functions that may wish to override? – Shanpei Zhou Mar 03 '14 at 21:48
  • 1
    Well, say some client code already has a `Stack my_stack;` and calls a function `void add_data(Stack* p) { p->push(10); p->push(20); }`, then you tell everyone you've got a great new version that also tracks the minimum value: the client programmer goes and changes `Stack_With_Min my_stack;` and compiles... no errors - their call to `f(my_stack)` is still valid because by inheriting from `Stack` you're saying that operations via a base-class pointer are legitimate, but sadly the `push` and `pop` done within `f()` aren't virtual so call the `Stack` implementation. – Tony Delroy Mar 03 '14 at 22:01
  • That implementation doesn't make any effort to track the minimum. When `f()` returns to the client code and later a call is made to get the minimum, it will likely be wrong, but depending on what you expected it to have done and how carefully you check it could crash (for example, if you depend on there always being a valid pointer to the minimum somewhere that `f()` hadn't populated). – Tony Delroy Mar 03 '14 at 22:03
  • Thank you so much! I am just a beginner. You help me a lot!Thanks. – Shanpei Zhou Mar 03 '14 at 22:04
  • @user3352668: you're welcome. You're asking good questions - I'm sure you'll progress quickly. You might like to read about the Liskov Substitution Principle - it is a great guideline for what it should mean to inherit from and customise a base class, though most explanations of it take a bit of effort to understand, it's well worth it. – Tony Delroy Mar 03 '14 at 22:06
0

Sometimes in these situations you should use non-template base class. This base class can implement all functions which aren't depend of a given data type.

class Node
{
public:
    Node() : _next(nullptr), _prev(nullptr) {
    }
public:
    Node* getNext() {
        return _next;
    }
    Node* getPrev() {
        return _prev;
    }
    void setNext(Node *node) {
        _next = node;
    }
    void setPrev(Node *node) {
        _prev = node;
    }
private:
    Node* _next;
    Node* _prev;
};

template <typename Value>
class NodeWithType : public Node
{
public:
    NodeWithType() : Node() {
    }
public:
    const Value& getValue() const {
        return _value;
    }
    void setValue(const Value &value) {
        _value = value;
    }

public:
    Value _value;
};

Now, the stack class can use the value independent interface. This simplifies everything.

Flovdis
  • 2,945
  • 26
  • 49
  • Sorry, I don't get your point. I want to put those basic Node operation, like addNext, addPrev, delete, in the basic Node class, because they are basic operation of nodes and I don't want to put it somewhere else. However, these operations work only on class Node. All the inheritance of Node Class cannot make use of those functions. I don't know how to solve this problem. – Shanpei Zhou Mar 03 '14 at 20:41