Is it OK to make node struct public within stack class?

[niko79542]

I wrote a stack data structure. It is a series of nodes . Each node has a value, and a pointer to its parent node...and child node.

Is it bad to make variables public within a class? Is it ok that I include the node struct in public? Do I need to make this private? Here is my stack.h header file:

class stack {
	public:
		struct node {
			int value;
			node* child;
			node* parent;
		};						//our definition of a node
		void setSize(int x) {
			size = x;
		}							//modifies the size of the stack
		int getSize() {
			return size;
		}							//returns the size of the stack
		stack();					//constructor

		node * getHead() {
			return head;
		}							//returns pointer to the stack's head

		void CreateNew (int x);  	//creates new node with a value x
		void PrintS (); 			//Prints the stack
		int RemoveParent ();		//adds new node at the end
		void AddParent (int x);		//removes parent node, returns old value

	protected:
	private:
		int size = 0;
		stack::node* head = NULL;
};

[E0p9LyTq]

Having class data members as public is not wrong, but it is not considered the best OOP design.

http://stackoverflow.com/questions/2977007/public-data-members-vs-getters-setters

[Peter87]

Do you want the node class to be used from outside the class? If yes, then make it public. If node is just something that the stack class uses internally and never expose to the users of the class, then I think it is best to make it private.

[niko79542]

Thanks for the friendly answers. Was simpler than I thought.

The only issue was that I had a function in the stack class which returned a pointer to the head node. I couldn't figure out how to do this after I had made node struct private. My solution was to have that function return the value of the head node, since that's all I really needed:

class stack {
	public:
		void setSize(int x) {
			size = x;
		}							//modifies the size of the stack
		int getSize() {
			return size;
		}							//returns the size of the stack

		int getHead() {
			return head -> value;
		}						//returns head value

		void CreateNew (int x);  	//creates new node with a value x
		void PrintS (); 			//Prints the stack
		int RemoveParent ();		//removes parent node
		void AddParent (int x);		//adds parent node

	protected:
	private:
		struct node {
			int value;
			node* child;
			node* parent;
		};							//our definition of a node
		int size = 0;
		node* head = NULL;
};

[doug4]

Just out of curiosity, what is "size"? Is that the current number of nodes on the stack, or is it the maximum number of elements the stack can hold? If it is the number of elements currently on the stack, why on earth would you have a setter function? You should only change the value when you add remove an element. If it is the maximum number of elements the stack can hold, you might want to change the name in order to remove ambiguity (like I'm questioning right now).

I'm also curious what a Parent is? Do you mean the top node? Generally when you are talking about stacks, the functions are called "push" and "pop". Based on your header file, it seems like AddParent() and RemoveParent() are your implementations of push() and pop(). I would recommend that you rename your functions to more common terminology to make it easier for others to understand what you are doing (and for you to understand 8 months from now when you come back to this code after a long hiatus).

If I'm right about Parent, I wonder what CreateNew does. Is it the same as AddParent? Does it need to be in the public space of the class?

I would recommend that you start from traditional stack terminology, and implement a basic stack with the following public interface:

class stack {
public:
    int getSize();
    int getHead();
    void push(int x);
    void pop();
    void printS(); // why not?  it's a good for verifying your work.
private:
    ...
};

[niko79542]

Yes Doug, all of your assumptions about my stack were correct. Much simpler, thank you.

stack.h:

class stack {
public:
	//returns stack size
	int getSize() {
		return size;
	}	
	//returns head value
	int getHead() {
		return head -> value;
	}
	//adds top node with value x
	void push (int x);
	//removes top node
	void pop ();
	//prints out stack for debugging
	void PrintS ();

protected:
private:
	//definition of a node
	struct node {
		int value;
		node* child;
		node* parent;
	};
	//initializes and tracks size of the stack
	int size = 0;
	//initializes and tracks head value of the stack
	node* head = NULL;
};

stack.cpp

#include <iostream>
#include <stdlib.h>
#include "stack.h"

using namespace std;

void stack::push (int x) {

	//new node's:
		// parent is itself
		// child is the old head
		// value  is x

	node* NewNode;
	NewNode = new node;

	NewNode -> value = x;
	NewNode -> parent = NewNode;
	NewNode -> child = head;

	//head is now new node
	head = NewNode;

	//bump size
	size++;
	return;
}

void stack::pop () {

	//throw an exception if the user tries to remove a node 
		//if stack is already empty
	try {
		if (size == 0)
			throw 99;
	} catch (int x) {
		cout << "stack is already size 0, ERROR: " << x << endl;
	}

	//new head is the old head's child
	head = head -> child;
	size--;

	return;
}

void stack::PrintS ( ) {

	//declares a temporary node
	stack::node* temp;
	temp = head;	

	//loop through the stack using temp node...setting it first to head
		//...and then to each child until NULL...
	while (temp != NULL) {
		cout  << temp -> value << " (" << temp << ")  ";
		temp = temp -> child;
	}
	cout << endl;
}