4 Answers
4

Please stop doing this. when you add using namespace directives in header files, any file including that header will have potential namespace clashes (which is the problem namespaces are trying to prevent).

Implementing a linked list without a dummy head node should be very, very high on the list of "do nots" in my opinion. Just means the code needs tons of special cases for no good reason.
–
VooMar 6 '14 at 15:38

@Voo, can you please be more specific? My proposed implementation doesn't require (or impose) a dummy head, nor special cases in processing (as far as I can tell). Maybe I am missing something?
–
utnapistimMar 6 '14 at 15:50

1

I just tried writing an append method for the list and saw what you mean: an implementation with a dummy head, will ensure that in the situations where you implement iteration (Node *p = head; while(p->next) ...) you don't need to check the validity of the iterator before accessing 'next'.
–
utnapistimMar 6 '14 at 15:59

1

If you're going to do Node(int value=0)... you really must make it an explicit constructor - explicit Node(int value=0) - or you will create an implicit casting between your list node and its payload and you do not want that. In fact, in general, you should get in the habit of always putting explicit in front of single parameter constructors unless you really, really want casting.
–
Jack AidleyMar 6 '14 at 17:11

Well, there are many problems in the code. Does this compile and run correctly? Some things I have noticed:

Make the member variables in Node private as they are not needed outside. And if they were, you should probably provide getter and setter methods.

The member variable Node::head should be static because the head is ALWAYS the same. The way you are doing it now each instance of Node (so each Node) has its own head. This is not only waste of memory but also wrong. With static there is only one instance of Node *head in the memory. (Of course in that case you must not set head to NULL). Notice that this solution has the disadvantage that you can only have one list at a time. But to solve this there are only two possibilities:

Leave it non static. But then you have to adjust the member variable in each node once the head changes (very costly). Moreover (as mentioned above) you waste memory.

As suggested by the other answers: make two classes. A class for the List and a (nested) class for the node. The head pointer is then a member of the List (not the Node anymore). This way you can instantiate many Lists with different heads.

The latter is obviously the better solution an is the common way to solve that problem.

Another thing that is seen as better practice is to use initialization lists (read about it). In short:
An initialization list initializes (as the name suggests) variables on construction.

And many other things. Including but not limited to the following:

void Node::addValueLeft(int nVal)
{
Node *newNode = new Node;
newNode->nX = nVal;
newNode->next = head; // set to old head. If head is NULL, it is OK, too.
if(!head) // if list is empty, make head pointing at the new node
head = newNode;
}
void Node::printList()
{
// if is unnecessary here. If list is empty the while loop
// is skipped anyway.
Node *tempNode = head; // no new Node here
while(tempNode)
{
cout << tempNode-> nX << endl;
tempNode = tempNode->next;
}
}
void Node::addValueRight(int nVal)
{
Node *tempNode1; // no good naming and no need for new Node
Node *tempNode2 = new Node; //create temp node
tempNode2 -> nX = nVal;
tempNode2 -> next = NULL;
if(head == NULL){ // if list is empty, the new Node is the first Node
head = tempNode2;
}else{ // if there are Nodes in the list ...
tempNode1 = head;
while(tempNode1 -> next != NULL) // ... Go to last Node
{
tempNode1 = tempNode1->next;
}
tempNode1 -> next = tempNode2; //tempNode2 will be last Node
}
}

I would really like to help more but I think you have to read more about the basics. Most important: read much much much about pointers. Moreover read about new and delete.
If you have more (specific) questions feel free to ask.

+1; but head cannot be static if there is more than one List in the program.
–
ChrisWMar 6 '14 at 14:27

2

A static head member in the Node class is so wrong that you should edit it out of your answer entirely.
–
PeterMar 6 '14 at 15:05

@Peter on the one hand you are right. But on the other hand it is much better than the solution from the OP. But I will think about it and edit my answer to something more secure.
–
exilitMar 6 '14 at 15:28

@exilit The OPs code may not be great but at least correct (ok, there's at least a chance it may be correct at least). Making the head node static on the other hand is a bug in any way you think about it.
–
VooMar 6 '14 at 15:41

1

Putting a pointer to the head in a node is just plain bad design (since it prevents many of the operations that make lists worthwhile speedily). Doing it statically is even worse (since it limits you to one list in your whole program!).
–
Jack AidleyMar 6 '14 at 17:13

Aside from the problems with the code itself as pointed out in the other answers, I would like to comment on your interface naming. There are a few common names for linked list operations that any experienced programmer will expect:

push / pop for stacks (first in, last out): Add an element to the end of the list, or remove the last added element from the end of the list

queue/ dequeue for queues (first in, first out lists): Add an element to the start of the list and remove one from the end of the list.

shift / unshift - I am not sure how common these terms are, but for example PHP uses this as the analog to push and pop for operations on the start of the list.