Implementing a proper linked list for a professional environment

I have some concerns:

-
Is it normal for the class to have at least one node? In other words, this implementation cannot have an empty linked list; the length is always 1.

-
Would this be a proper way to implement a linked list in, say, a professional environment? Or perhaps there is a better way to do it? I am not a professional in any way, mind you, just a college freshman.

Implementation file:

```
#include "ListNode.h"
#include
#include

using namespace std;

ListNode::ListNode()
{
}

ListNode::ListNode(int value, ListNode* node){
this->value = value;
this->next = node;
}

void ListNode::setNext(ListNode* node){
this->next = node;
}

void ListNode::setValue(int value){
this->value = value;
}

ListNode* ListNode::getNext(){
return this->next;
}

int ListNode::getValue(){
return this->value;
}

int ListNode::getLength(){
if (this != NULL){//if this is not NULL
if ((*this).getNext() == 0)
return 1;
else{
ListNode* temp = this;
int count = 0;
while (temp != 0){
count++;
temp = (*temp).getNext();
}//end while
temp = NULL;
return count;
}//end else
}//end if
return 0;
}

void ListNode::replace(int position, int value){
ListNode* temp = this;
if (position > -1 && position -1 && position -1 && position < getLength() + 1){
if (position == 0){
addInfront(value);
}//end if
else
if (position == getLength()){
addAppend(value);
}//end else if
else{
for (int i = 0; i < position - 1; i++){
temp = (*temp).getNext();
}//end for
temp2 = (*temp).getNext();
(*temp).setNext(new ListNode(value, temp2));
}//end else
}//end if
else
cout << "Desired index is out of bounds by " << getLength() - position << " units" << endl;
temp = NULL;

For example, is it normal for the class to have at least one node? In
other words, this implementation cannot have an empty linked list. The
length is always 1.

No, a linked list can be empty (i.e. size 0).

But there are designs that intentionally always have at least one member in the list. This special member is referred to as a sentinel and should not hold any data or be counted as a member of the list in terms of size. So if the only member of the list is a sentinel, the size is zero.

There are design advantages to using a sentinel as the list never has any NULL pointers. This actually makes implementing the list easier (as you don't have to check for the NULL special case).


Lastly, would this be a proper way to implement a linkedList in, say,
a professional environment?

Even without looking at it, I would still have to say that std::list would be preferred in a professional environment as it comes from the Standard Template Library (STL). That said, I'd recommend closely following the STL while studying best practices in the language from any trusted reference material at your disposal. Practicing writing your own list, however, would be helpful for learning how the linked list implementation works under the hood.

After looking at it further, it already looks like an uncommon implementation. There's only one class which, based off the name, seems to resembles a node. However, the implementation seems more like a list itself with the basic aspects of a node, primarily as the private data members. They're two separate entities and should have their own properties, which I will explain below.

You should have two main entities - the linked list itself (as a class) and a node (as a class or as a struct). The node structure should be a private member of the linked list class. The node just has two data members - a value (of some type or a templated type) and a pointer to the next node (and the previous node if it's a doubly-linked list). The linked list class handles the list operations and maintains a pointer to the head node, which should also be private. The head node points to NULL (or a sentinel) if the list is empty, or the first node if it's not. It should also maintain a private size, based on the number of nodes (excluding a sentinel). The size should also be accessible to the client via a public accessor ("getter") function.

Moreover, I'll point out some general flaws and other points in your code with no regard to the above:

-
Prefer nullptr to NULL if you're using C++11.

-
Your accessors (or "getters") should be const. This prevents data members from possibly being modified in the function. This is important because you want to make sure the accessors return the unchanged data member in order to avoid bugs. This also helps ensure const correctness.

-
Linked lists add nodes by allocating new memory. In C++, that's done with new. You also need to deallocate the memory with delete when you're finished with it. If you do not, you'll experience memory leaks. This deallocation should be done in the destructor, specifically with a loop that destroys each node with delete.

-
You're interchanging pointer->member and (*pointer).member. Prefer the -> operator as it is more readable and "less-awkward" than the other.

StackExchange Code Review Q#37475, answer score: 27